[keycloak-dev] Remove TokenManager.verifyAccess method?

[Marek Posolda]

Dne 26. 11. 18 v 13:57 Pedro Igor Silva napsal(a):
> Marek, please correct me if I wrong.
>
> What Marek is suggesting is remove role check entirely and only check 
> for user consents. The reason is that from an OAuth2 perspective we 
> *only* need to make sure that user consents are still valid.
>
> With client scopes, the user no longer consents access to individual 
> roles but scopes where the actually granted roles could depend on the 
> granted scopes. In case roles are granted directly to a user, during a 
> refresh the new token will reflect the current granted roles and it 
> does not really matter if the RT was issued before with less or more 
> roles as the resulting AT will reflect the actual granted roles.

Yes, exactly

Example scenario:

- User authenticated to some client and the token was issued to him with 
roles ["role1", "role2"]

- Admin removed the role "role1" from user role mappings (or from client 
scope role mappings of client)

- Now refreshToken request is sent. I think at this moment new token can 
be issued, which won't contain "role1", but will contain just single 
role ["role2"] . What we're doing now is that refreshToken request is 
rejected due the TokenManager.verifyAccess and that's the behaviour, 
which I propose to change and remove this TokenManager.verifyAccess. 
Once that is removed, we will have expected behaviour (just "role2" will 
be present in the token).

>
> Regards.
> Pedro Igor
>
> On Mon, Nov 26, 2018 at 6:02 AM Stian Thorgersen <sthorger at redhat.com 
> <mailto:sthorger at redhat.com>> wrote:
>
>     I think it's better if the old role is just removed. If you think
>     about it
>     the new access token is sent to a service in most cases and the
>     service
>     only has that new token as a reference for what roles the user has
>     anyways.
>
>     I don't understand what you mean about "it is sufficient to check
>     consents
>     rather than roles". Both need to be checked, always. Consents
>     limits the
>     access, while role is the permissions the user/client have.
>
I meant that refreshToken request should be rejected if it requires some 
clientScopes, which user doesn't have consent anymore. Which can happen 
if consent was rejected in the meantime. We're already doing it. Another 
example scenario:

- User authenticated to some client and the token was issued to him. On 
the consent screen he approved scopes "profile" and "email". The token 
will contain those client scopes "profile email"

- Consent was removed from the user (for example revoked by the user in 
account management)

- RefreshToken request is sent. At this moment, refreshToken request 
should be rejected as user doesn't have required consents. And that's 
what we're doing.

For roles, we don't need to reject the refreshToken request, but just 
remove the roles, which are not available anymore from the token as 
mentioned above.

Marek

>
>     On Mon, 26 Nov 2018 at 08:53, Marek Posolda <mposolda at redhat.com
>     <mailto:mposolda at redhat.com>> wrote:
>
>     > Yes, it is updated. And new token can contain some more roles, which
>     > weren't presented before on the old refresh token. However if
>     the newToken
>     > doesn't contain any role, which was present in the old refresh
>     token, then
>     > refreshToken request is rejected ATM. That's what I think is not
>     great
>     > behaviour as it is sufficient to check consents rather than roles.
>     >
>     > Marek
>     >
>     > On 26/11/2018 08:46, Stian Thorgersen wrote:
>     >
>     > If I'm not mistaken the token is already updated with new roles
>     today.
>     >
>     > On Mon, 26 Nov 2018 at 08:44, Stian Thorgersen
>     <sthorger at redhat.com <mailto:sthorger at redhat.com>>
>     > wrote:
>     >
>     >> +1
>     >>
>     >> On Wed, 14 Nov 2018 at 09:09, Marek Posolda
>     <mposolda at redhat.com <mailto:mposolda at redhat.com>> wrote:
>     >>
>     >>> Right now, during each token refresh, we're verifying if the newly
>     >>> refreshed access token still contains all the roles, which
>     were present
>     >>> in the refresh token. If not, the refresh token is rejected.
>     >>>
>     >>> I wonder if this check can be removed? This will also allow us
>     to remove
>     >>> the roles (realm_access and resource_access claims) from the
>     refresh
>     >>> token. Anyone knows a reason if this check can't be removed?
>     >>>
>     >>> I think the reason why this check was originally added is due the
>     >>> consent. Previously we did not have clientScopes and the
>     consents on the
>     >>> consent screen were represented by individual roles and
>     protocolMappers.
>     >>> However with clientScopes, this seem to be obsolete IMO.
>     >>>
>     >>> During token refresh, we should check that consents represented by
>     >>> clientScopes in the refresh token were not revoked by the user (or
>     >>> admin). If they were rejected, the refresh token should be
>     rejected.
>     >>> We're doing this. However if some individual role was removed
>     from the
>     >>> user (or from the role scope mappings), I don't see an issue with
>     >>> successfully refresh token and just ensure that the revoked
>     role is not
>     >>> in the new token anymore.
>     >>>
>     >>> WDYT?
>     >>>
>     >>> Marek
>     >>>
>     >>> _______________________________________________
>     >>> keycloak-dev mailing list
>     >>> keycloak-dev at lists.jboss.org <mailto:keycloak-dev at lists.jboss.org>
>     >>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>     >>>
>     >>
>     >
>     _______________________________________________
>     keycloak-dev mailing list
>     keycloak-dev at lists.jboss.org <mailto:keycloak-dev at lists.jboss.org>
>     https://lists.jboss.org/mailman/listinfo/keycloak-dev
>