[keycloak-dev] Remove TokenManager.verifyAccess method?

[Stian Thorgersen]

Sounds good to me.

On Tue, 27 Nov 2018, 07:48 Marek Posolda <mposolda at redhat.com wrote:

> 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>
> 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> 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>
>> > wrote:
>> >
>> >> +1
>> >>
>> >> On Wed, 14 Nov 2018 at 09:09, Marek Posolda <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
>> >>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>> >>>
>> >>
>> >
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>
>