[
https://issues.apache.org/jira/browse/ISIS-2844?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Keir Haywood updated ISIS-2844:
--------------------------------------
Description:
_*Analysis:*_
when sudo service runs and secman is configured, the effective permissions are
obtained from the ApplicationUser object, and whichever ApplicationRoles that
ApplicationUser happens to have.
in contrast, when impersonating then the permissions are obtained from the
UserMemento + associated RoleMementos.
For consistency, I think that possibly sudo service should also use the
UserMemento to obtain the roles in effect.... though see additional analysis
below, I'm not 100% sure on this.
(Note: if not running under sudo service and not impersonating, then we also
use the roles from usermemento; but these would have been copied from the
ApplicatoinUser on login).
In terms of change to the user experience, because a `UserMemento` is immutable
and is only populated on login from the `ApplicationUser`, and that it contains
the roles, then the user will need to logout and login if they are added to any
new roles while logged in. I think this is acceptable.
On the other hand... we do need to understand all of the code paths that are
affected here. There are 4 callers to `ApplicationUser#getPermissionSet()`:
!image-2022-08-20-11-20-02-704.png|width=601,height=177!
This issue was raised originally in the context of `AuthorizorSecman#grants` to
determine the effective permissoin, where it looks up the current
`ApplicatoinUser` but the `ApplicationUser` (as currently implemented) is aware
that impersonation can happen and if so to use the `UserMemento` rather than
itself. But what of the other 3 callers? So it might be that the knowledge
about impersonatoin should reside in the callers, eg in
`AuthorizorSecman#grants` rather than in `ApplicationUser#getPermissionSet()`.
So, more analysis needed, I think.
~~~
_*Implementation (if decide to go ahead):*_
In terms of code, it's pretty trivial I think; we just remove the check for
userService.isImpersonating() below and always run the first branch, ie query
`byUserMemento(...)`. The `byUser(...)` method is probably therefore redundant
and could be removed. See code snippet below.
!image-2021-08-18-16-24-04-978.png|width=879,height=376!
was:
_*Analysis:*_
when sudo service runs and secman is configured, the effective permissions are
obtained from the ApplicationUser object, and whichever ApplicationRoles that
ApplicationUser happens to have.
in contrast, when impersonating then the permissions are obtained from the
UserMemento + associated RoleMementos.
For consistency, I think that possibly sudo service should also use the
UserMemento to obtain the roles in effect.
(Note: if not running under sudo service and not impersonating, then we also
use the roles from usermemento; but these would have been copied from the
ApplicatoinUser on login).
In terms of change to the user experience, because a `UserMemento` is immutable
and is only populated on login from the `ApplicationUser`, and that it contains
the roles, then the user will need to logout and login if they are added to any
new roles while logged in. I think this is acceptable.
On the other hand... we do need to understand all of the code paths that are
affected here. There are 4 callers to `ApplicationUser#getPermissionSet()`:
!image-2022-08-20-11-20-02-704.png!
~~~
_*Implementation:*_
In terms of code, it's pretty trivial I think; we just remove the check for
userService.isImpersonating() below and always run the first branch, ie query
`byUserMemento(...)`. The `byUser(...)` method is probably therefore redundant
and could be removed. See code snippet below.
!image-2021-08-18-16-24-04-978.png|width=879,height=376!
> With Secman, SudoService behaves differently from impersonation - should be
> consistent.
> ---------------------------------------------------------------------------------------
>
> Key: ISIS-2844
> URL: https://issues.apache.org/jira/browse/ISIS-2844
> Project: Isis
> Issue Type: Improvement
> Components: Isis Extensions SecMan
> Affects Versions: 2.0.0-M6
> Reporter: Daniel Keir Haywood
> Priority: Minor
> Fix For: 2.0.0-RC1
>
> Attachments: image-2021-08-18-16-24-04-978.png,
> image-2022-08-20-11-20-02-704.png
>
>
> _*Analysis:*_
> when sudo service runs and secman is configured, the effective permissions
> are obtained from the ApplicationUser object, and whichever ApplicationRoles
> that ApplicationUser happens to have.
> in contrast, when impersonating then the permissions are obtained from the
> UserMemento + associated RoleMementos.
> For consistency, I think that possibly sudo service should also use the
> UserMemento to obtain the roles in effect.... though see additional analysis
> below, I'm not 100% sure on this.
> (Note: if not running under sudo service and not impersonating, then we also
> use the roles from usermemento; but these would have been copied from the
> ApplicatoinUser on login).
> In terms of change to the user experience, because a `UserMemento` is
> immutable and is only populated on login from the `ApplicationUser`, and that
> it contains the roles, then the user will need to logout and login if they
> are added to any new roles while logged in. I think this is acceptable.
>
> On the other hand... we do need to understand all of the code paths that are
> affected here. There are 4 callers to `ApplicationUser#getPermissionSet()`:
> !image-2022-08-20-11-20-02-704.png|width=601,height=177!
> This issue was raised originally in the context of `AuthorizorSecman#grants`
> to determine the effective permissoin, where it looks up the current
> `ApplicatoinUser` but the `ApplicationUser` (as currently implemented) is
> aware that impersonation can happen and if so to use the `UserMemento` rather
> than itself. But what of the other 3 callers? So it might be that the
> knowledge about impersonatoin should reside in the callers, eg in
> `AuthorizorSecman#grants` rather than in
> `ApplicationUser#getPermissionSet()`.
> So, more analysis needed, I think.
> ~~~
> _*Implementation (if decide to go ahead):*_
> In terms of code, it's pretty trivial I think; we just remove the check for
> userService.isImpersonating() below and always run the first branch, ie query
> `byUserMemento(...)`. The `byUser(...)` method is probably therefore
> redundant and could be removed. See code snippet below.
> !image-2021-08-18-16-24-04-978.png|width=879,height=376!
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
