Encode the user’s roles directly into the JWT #722
Conversation
| @@ -104,9 +104,10 @@ public HaGatewayProviderModule(HaGatewayConfiguration configuration) | |||
| Map<String, UserConfiguration> presetUsers = configuration.getPresetUsers(); | |||
|
|
|||
| oauthManager = getOAuthManager(configuration); | |||
| formAuthManager = getFormAuthManager(configuration); | |||
There was a problem hiding this comment.
inside getFormAuthManager(configuration) i am using authorizationManager variable. that is why i want to initialize authorizationManager first and then call that method
| catch (Exception e) { | ||
| return Optional.empty(); | ||
| } | ||
| String userId = claims.get(userIdField).asString().replace("\"", ""); |
There was a problem hiding this comment.
why do we need to replace \" ?
| Optional<String> memberOf = getMemberOf(username); | ||
| List<String> privileges = new ArrayList<String>(); | ||
|
|
||
| if (authorizationConfiguration.getAdmin() != null) { |
There was a problem hiding this comment.
would it be cleaner to replace the ifs with
Map<String, String> rolePatterns = Map.of(
"ADMIN", authorizationConfiguration.getAdmin(),
"USER", authorizationConfiguration.getUser(),
"API", authorizationConfiguration.getApi()
);
rolePatterns.forEach((role, pattern) -> {
if (pattern != null) {
memberOf.filter(m -> m.matches(pattern)).ifPresent(m -> privileges.add(role));
}
});
There was a problem hiding this comment.
We want to use cache for the performance reason and we also need to prevent stale data, so we should cache the results of the LbLdapClient.getMemberOf method using a Guava cache that invalidates at regular intervals.
Similarly, when using token based claims for authorization, we should treat these claims as memberOf attributes. This would distinguish the roles supported by the gateway from the external system providing the privileges.
| { | ||
| this.name = name; | ||
| this.memberOf = memberOf; | ||
| this.privileges = privileges; |
There was a problem hiding this comment.
It's not a good idea to set the privileges directly in the Principal. The privileges should be calculated dynamically. If any privilege for a user changes, we need to invalidate the session which is going to be difficult. This is especially true with the LDAP based privileges.
| throw new AuthenticationException("No privileges found for user " + userId + " in idToken"); | ||
| } | ||
|
|
||
| String privileges = claim.asString(); |
There was a problem hiding this comment.
Rather than setting the privileges, let's set the membefOf and everything should fall in place.
| public String getPrivileges(String username) | ||
| { | ||
| if (authorizationConfiguration == null) { | ||
| return "ADMIN_USER_API"; |
There was a problem hiding this comment.
I’d prefer not to give any privileges when authorizationConfiguration is not set.
Description
Currently, when navigating between pages in the Trino-gateway UI, each page transition triggers API calls to the gateway. For every API call, a new LbPrincipal is generated by decoding the JWT and fetching the memberOf attribute from LDAP. This causes unnecessary and excessive LDAP queries on every API request, even within the same authenticated session.
To reduce the LDAP calls , i have encoded the roles in the JWT claims and decode it instead of calling LDAP server.
Note: we should use short-lived JWTs to minimize the risk of stale role data
#715
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required, with the following suggested text: