From 926330f7e37e7d4fbde8da96f5c190aca016810d Mon Sep 17 00:00:00 2001 From: GeorgeC Date: Tue, 28 Jan 2025 09:04:10 -0500 Subject: [PATCH 1/3] Refactor RAS authentication logic for better modularity --- .../dbmi/avillach/auth/filter/JWTFilter.java | 3 - .../RASAuthenticationService.java | 56 +++++++++++-------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/filter/JWTFilter.java b/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/filter/JWTFilter.java index 115d180b9..a4b282a2e 100755 --- a/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/filter/JWTFilter.java +++ b/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/filter/JWTFilter.java @@ -197,10 +197,7 @@ private void setSecurityContextForUser(HttpServletRequest request, HttpServletRe } } - // Get the user's roles Set userRoles = authenticatedUser.getUser().getRoles(); - - // Check if the user has any roles and privileges associated with them if (userRoles == null || userRoles.isEmpty() || userRoles.stream().allMatch(role -> CollectionUtils.isEmpty(role.getPrivileges()))) { logger.error("User doesn't have any roles or privileges."); try { diff --git a/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationService.java b/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationService.java index d61d4698e..bda9a6413 100644 --- a/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationService.java +++ b/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationService.java @@ -4,7 +4,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; import edu.harvard.hms.dbmi.avillach.auth.entity.Connection; -import edu.harvard.hms.dbmi.avillach.auth.entity.Role; import edu.harvard.hms.dbmi.avillach.auth.entity.User; import edu.harvard.hms.dbmi.avillach.auth.model.ras.Passport; import edu.harvard.hms.dbmi.avillach.auth.model.ras.RasDbgapPermission; @@ -105,46 +104,52 @@ public HashMap authenticate(Map authRequest, Str } User user = initializedUser.get(); + Optional rasPassport = extractAndVerifyPassport(authRequest, introspectResponse, user); + if (rasPassport.isEmpty()) return null; + user = updateUserRoles(authRequest.get("code"), user, rasPassport.get()); + setUserPassport(authRequest, introspectResponse, user); + HashMap responseMap = createUserClaims(user, idToken); + + responseMap.put("oktaIdToken", idToken); + logger.info("LOGIN SUCCESS ___ USER {}:{} ___ WITH ROLES ___ {} ___ AUTHORIZATION WILL EXPIRE AT ___ {} ___ CODE {}", + user.getSubject(), user.getUuid().toString(), + user.getRoles().stream().map(role -> role.getName().replace("MANAGED_", "")).collect(Collectors.joining(",")), + responseMap.get("expirationDate"), authRequest.get("code")); + + return responseMap; + } + + private Optional extractAndVerifyPassport(Map authRequest, JsonNode introspectResponse, User user) { Optional rasPassport = this.rasPassPortService.extractPassport(introspectResponse); if (rasPassport.isEmpty()) { logger.info("LOGIN FAILED ___ NO RAS PASSPORT FOUND ___ USER: {} ___ CODE {}", user.getSubject(), authRequest.get("code")); - return null; + return Optional.empty(); } if (rasPassPortService.isExpired(rasPassport.get())) { logger.error("validateRASPassport() LOGIN FAILED ___ PASSPORT IS EXPIRED ___ USER: {} ___ CODE {}", user.getSubject(), authRequest.get("code")); - return null; + return Optional.empty(); } if (!rasPassport.get().getIss().equals(this.rasPassportIssuer)) { logger.error("validateRASPassport() LOGIN FAILED ___ PASSPORT ISSUER IS NOT CORRECT ___ USER: {} ___ " + "EXPECTED ISSUER {} ___ ACTUAL ISSUER {} ___ CODE {}", user.getSubject(), this.rasPassportIssuer, rasPassport.get().getIss(), authRequest.get("code")); - return null; + return Optional.empty(); } + return rasPassport; + } - logger.info("RAS PASSPORT FOUND ___ USER: {} ___ PASSPORT: {} ___ CODE {}", user.getSubject(), rasPassport.get(), authRequest.get("code")); - - Set dbgapPermissions = this.rasPassPortService.ga4gpPassportToRasDbgapPermissions(rasPassport.get().getGa4ghPassportV1()); + protected User updateUserRoles(String code, User user, Passport rasPassport) { + logger.info("RAS PASSPORT FOUND ___ USER: {} ___ PASSPORT: {} ___ CODE {}", user.getSubject(), rasPassport, code); + Set dbgapPermissions = this.rasPassPortService.ga4gpPassportToRasDbgapPermissions(rasPassport.getGa4ghPassportV1()); Set dbgapRoleNames = this.roleService.getRoleNamesForDbgapPermissions(dbgapPermissions); user = userService.updateUserRoles(user, dbgapRoleNames); logger.debug("USER {} ROLES UPDATED {} ___ CODE {}", user.getSubject(), user.getRoles().stream().map(role -> role.getName().replace("MANAGED_", "")).toArray(), - authRequest.get("code")); - - String passport = introspectResponse.get("passport_jwt_v11").toString(); - user.setPassport(passport); - logger.info("RAS PASSPORT SUCCESSFULLY ADDED TO USER: {} ___ CODE {}", user.getSubject(), authRequest.get("code")); - userService.save(user); - HashMap responseMap = createUserClaims(user, idToken); - responseMap.put("oktaIdToken", idToken); - logger.info("LOGIN SUCCESS ___ USER {}:{} ___ WITH ROLES ___ {} ___ AUTHORIZATION WILL EXPIRE AT ___ {} ___ CODE {}", - user.getSubject(), user.getUuid().toString(), - user.getRoles().stream().map(role -> role.getName().replace("MANAGED_", "")).collect(Collectors.joining(",")), - responseMap.get("expirationDate"), authRequest.get("code")); - - return responseMap; + code); + return user; } private Optional initializeUser(JsonNode introspectResponse) { @@ -197,7 +202,13 @@ protected ObjectNode generateRasUserMetadata(User user) { return objectNode; } - + + private void setUserPassport(Map authRequest, JsonNode introspectResponse, User user) { + String passport = introspectResponse.get("passport_jwt_v11").toString(); + user.setPassport(passport); + userService.save(user); + logger.info("RAS PASSPORT SUCCESSFULLY ADDED TO USER: {} ___ CODE {}", user.getSubject(), authRequest.get("code")); + } @Override public String getProvider() { @@ -213,4 +224,5 @@ public void setRasConnection(Connection rasConnection) { this.rasConnection = rasConnection; } + } \ No newline at end of file From ff8c8448a86cec1625127e21acaf11debce3bd68 Mon Sep 17 00:00:00 2001 From: GeorgeC Date: Tue, 28 Jan 2025 14:45:02 -0500 Subject: [PATCH 2/3] Refactor method name and add unit test for role updates --- .../RASAuthenticationService.java | 4 +-- .../RASAuthenticationServiceTest.java | 31 ++++++++++++++++++- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationService.java b/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationService.java index bda9a6413..40b62ef31 100644 --- a/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationService.java +++ b/pic-sure-auth-services/src/main/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationService.java @@ -106,7 +106,7 @@ public HashMap authenticate(Map authRequest, Str User user = initializedUser.get(); Optional rasPassport = extractAndVerifyPassport(authRequest, introspectResponse, user); if (rasPassport.isEmpty()) return null; - user = updateUserRoles(authRequest.get("code"), user, rasPassport.get()); + user = updateRasUserRoles(authRequest.get("code"), user, rasPassport.get()); setUserPassport(authRequest, introspectResponse, user); HashMap responseMap = createUserClaims(user, idToken); @@ -140,7 +140,7 @@ private Optional extractAndVerifyPassport(Map authRequ return rasPassport; } - protected User updateUserRoles(String code, User user, Passport rasPassport) { + protected User updateRasUserRoles(String code, User user, Passport rasPassport) { logger.info("RAS PASSPORT FOUND ___ USER: {} ___ PASSPORT: {} ___ CODE {}", user.getSubject(), rasPassport, code); Set dbgapPermissions = this.rasPassPortService.ga4gpPassportToRasDbgapPermissions(rasPassport.getGa4ghPassportV1()); Set dbgapRoleNames = this.roleService.getRoleNamesForDbgapPermissions(dbgapPermissions); diff --git a/pic-sure-auth-services/src/test/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationServiceTest.java b/pic-sure-auth-services/src/test/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationServiceTest.java index 2c04b8750..90fef8b4d 100644 --- a/pic-sure-auth-services/src/test/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationServiceTest.java +++ b/pic-sure-auth-services/src/test/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationServiceTest.java @@ -1,9 +1,14 @@ package edu.harvard.hms.dbmi.avillach.auth.service.impl.authentication; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import edu.harvard.hms.dbmi.avillach.auth.entity.Connection; import edu.harvard.hms.dbmi.avillach.auth.entity.Privilege; import edu.harvard.hms.dbmi.avillach.auth.entity.Role; import edu.harvard.hms.dbmi.avillach.auth.entity.User; +import edu.harvard.hms.dbmi.avillach.auth.model.ras.Passport; +import edu.harvard.hms.dbmi.avillach.auth.model.ras.RasDbgapPermission; import edu.harvard.hms.dbmi.avillach.auth.repository.RoleRepository; import edu.harvard.hms.dbmi.avillach.auth.service.impl.*; import edu.harvard.hms.dbmi.avillach.auth.utils.FenceMappingUtility; @@ -19,6 +24,7 @@ import java.util.*; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.*; @SpringBootTest @@ -90,7 +96,6 @@ public void testAuthorizationCodeFlow_Successful() { // token exchange when(restClientUtil.retrievePostResponse(anyString(), any(), eq(queryString))).thenReturn(ResponseEntity.ok(data)); - // introspect when(restClientUtil.retrievePostResponse(anyString(), any(), eq(payload))).thenReturn(ResponseEntity.ok(introspectionResponse)); @@ -104,6 +109,30 @@ public void testAuthorizationCodeFlow_Successful() { assertNotNull(authenticate); } + @Test + public void testUpdateUserRoles_withEmptyDBGapPermissions() throws JsonProcessingException { + String introspectionResponse = + "{\"active\":true,\"sub\":\"example_email@test.com\",\"client_id\":\"test_client_id\",\"passport_jwt_v11\":\""+ exampleRasPassport +"\"}"; + JsonNode introspectionResponseParsed = new ObjectMapper().readTree(introspectionResponse); + String code = "AlphaNumericCode"; + User user = createTestUser(); + Optional passport = this.rasPassPortService.extractPassport(introspectionResponseParsed); + assertTrue(passport.isPresent()); + + Set dbgapPermissions = new HashSet<>(); + Set dbgapRoleNames = new HashSet<>(); + + when(rasPassPortService.ga4gpPassportToRasDbgapPermissions(any())).thenReturn(dbgapPermissions); + when(roleService.getRoleNamesForDbgapPermissions(any())).thenReturn(dbgapRoleNames); + when(userService.updateUserRoles(any(), any())).thenReturn(user); + + user = this.rasAuthenticationService.updateRasUserRoles(code, user, passport.get()); + assertNotNull(user); + + // We are verifying that we attempt to update a users roles even if no dbgap roles are present. + verify(userService, times(1)).updateUserRoles(user, dbgapRoleNames); + } + private User createTestUser() { User user = new User(); user.setUuid(UUID.randomUUID()); From b2677be5d4c6bbbc25b78927be04cbeefe5356c1 Mon Sep 17 00:00:00 2001 From: GeorgeC Date: Tue, 28 Jan 2025 15:06:57 -0500 Subject: [PATCH 3/3] Remove unused variable in RASAuthenticationServiceTest --- .../impl/authentication/RASAuthenticationServiceTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/pic-sure-auth-services/src/test/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationServiceTest.java b/pic-sure-auth-services/src/test/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationServiceTest.java index 90fef8b4d..b6b90423e 100644 --- a/pic-sure-auth-services/src/test/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationServiceTest.java +++ b/pic-sure-auth-services/src/test/java/edu/harvard/hms/dbmi/avillach/auth/service/impl/authentication/RASAuthenticationServiceTest.java @@ -114,7 +114,6 @@ public void testUpdateUserRoles_withEmptyDBGapPermissions() throws JsonProcessin String introspectionResponse = "{\"active\":true,\"sub\":\"example_email@test.com\",\"client_id\":\"test_client_id\",\"passport_jwt_v11\":\""+ exampleRasPassport +"\"}"; JsonNode introspectionResponseParsed = new ObjectMapper().readTree(introspectionResponse); - String code = "AlphaNumericCode"; User user = createTestUser(); Optional passport = this.rasPassPortService.extractPassport(introspectionResponseParsed); assertTrue(passport.isPresent());