Skip to content
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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: 2.10.x
Choose a base branch
from
Draft

feat(grant): enhance user on ExtensionGrant to get dynamic scopes #935

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a311b45
feat(grant): enhance user on ExtensionGrant to get dynamic scopes
micivray Oct 9, 2020
31632ba
fix(grant): enhance only user's rolesPermissions
micivray Oct 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...vitee/am/gateway/handler/oauth2/service/granter/extensiongrant/ExtensionGrantGranter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import io.gravitee.am.extensiongrant.api.ExtensionGrantProvider;
import io.gravitee.am.gateway.handler.common.auth.UserAuthenticationManager;
import io.gravitee.am.gateway.handler.common.auth.idp.IdentityProviderManager;
import io.gravitee.am.gateway.handler.common.user.UserService;
import io.gravitee.am.gateway.handler.oauth2.exception.InvalidGrantException;
import io.gravitee.am.gateway.handler.oauth2.exception.UnauthorizedClientException;
import io.gravitee.am.gateway.handler.oauth2.service.granter.AbstractTokenGranter;
Expand Down Expand Up @@ -55,14 +56,16 @@ public class ExtensionGrantGranter extends AbstractTokenGranter {
private final ExtensionGrant extensionGrant;
private final UserAuthenticationManager userAuthenticationManager;
private final IdentityProviderManager identityProviderManager;
private final UserService userService;
private Date minDate;

public ExtensionGrantGranter(ExtensionGrantProvider extensionGrantProvider,
ExtensionGrant extensionGrant,
UserAuthenticationManager userAuthenticationManager,
TokenService tokenService,
TokenRequestResolver tokenRequestResolver,
IdentityProviderManager identityProviderManager) {
IdentityProviderManager identityProviderManager,
UserService userService) {
super(extensionGrant.getGrantType());
setTokenService(tokenService);
setTokenRequestResolver(tokenRequestResolver);
Expand All @@ -71,6 +74,7 @@ public ExtensionGrantGranter(ExtensionGrantProvider extensionGrantProvider,
this.extensionGrant = extensionGrant;
this.userAuthenticationManager = userAuthenticationManager;
this.identityProviderManager = identityProviderManager;
this.userService = userService;
}

@Override
Expand Down Expand Up @@ -124,6 +128,7 @@ protected Maybe<User> resolveResourceOwner(TokenRequest tokenRequest, Client cli
user.setRoles(idpUser.getRoles());
return user;
})
.flatMap(user -> userService.enhance(user).toMaybe())
Copy link
Member

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))

Copy link
Author

@micivray micivray Oct 12, 2020 •
edited
Loading

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;
                                });
 }

Copy link

@bcollard bcollard Oct 14, 2020 •
edited
Loading

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?

Copy link
Author

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.

.switchIfEmpty(Maybe.error(new InvalidGrantException("Unknown user: " + endUser.getId())));
} else {
User user = new User();
Expand Down
6 changes: 5 additions & 1 deletion ...gateway/handler/oauth2/service/granter/extensiongrant/impl/ExtensionGrantManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import io.gravitee.am.common.event.ExtensionGrantEvent;
import io.gravitee.am.gateway.handler.common.auth.UserAuthenticationManager;
import io.gravitee.am.gateway.handler.common.auth.idp.IdentityProviderManager;
import io.gravitee.am.gateway.handler.common.user.UserService;
import io.gravitee.am.gateway.handler.oauth2.service.granter.CompositeTokenGranter;
import io.gravitee.am.gateway.handler.oauth2.service.granter.TokenGranter;
import io.gravitee.am.gateway.handler.oauth2.service.granter.extensiongrant.ExtensionGrantGranter;
Expand Down Expand Up @@ -81,6 +82,9 @@ public class ExtensionGrantManagerImpl extends AbstractService implements Extens

@Autowired
private EventManager eventManager;

@Autowired
private UserService userService;

@Override
public void afterPropertiesSet() {
Expand Down Expand Up @@ -163,7 +167,7 @@ private void updateExtensionGrantProvider(ExtensionGrant extensionGrant) {
}
ExtensionGrantProvider extensionGrantProvider = extensionGrantPluginManager.create(extensionGrant.getType(), extensionGrant.getConfiguration(), authenticationProvider);
ExtensionGrantGranter extensionGrantGranter = new ExtensionGrantGranter(extensionGrantProvider, extensionGrant,
userAuthenticationManager, tokenService, tokenRequestResolver, identityProviderManager);
userAuthenticationManager, tokenService, tokenRequestResolver, identityProviderManager, userService);
// backward compatibility, set min date to the extension grant granter to choose the good one for the old clients
extensionGrantGranter.setMinDate(minDate);
((CompositeTokenGranter) tokenGranter).addTokenGranter(extensionGrant.getId(), extensionGrantGranter);
Expand Down