Skip to content

Conversation

achour94
Copy link
Contributor

@achour94 achour94 commented May 12, 2025

  • This PR enhances API security by implementing token audience validation against a configurable whitelist for both JWT and opaque tokens, ensuring only authorized applications can access the gateway.
    Split audience validation into two separate lists:
    * allowedAudiences: Exclusively for validating the aud claim in JWT tokens
    * allowedClients: For validating client_id claim in JWT tokens and client IDs in opaque tokens
  • Profile claims from JWT tokens are now extracted and added as roles headers to downstream requests, enabling role-based authorization across backend services.
  • User admin validation checks were removed.

Signed-off-by: achour94 <berrahmaachour@gmail.com>
achour94 added 2 commits May 14, 2025 17:44
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Signed-off-by: achour94 <berrahmaachour@gmail.com>

if (maybeSubList != null && !maybeSubList.isEmpty()) {
String sub = maybeSubList.getFirst();
return userAdminService.userRecordConnection(sub, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do fire and forget instead of waiting for the recorded user connection now that we don't need the response anymore


public UserAdminControlGlobalPreFilter(UserAdminService userAdminService) {
this.userAdminService = userAdminService;
super(userAdminService);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment for this class saying that this comes after other filters (getOrder), and we don't use it to reject request, only to record connection. Optionally remove the dead code that rejects (clientList and else block at the end) or add a comment saying it can't happen and needs to be refactored

return userAdminService.userExists(sub).flatMap(userExist -> Boolean.TRUE.equals(userExist) ? chain.filter(exchange) : completeWithCode(exchange, HttpStatus.FORBIDDEN));
// Record the connection with isConnectionAccepted=true
// and continue with the filter chain regardless of the result
return userAdminService.userRecordConnection(sub, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fire and forget

* @param exchange The server web exchange
* @return Mono<Void> with unauthorized response if validation fails, null if validation passes
*/
private Mono<Void> validateAudienceOrClientId(JWTClaimsSet jwtClaimsSet, ServerWebExchange exchange) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better to return a synchronous value (boolean true false, or void/exception like the lib validator) and completeWithCode in the caller

* @param exchange The server web exchange
* @return Mono<Void> with unauthorized response if validation fails, null if validation passes
*/
private Mono<Void> validateClientId(String clientId, ServerWebExchange exchange) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same Maybe better to return a synchronous value (boolean true false, or void/exception like the lib validator) and completeWithCode in the caller

@@ -1,3 +1,5 @@
allowed-issuers: #, #
allowed-issuers: http://172.17.0.1:9090
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to application-local.yml ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also we have "localhost" everywhere (except once in explore-server, probably no reason), instead of the docker localhost. Maybe keep that for homogeneity

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just move it to the docker compose deployment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's already in deployment, just does not work when we don't work with docker images

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we check issuers using String.startsWith() so localhost will not work in this case since issuer in the token is 172.17.0.1

@@ -1,3 +1,5 @@
allowed-issuers: #, #
allowed-issuers: http://172.17.0.1:9090
allowed-audiences: gridexplore-client, gridadmin-client, griddyna-client, gridstudy-client, gridexplore-local, gridadmin-local, griddyna-local, gridstudy-local
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to put this in the docker compose deployment only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it does not work if we start gateway locally using intellij !

achour94 added 6 commits May 14, 2025 23:22
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Signed-off-by: achour94 <berrahmaachour@gmail.com>
@achour94 achour94 requested a review from TheMaskedTurtle May 15, 2025 13:27
achour94 added 2 commits May 15, 2025 16:40
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Signed-off-by: achour94 <berrahmaachour@gmail.com>
@achour94 achour94 requested a review from TheMaskedTurtle May 15, 2025 14:56
Copy link
Contributor

@TheMaskedTurtle TheMaskedTurtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the isValidAudienceOrClientId method is missing the by-pass for Demo env, where we don't perform audiance check if allowedAudiences is empty

Signed-off-by: achour94 <berrahmaachour@gmail.com>
@achour94 achour94 requested a review from TheMaskedTurtle May 19, 2025 13:46
Signed-off-by: achour94 <berrahmaachour@gmail.com>
Copy link

@achour94 achour94 merged commit 8eeedc2 into main May 20, 2025
4 checks passed
@achour94 achour94 deleted the use-token-profile-claims branch May 20, 2025 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants