-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(grant): enhance user on ExtensionGrant to get dynamic scopes #935
base: 2.10.x
Are you sure you want to change the base?
feat(grant): enhance user on ExtensionGrant to get dynamic scopes #935
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @micivray thanks for the PR, you can check my comments.
@@ -124,6 +128,7 @@ public boolean handle(String grantType, Client client) { | |||
user.setRoles(idpUser.getRoles()); | |||
return user; | |||
}) | |||
.flatMap(user -> userService.enhance(user).toMaybe()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @micivray, you cannot use directly the enhance
method because the current user hold the ID of the IDP user (external_id). You need first to retrieve the internal user from Gravitee, here is a snippet
.flatMap(user -> userService.findByDomainAndExternalIdAndSource(domain, user.getId(), idp)
.flatMap(userService.enhance(user).toMaybe())
.defaultEmpty(user))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you @tcompiegne. I didn't think that I needed to load any other information as it was working as is (local test with a LDAP IDP) just setting: user id with idpUser id.
The problem is that if we try to load user from userService we don't find him as the user is not saved in our case.
But what I can do is to extract from enhance
only what is interesting for us, to call only roleService and not usrService as you explained that is not correct:
if (!roles.isEmpty()) {
return roleService.findByIdIn(new ArrayList<>(roles))
.map(roles1 -> {
user.setRolesPermissions(roles1);
return user;
});
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi TItouan!
Here is a little update if this can help understand why @micivray is struggling with the suggestion you made.
Our flow is:
(end user / RO) --> Client App --> Corporate OIDC server --> get an ID_TOKEN, then used as a JWT bearer --> (Gravitee APIM) --> custom API inserting the user identity into an OpenLDAP, within a group carried by one of the JWT Bearer claim --> Gravitee AM --> OpenLDAP (as IdP) to fetch the user's group --> Gravitee AM to map user's group to a role and to dynamically add scopes
We have a more detailed procedure (24 pages, step-by-step) on how-to reproduce the bug detailed in issue gravitee-io/issues#3534, that we can share privately.
So here, as @micivray said, the "user is not saved in our case", as you expect in your suggestion.
@tcompiegne what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see my proposal with the fix below to enhance user roles and permissions without calling the enhance method with the wrong ID.
In the case of ExtensionGrantGranter.resolveResourceOwner the user's roles permissions are not loaded, whereas we expect them to be added to user's scope.
These permissions/scopes are well loaded by AuthorizationCodeGranter or RefreshTokenGranter for example.
An other solution that the one proposed could be to modify the AbstractTokenGranter grant method but it didn't seem relevant as the issue is only on extension grant.