From d31dff283fca62a85d43780c2edbc798f7cab746 Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 6 Oct 2026 18:43:21 +0000 Subject: [PATCH 1/3] fix(ENGKNOW-3998): stop CSA 404 lookups and warnings for users not in CSA Since ENGKNOW-3936 service-account tokens resolve a username (e.g. sequenceminer), so CsaApiUtils.updateWithCsaApi now looks them up in CSA. They have no CSA user record, so every new token caused 404s, each retried with a re-auth, and logged as WARN with a full stack trace. - HttpJsonServiceClient throws HttpStatusException (an IOException) carrying the HTTP status for error responses. - CsaApiService no longer retries 404s with new auth. - CsaApiUtils remembers users (and project/user pairs) CSA answered 404 for for 10 minutes, skips their user/role lookups meanwhile, and logs the miss once at INFO without a stack trace. Other CSA errors still log WARN. The resulting auth info is unchanged: empty user id and no CSA roles, as before the lookups started. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../gorpipe/gor/auth/utils/CsaApiUtils.java | 52 ++++++- .../gorpipe/security/cred/CsaApiService.java | 4 + .../security/cred/HttpJsonServiceClient.java | 7 +- .../security/cred/HttpStatusException.java | 26 ++++ .../gor/auth/utils/UTestCsaApiUtils.java | 140 ++++++++++++++++++ .../security/cred/UTestCsaApiService.java | 42 ++++++ 6 files changed, 266 insertions(+), 5 deletions(-) create mode 100644 auth/src/main/java/org/gorpipe/security/cred/HttpStatusException.java create mode 100644 auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java create mode 100644 auth/src/test/java/org/gorpipe/security/cred/UTestCsaApiService.java diff --git a/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java b/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java index 935fbf07b..f581b8ee8 100644 --- a/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java +++ b/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java @@ -1,13 +1,17 @@ package org.gorpipe.gor.auth.utils; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; import com.google.common.base.Strings; import org.gorpipe.gor.auth.GeneralAuthInfo; import org.gorpipe.gor.auth.GorAuthInfo; import org.gorpipe.security.cred.CsaApiService; +import org.gorpipe.security.cred.HttpStatusException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; +import java.time.Duration; import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; @@ -17,6 +21,17 @@ public class CsaApiUtils { private static final Logger log = LoggerFactory.getLogger(CsaApiUtils.class); + static final Duration NOT_IN_CSA_TTL = Duration.ofMinutes(10); + + /** + * Users (and project/user pairs) CSA answered 404 for, e.g. service accounts without a CSA user record. Skip + * looking them up again until the entry expires so each request doesn't hit CSA and log a warning. + */ + private static final Cache notInCsa = Caffeine.newBuilder() + .expireAfterWrite(NOT_IN_CSA_TTL) + .maximumSize(10_000) + .build(); + /** * Add ids from CSA API to the given gor auth, but only if missing and if found. * @@ -37,11 +52,12 @@ public static GorAuthInfo updateWithCsaApi(CsaApiService csaApiService, GorAuthI organizationId = updateOrganizationId(projectId, projectMap); } - if (Strings.isNullOrEmpty(userId) && !Strings.isNullOrEmpty(userName)) { + if (Strings.isNullOrEmpty(userId) && !Strings.isNullOrEmpty(userName) && !isNotInCsa(userName)) { Map userMap = getUserMapByEmail(csaApiService, userName); userId = updateUserId(userId, userMap); - if (userRoles.isEmpty() && !Strings.isNullOrEmpty(project) && !Strings.isNullOrEmpty(userName)) { + if (userRoles.isEmpty() && !Strings.isNullOrEmpty(project) && !isNotInCsa(userName) + && !isNotInCsa(projectUserKey(project, userName))) { List csaUserRoles = getUserRoleList(csaApiService, project, userName); updateUserRoles(userRoles, csaUserRoles); } @@ -51,6 +67,22 @@ public static GorAuthInfo updateWithCsaApi(CsaApiService csaApiService, GorAuthI organizationId, info.getExpiration()); } + static void clearUsersNotInCsa() { + notInCsa.invalidateAll(); + } + + private static boolean isNotInCsa(String key) { + return notInCsa.getIfPresent(key) != null; + } + + private static String projectUserKey(String project, String userName) { + return project + "/" + userName; + } + + private static boolean isNotFound(IOException e) { + return e instanceof HttpStatusException hse && hse.isNotFound(); + } + public static int getProjectId(Map projectMap) { if (projectMap != null && projectMap.containsKey("id")) { return (int) projectMap.get("id"); @@ -82,7 +114,13 @@ private static Map getUserMapByEmail(CsaApiService csaApiService try { userMap = csaApiService != null ? csaApiService.getUserByEmail(userEmail) : null; } catch (IOException e) { - log.warn("Unable to get user id from CSA API", e); + if (isNotFound(e)) { + notInCsa.put(userEmail, Boolean.TRUE); + log.info("User {} not found in CSA, skipping CSA user id/role lookups for it for {} minutes", + userEmail, NOT_IN_CSA_TTL.toMinutes()); + } else { + log.warn("Unable to get user id from CSA API", e); + } } return userMap; } @@ -92,7 +130,13 @@ private static List getUserRoleList(CsaApiService csaApiService, String project, try { userRoleList = csaApiService != null ? csaApiService.getUserRoleList(project, userEmail) : null; } catch (IOException e) { - log.warn("Unable to get user roles from CSA API", e); + if (isNotFound(e)) { + notInCsa.put(projectUserKey(project, userEmail), Boolean.TRUE); + log.info("User {} not found in CSA project {}, skipping CSA role lookups for it for {} minutes", + userEmail, project, NOT_IN_CSA_TTL.toMinutes()); + } else { + log.warn("Unable to get user roles from CSA API", e); + } } return userRoleList; } diff --git a/auth/src/main/java/org/gorpipe/security/cred/CsaApiService.java b/auth/src/main/java/org/gorpipe/security/cred/CsaApiService.java index b080363a9..aedda402e 100644 --- a/auth/src/main/java/org/gorpipe/security/cred/CsaApiService.java +++ b/auth/src/main/java/org/gorpipe/security/cred/CsaApiService.java @@ -53,6 +53,10 @@ private Map getApiResults(String path) throws IOException { try { result = jsonGet(path); } catch (IOException ioe) { + if (ioe instanceof HttpStatusException hse && hse.isNotFound()) { + // New auth will not make a missing resource appear. + throw hse; + } // Retry once with new Auth. result = initializeAndRetry(path); } diff --git a/auth/src/main/java/org/gorpipe/security/cred/HttpJsonServiceClient.java b/auth/src/main/java/org/gorpipe/security/cred/HttpJsonServiceClient.java index bafb0bd8c..0410eff2f 100644 --- a/auth/src/main/java/org/gorpipe/security/cred/HttpJsonServiceClient.java +++ b/auth/src/main/java/org/gorpipe/security/cred/HttpJsonServiceClient.java @@ -110,7 +110,12 @@ protected String readInput(HttpURLConnection conn) throws IOException { InputStream ie = conn.getErrorStream(); String headerinfo = conn.getHeaderFields().entrySet().stream().map(entry -> entry.getKey() + ": " + entry.getValue()).collect(Collectors.joining("\n")); String str = ie == null ? headerinfo : headerinfo + "\n" + new BufferedReader(new InputStreamReader(ie)).lines().collect(Collectors.joining()); - throw new IOException(conn.getResponseMessage() + ": " + str, e); + int status = conn.getResponseCode(); + String message = conn.getResponseMessage() + ": " + str; + if (status >= 400) { + throw new HttpStatusException(status, message, e); + } + throw new IOException(message, e); } } diff --git a/auth/src/main/java/org/gorpipe/security/cred/HttpStatusException.java b/auth/src/main/java/org/gorpipe/security/cred/HttpStatusException.java new file mode 100644 index 000000000..082fe339f --- /dev/null +++ b/auth/src/main/java/org/gorpipe/security/cred/HttpStatusException.java @@ -0,0 +1,26 @@ +package org.gorpipe.security.cred; + +import java.io.IOException; + +/** + * Thrown by {@link HttpJsonServiceClient} when the server responds with an error status. + */ +public class HttpStatusException extends IOException { + + public static final int NOT_FOUND = 404; + + private final int statusCode; + + public HttpStatusException(int statusCode, String message, Throwable cause) { + super(message, cause); + this.statusCode = statusCode; + } + + public int getStatusCode() { + return statusCode; + } + + public boolean isNotFound() { + return statusCode == NOT_FOUND; + } +} diff --git a/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java b/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java new file mode 100644 index 000000000..545e8ee99 --- /dev/null +++ b/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java @@ -0,0 +1,140 @@ +package org.gorpipe.gor.auth.utils; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import org.gorpipe.gor.auth.GeneralAuthInfo; +import org.gorpipe.gor.auth.GorAuthInfo; +import org.gorpipe.security.cred.CsaApiService; +import org.gorpipe.security.cred.HttpStatusException; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.*; + +/** + * CSA user lookups for users that don't exist in CSA (e.g. service accounts such as sequenceminer) must not flood the + * logs or hit CSA on every request. + */ +public class UTestCsaApiUtils { + + private static final String PROJECT = "test-proj"; + private static final String SERVICE_ACCOUNT = "sequenceminer"; + + private Logger logger; + private Level originalLevel; + private ListAppender appender; + private CsaApiService csaApiService; + + @Before + public void setUp() throws IOException { + CsaApiUtils.clearUsersNotInCsa(); + logger = (Logger) LoggerFactory.getLogger(CsaApiUtils.class); + originalLevel = logger.getLevel(); + logger.setLevel(Level.INFO); + appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + + csaApiService = mock(CsaApiService.class); + Map projectMap = new LinkedHashMap<>(); + projectMap.put("id", 5); + projectMap.put("organization_id", 7); + doReturn(projectMap).when(csaApiService).getProject(PROJECT); + } + + @After + public void tearDown() { + logger.detachAppender(appender); + logger.setLevel(originalLevel); + CsaApiUtils.clearUsersNotInCsa(); + } + + @Test + public void userNotInCsaKeepsEmptyUserIdAndRoles() throws IOException { + doThrow(notFound()).when(csaApiService).getUserByEmail(SERVICE_ACCOUNT); + doThrow(notFound()).when(csaApiService).getUserRoleList(anyString(), anyString()); + + GorAuthInfo info = CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); + + Assert.assertEquals(5, info.getProjectId()); + Assert.assertEquals(SERVICE_ACCOUNT, info.getUsername()); + Assert.assertEquals("", info.getUserId()); + Assert.assertTrue(info.getUserRoles().isEmpty()); + } + + @Test + public void userNotInCsaIsLookedUpOnlyOnce() throws IOException { + doThrow(notFound()).when(csaApiService).getUserByEmail(SERVICE_ACCOUNT); + doThrow(notFound()).when(csaApiService).getUserRoleList(anyString(), anyString()); + + for (int i = 0; i < 5; i++) { + CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); + } + + verify(csaApiService, times(1)).getUserByEmail(SERVICE_ACCOUNT); + verify(csaApiService, never()).getUserRoleList(anyString(), anyString()); + } + + @Test + public void userNotInCsaLogsOnceWithoutWarnOrStackTrace() throws IOException { + doThrow(notFound()).when(csaApiService).getUserByEmail(SERVICE_ACCOUNT); + doThrow(notFound()).when(csaApiService).getUserRoleList(anyString(), anyString()); + + for (int i = 0; i < 5; i++) { + CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); + } + + List warnings = appender.list.stream().filter(e -> e.getLevel().isGreaterOrEqual(Level.WARN)).toList(); + Assert.assertEquals("Unexpected warnings: " + warnings, 0, warnings.size()); + + List infos = appender.list.stream().filter(e -> e.getLevel() == Level.INFO).toList(); + Assert.assertEquals(1, infos.size()); + Assert.assertNull(infos.get(0).getThrowableProxy()); + Assert.assertTrue(infos.get(0).getFormattedMessage().contains(SERVICE_ACCOUNT)); + } + + @Test + public void otherCsaErrorsStillWarnAndAreNotCached() throws IOException { + doThrow(new HttpStatusException(500, "Internal Server Error", null)).when(csaApiService).getUserByEmail(SERVICE_ACCOUNT); + + CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); + CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); + + verify(csaApiService, times(2)).getUserByEmail(SERVICE_ACCOUNT); + Assert.assertTrue(appender.list.stream().anyMatch(e -> e.getLevel() == Level.WARN)); + } + + @Test + public void existingCsaUserGetsIdAndRoles() throws IOException { + String user = "user@email.com"; + doReturn(Collections.singletonMap("id", 10)).when(csaApiService).getUserByEmail(user); + Map role = new LinkedHashMap<>(); + role.put("role", "researcher"); + doReturn(List.of(role)).when(csaApiService).getUserRoleList(PROJECT, user); + + GorAuthInfo info = CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(user)); + + Assert.assertEquals("10", info.getUserId()); + Assert.assertEquals(List.of("researcher"), info.getUserRoles()); + } + + private static GorAuthInfo authInfo(String username) { + return new GeneralAuthInfo(0, PROJECT, username, "", null, 0, Long.MAX_VALUE); + } + + private static HttpStatusException notFound() { + return new HttpStatusException(404, "Not Found: {\"error\":{\"full_message\":\"Couldn't find user\"}}", null); + } +} diff --git a/auth/src/test/java/org/gorpipe/security/cred/UTestCsaApiService.java b/auth/src/test/java/org/gorpipe/security/cred/UTestCsaApiService.java new file mode 100644 index 000000000..4d2b36d4c --- /dev/null +++ b/auth/src/test/java/org/gorpipe/security/cred/UTestCsaApiService.java @@ -0,0 +1,42 @@ +package org.gorpipe.security.cred; + +import org.gorpipe.gor.auth.AuthConfig; +import org.junit.Assert; +import org.junit.Test; + +import java.io.IOException; +import java.util.Collections; +import java.util.Map; + +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.*; + +public class UTestCsaApiService { + + private CsaApiService service() { + return spy(new CsaApiService(mock(CsaAuthConfiguration.class), mock(AuthConfig.class))); + } + + @Test + public void notFoundIsNotRetried() throws IOException { + CsaApiService service = service(); + doThrow(new HttpStatusException(404, "Not Found", null)).when(service).jsonGet(anyString()); + + HttpStatusException e = Assert.assertThrows(HttpStatusException.class, () -> service.getUserByEmail("sequenceminer")); + + Assert.assertEquals(404, e.getStatusCode()); + verify(service, times(1)).jsonGet(anyString()); + verify(service, never()).initializeAndRetry(anyString()); + } + + @Test + public void otherErrorsAreRetriedWithNewAuth() throws IOException { + CsaApiService service = service(); + doThrow(new HttpStatusException(401, "Unauthorized", null)).when(service).jsonGet(anyString()); + Map user = Collections.singletonMap("id", 10); + doReturn(Collections.singletonMap("user", user)).when(service).initializeAndRetry(anyString()); + + Assert.assertEquals(user, service.getUserByEmail("user@email.com")); + verify(service, times(1)).initializeAndRetry(anyString()); + } +} From d3136baae7bb73109e16a461d5b04e4958dc3bb4 Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 6 Oct 2026 22:02:24 +0000 Subject: [PATCH 2/3] refactor(ENGKNOW-3998): only look up email usernames in CSA instead of caching 404s CSA looks users up by email, so skip the CSA user id/role lookup when the username is not an email (e.g. service accounts such as sequenceminer). Replaces the HttpStatusException, no-retry-on-404 and negative cache from the previous commit with a single check, Strings.isEmail in the util module. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../gorpipe/gor/auth/utils/CsaApiUtils.java | 55 ++---------- .../gorpipe/security/cred/CsaApiService.java | 4 - .../security/cred/HttpJsonServiceClient.java | 7 +- .../security/cred/HttpStatusException.java | 26 ------ .../gor/auth/UTestPlatformAuthUsername.java | 10 ++- .../gor/auth/utils/UTestCsaApiUtils.java | 90 +++---------------- .../security/cred/UTestCsaApiService.java | 42 --------- .../main/java/org/gorpipe/util/Strings.java | 16 +++- .../java/org/gorpipe/util/UTestStrings.java | 13 ++- 9 files changed, 50 insertions(+), 213 deletions(-) delete mode 100644 auth/src/main/java/org/gorpipe/security/cred/HttpStatusException.java delete mode 100644 auth/src/test/java/org/gorpipe/security/cred/UTestCsaApiService.java diff --git a/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java b/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java index f581b8ee8..36d509015 100644 --- a/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java +++ b/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java @@ -1,17 +1,13 @@ package org.gorpipe.gor.auth.utils; -import com.github.benmanes.caffeine.cache.Cache; -import com.github.benmanes.caffeine.cache.Caffeine; -import com.google.common.base.Strings; +import org.gorpipe.util.Strings; import org.gorpipe.gor.auth.GeneralAuthInfo; import org.gorpipe.gor.auth.GorAuthInfo; import org.gorpipe.security.cred.CsaApiService; -import org.gorpipe.security.cred.HttpStatusException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; -import java.time.Duration; import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; @@ -21,17 +17,6 @@ public class CsaApiUtils { private static final Logger log = LoggerFactory.getLogger(CsaApiUtils.class); - static final Duration NOT_IN_CSA_TTL = Duration.ofMinutes(10); - - /** - * Users (and project/user pairs) CSA answered 404 for, e.g. service accounts without a CSA user record. Skip - * looking them up again until the entry expires so each request doesn't hit CSA and log a warning. - */ - private static final Cache notInCsa = Caffeine.newBuilder() - .expireAfterWrite(NOT_IN_CSA_TTL) - .maximumSize(10_000) - .build(); - /** * Add ids from CSA API to the given gor auth, but only if missing and if found. * @@ -52,12 +37,12 @@ public static GorAuthInfo updateWithCsaApi(CsaApiService csaApiService, GorAuthI organizationId = updateOrganizationId(projectId, projectMap); } - if (Strings.isNullOrEmpty(userId) && !Strings.isNullOrEmpty(userName) && !isNotInCsa(userName)) { + // CSA looks users up by email, so skip non-email usernames (e.g. service accounts) that it can't know. + if (Strings.isNullOrEmpty(userId) && Strings.isEmail(userName)) { Map userMap = getUserMapByEmail(csaApiService, userName); userId = updateUserId(userId, userMap); - if (userRoles.isEmpty() && !Strings.isNullOrEmpty(project) && !isNotInCsa(userName) - && !isNotInCsa(projectUserKey(project, userName))) { + if (userRoles.isEmpty() && !Strings.isNullOrEmpty(project) && !Strings.isNullOrEmpty(userName)) { List csaUserRoles = getUserRoleList(csaApiService, project, userName); updateUserRoles(userRoles, csaUserRoles); } @@ -67,22 +52,6 @@ public static GorAuthInfo updateWithCsaApi(CsaApiService csaApiService, GorAuthI organizationId, info.getExpiration()); } - static void clearUsersNotInCsa() { - notInCsa.invalidateAll(); - } - - private static boolean isNotInCsa(String key) { - return notInCsa.getIfPresent(key) != null; - } - - private static String projectUserKey(String project, String userName) { - return project + "/" + userName; - } - - private static boolean isNotFound(IOException e) { - return e instanceof HttpStatusException hse && hse.isNotFound(); - } - public static int getProjectId(Map projectMap) { if (projectMap != null && projectMap.containsKey("id")) { return (int) projectMap.get("id"); @@ -114,13 +83,7 @@ private static Map getUserMapByEmail(CsaApiService csaApiService try { userMap = csaApiService != null ? csaApiService.getUserByEmail(userEmail) : null; } catch (IOException e) { - if (isNotFound(e)) { - notInCsa.put(userEmail, Boolean.TRUE); - log.info("User {} not found in CSA, skipping CSA user id/role lookups for it for {} minutes", - userEmail, NOT_IN_CSA_TTL.toMinutes()); - } else { - log.warn("Unable to get user id from CSA API", e); - } + log.warn("Unable to get user id from CSA API", e); } return userMap; } @@ -130,13 +93,7 @@ private static List getUserRoleList(CsaApiService csaApiService, String project, try { userRoleList = csaApiService != null ? csaApiService.getUserRoleList(project, userEmail) : null; } catch (IOException e) { - if (isNotFound(e)) { - notInCsa.put(projectUserKey(project, userEmail), Boolean.TRUE); - log.info("User {} not found in CSA project {}, skipping CSA role lookups for it for {} minutes", - userEmail, project, NOT_IN_CSA_TTL.toMinutes()); - } else { - log.warn("Unable to get user roles from CSA API", e); - } + log.warn("Unable to get user roles from CSA API", e); } return userRoleList; } diff --git a/auth/src/main/java/org/gorpipe/security/cred/CsaApiService.java b/auth/src/main/java/org/gorpipe/security/cred/CsaApiService.java index aedda402e..b080363a9 100644 --- a/auth/src/main/java/org/gorpipe/security/cred/CsaApiService.java +++ b/auth/src/main/java/org/gorpipe/security/cred/CsaApiService.java @@ -53,10 +53,6 @@ private Map getApiResults(String path) throws IOException { try { result = jsonGet(path); } catch (IOException ioe) { - if (ioe instanceof HttpStatusException hse && hse.isNotFound()) { - // New auth will not make a missing resource appear. - throw hse; - } // Retry once with new Auth. result = initializeAndRetry(path); } diff --git a/auth/src/main/java/org/gorpipe/security/cred/HttpJsonServiceClient.java b/auth/src/main/java/org/gorpipe/security/cred/HttpJsonServiceClient.java index 0410eff2f..bafb0bd8c 100644 --- a/auth/src/main/java/org/gorpipe/security/cred/HttpJsonServiceClient.java +++ b/auth/src/main/java/org/gorpipe/security/cred/HttpJsonServiceClient.java @@ -110,12 +110,7 @@ protected String readInput(HttpURLConnection conn) throws IOException { InputStream ie = conn.getErrorStream(); String headerinfo = conn.getHeaderFields().entrySet().stream().map(entry -> entry.getKey() + ": " + entry.getValue()).collect(Collectors.joining("\n")); String str = ie == null ? headerinfo : headerinfo + "\n" + new BufferedReader(new InputStreamReader(ie)).lines().collect(Collectors.joining()); - int status = conn.getResponseCode(); - String message = conn.getResponseMessage() + ": " + str; - if (status >= 400) { - throw new HttpStatusException(status, message, e); - } - throw new IOException(message, e); + throw new IOException(conn.getResponseMessage() + ": " + str, e); } } diff --git a/auth/src/main/java/org/gorpipe/security/cred/HttpStatusException.java b/auth/src/main/java/org/gorpipe/security/cred/HttpStatusException.java deleted file mode 100644 index 082fe339f..000000000 --- a/auth/src/main/java/org/gorpipe/security/cred/HttpStatusException.java +++ /dev/null @@ -1,26 +0,0 @@ -package org.gorpipe.security.cred; - -import java.io.IOException; - -/** - * Thrown by {@link HttpJsonServiceClient} when the server responds with an error status. - */ -public class HttpStatusException extends IOException { - - public static final int NOT_FOUND = 404; - - private final int statusCode; - - public HttpStatusException(int statusCode, String message, Throwable cause) { - super(message, cause); - this.statusCode = statusCode; - } - - public int getStatusCode() { - return statusCode; - } - - public boolean isNotFound() { - return statusCode == NOT_FOUND; - } -} diff --git a/auth/src/test/java/org/gorpipe/gor/auth/UTestPlatformAuthUsername.java b/auth/src/test/java/org/gorpipe/gor/auth/UTestPlatformAuthUsername.java index 1f9c6c0b9..882754ea0 100644 --- a/auth/src/test/java/org/gorpipe/gor/auth/UTestPlatformAuthUsername.java +++ b/auth/src/test/java/org/gorpipe/gor/auth/UTestPlatformAuthUsername.java @@ -29,6 +29,9 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; /** * Username resolution for platform JWTs. Service-account tokens (Keycloak client_credentials) carry no email claim, @@ -139,8 +142,6 @@ public void jwtAuthHandlesMissingRealmAccess() throws Exception { @Test public void jwtAuthCachesServiceAccountsSeparately() throws Exception { CsaApiService csaApiService = mock(CsaApiService.class); - doReturn(Collections.singletonMap("id", 11)).when(csaApiService).getUserByEmail("service-account-a"); - doReturn(Collections.singletonMap("id", 22)).when(csaApiService).getUserByEmail("service-account-b"); doReturn(null).when(csaApiService).getProject(anyString()); doReturn("CSA").when(config).updateAuthInfoPolicy(); @@ -149,9 +150,10 @@ public void jwtAuthCachesServiceAccountsSeparately() throws Exception { GorAuthInfo b = auth.getGorAuthInfo(PROJECT, parse(token().withClaim("preferred_username", "service-account-b"))); Assert.assertEquals("service-account-a", a.getUsername()); - Assert.assertEquals("11", a.getUserId()); Assert.assertEquals("service-account-b", b.getUsername()); - Assert.assertEquals("22", b.getUserId()); + // One cache miss (and CSA project lookup) per service account; no CSA user lookup for non-email usernames. + verify(csaApiService, times(2)).getProject(PROJECT); + verify(csaApiService, never()).getUserByEmail(anyString()); } // --- PlatformAuth --- diff --git a/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java b/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java index 545e8ee99..ef92037a5 100644 --- a/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java +++ b/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java @@ -1,18 +1,11 @@ package org.gorpipe.gor.auth.utils; -import ch.qos.logback.classic.Level; -import ch.qos.logback.classic.Logger; -import ch.qos.logback.classic.spi.ILoggingEvent; -import ch.qos.logback.core.read.ListAppender; import org.gorpipe.gor.auth.GeneralAuthInfo; import org.gorpipe.gor.auth.GorAuthInfo; import org.gorpipe.security.cred.CsaApiService; -import org.gorpipe.security.cred.HttpStatusException; -import org.junit.After; import org.junit.Assert; import org.junit.Before; import org.junit.Test; -import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.Collections; @@ -24,29 +17,17 @@ import static org.mockito.Mockito.*; /** - * CSA user lookups for users that don't exist in CSA (e.g. service accounts such as sequenceminer) must not flood the - * logs or hit CSA on every request. + * CSA looks users up by email. Usernames that aren't emails (e.g. service accounts such as sequenceminer) must not be + * looked up, as CSA answers 404 for them and every request logged a warning. */ public class UTestCsaApiUtils { private static final String PROJECT = "test-proj"; - private static final String SERVICE_ACCOUNT = "sequenceminer"; - private Logger logger; - private Level originalLevel; - private ListAppender appender; private CsaApiService csaApiService; @Before public void setUp() throws IOException { - CsaApiUtils.clearUsersNotInCsa(); - logger = (Logger) LoggerFactory.getLogger(CsaApiUtils.class); - originalLevel = logger.getLevel(); - logger.setLevel(Level.INFO); - appender = new ListAppender<>(); - appender.start(); - logger.addAppender(appender); - csaApiService = mock(CsaApiService.class); Map projectMap = new LinkedHashMap<>(); projectMap.put("id", 5); @@ -54,70 +35,23 @@ public void setUp() throws IOException { doReturn(projectMap).when(csaApiService).getProject(PROJECT); } - @After - public void tearDown() { - logger.detachAppender(appender); - logger.setLevel(originalLevel); - CsaApiUtils.clearUsersNotInCsa(); - } - @Test - public void userNotInCsaKeepsEmptyUserIdAndRoles() throws IOException { - doThrow(notFound()).when(csaApiService).getUserByEmail(SERVICE_ACCOUNT); - doThrow(notFound()).when(csaApiService).getUserRoleList(anyString(), anyString()); + public void nonEmailUserIsNotLookedUpInCsa() throws IOException { + doThrow(new IOException("Not Found")).when(csaApiService).getUserByEmail(anyString()); + doThrow(new IOException("Not Found")).when(csaApiService).getUserRoleList(anyString(), anyString()); - GorAuthInfo info = CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); + GorAuthInfo info = CsaApiUtils.updateWithCsaApi(csaApiService, authInfo("sequenceminer")); + verify(csaApiService, never()).getUserByEmail(anyString()); + verify(csaApiService, never()).getUserRoleList(anyString(), anyString()); Assert.assertEquals(5, info.getProjectId()); - Assert.assertEquals(SERVICE_ACCOUNT, info.getUsername()); + Assert.assertEquals("sequenceminer", info.getUsername()); Assert.assertEquals("", info.getUserId()); Assert.assertTrue(info.getUserRoles().isEmpty()); } @Test - public void userNotInCsaIsLookedUpOnlyOnce() throws IOException { - doThrow(notFound()).when(csaApiService).getUserByEmail(SERVICE_ACCOUNT); - doThrow(notFound()).when(csaApiService).getUserRoleList(anyString(), anyString()); - - for (int i = 0; i < 5; i++) { - CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); - } - - verify(csaApiService, times(1)).getUserByEmail(SERVICE_ACCOUNT); - verify(csaApiService, never()).getUserRoleList(anyString(), anyString()); - } - - @Test - public void userNotInCsaLogsOnceWithoutWarnOrStackTrace() throws IOException { - doThrow(notFound()).when(csaApiService).getUserByEmail(SERVICE_ACCOUNT); - doThrow(notFound()).when(csaApiService).getUserRoleList(anyString(), anyString()); - - for (int i = 0; i < 5; i++) { - CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); - } - - List warnings = appender.list.stream().filter(e -> e.getLevel().isGreaterOrEqual(Level.WARN)).toList(); - Assert.assertEquals("Unexpected warnings: " + warnings, 0, warnings.size()); - - List infos = appender.list.stream().filter(e -> e.getLevel() == Level.INFO).toList(); - Assert.assertEquals(1, infos.size()); - Assert.assertNull(infos.get(0).getThrowableProxy()); - Assert.assertTrue(infos.get(0).getFormattedMessage().contains(SERVICE_ACCOUNT)); - } - - @Test - public void otherCsaErrorsStillWarnAndAreNotCached() throws IOException { - doThrow(new HttpStatusException(500, "Internal Server Error", null)).when(csaApiService).getUserByEmail(SERVICE_ACCOUNT); - - CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); - CsaApiUtils.updateWithCsaApi(csaApiService, authInfo(SERVICE_ACCOUNT)); - - verify(csaApiService, times(2)).getUserByEmail(SERVICE_ACCOUNT); - Assert.assertTrue(appender.list.stream().anyMatch(e -> e.getLevel() == Level.WARN)); - } - - @Test - public void existingCsaUserGetsIdAndRoles() throws IOException { + public void emailUserGetsIdAndRolesFromCsa() throws IOException { String user = "user@email.com"; doReturn(Collections.singletonMap("id", 10)).when(csaApiService).getUserByEmail(user); Map role = new LinkedHashMap<>(); @@ -133,8 +67,4 @@ public void existingCsaUserGetsIdAndRoles() throws IOException { private static GorAuthInfo authInfo(String username) { return new GeneralAuthInfo(0, PROJECT, username, "", null, 0, Long.MAX_VALUE); } - - private static HttpStatusException notFound() { - return new HttpStatusException(404, "Not Found: {\"error\":{\"full_message\":\"Couldn't find user\"}}", null); - } } diff --git a/auth/src/test/java/org/gorpipe/security/cred/UTestCsaApiService.java b/auth/src/test/java/org/gorpipe/security/cred/UTestCsaApiService.java deleted file mode 100644 index 4d2b36d4c..000000000 --- a/auth/src/test/java/org/gorpipe/security/cred/UTestCsaApiService.java +++ /dev/null @@ -1,42 +0,0 @@ -package org.gorpipe.security.cred; - -import org.gorpipe.gor.auth.AuthConfig; -import org.junit.Assert; -import org.junit.Test; - -import java.io.IOException; -import java.util.Collections; -import java.util.Map; - -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.*; - -public class UTestCsaApiService { - - private CsaApiService service() { - return spy(new CsaApiService(mock(CsaAuthConfiguration.class), mock(AuthConfig.class))); - } - - @Test - public void notFoundIsNotRetried() throws IOException { - CsaApiService service = service(); - doThrow(new HttpStatusException(404, "Not Found", null)).when(service).jsonGet(anyString()); - - HttpStatusException e = Assert.assertThrows(HttpStatusException.class, () -> service.getUserByEmail("sequenceminer")); - - Assert.assertEquals(404, e.getStatusCode()); - verify(service, times(1)).jsonGet(anyString()); - verify(service, never()).initializeAndRetry(anyString()); - } - - @Test - public void otherErrorsAreRetriedWithNewAuth() throws IOException { - CsaApiService service = service(); - doThrow(new HttpStatusException(401, "Unauthorized", null)).when(service).jsonGet(anyString()); - Map user = Collections.singletonMap("id", 10); - doReturn(Collections.singletonMap("user", user)).when(service).initializeAndRetry(anyString()); - - Assert.assertEquals(user, service.getUserByEmail("user@email.com")); - verify(service, times(1)).initializeAndRetry(anyString()); - } -} diff --git a/util/src/main/java/org/gorpipe/util/Strings.java b/util/src/main/java/org/gorpipe/util/Strings.java index c6a105ca8..91eb53979 100644 --- a/util/src/main/java/org/gorpipe/util/Strings.java +++ b/util/src/main/java/org/gorpipe/util/Strings.java @@ -29,4 +29,18 @@ public static String blankNull(String s) { return s; } -} \ No newline at end of file + /** + * Loose check, only looks for an '@' between a non-empty local part and domain. + * + * @param s string to check + * @return returns true if the String looks like an email address, otherwise returns false. + */ + public static boolean isEmail(String s) { + if (s == null) { + return false; + } + int at = s.indexOf('@'); + return at > 0 && at < s.length() - 1; + } + +} diff --git a/util/src/test/java/org/gorpipe/util/UTestStrings.java b/util/src/test/java/org/gorpipe/util/UTestStrings.java index aae4ef22f..3e08ef077 100644 --- a/util/src/test/java/org/gorpipe/util/UTestStrings.java +++ b/util/src/test/java/org/gorpipe/util/UTestStrings.java @@ -33,4 +33,15 @@ public void testBlankNull() { Assert.assertEquals("abc", Strings.blankNull("abc")); Assert.assertNotSame("abc", Strings.blankNull("abc ")); } -} \ No newline at end of file + + @Test + public void testIsEmail() { + Assert.assertTrue(Strings.isEmail("user@email.com")); + Assert.assertFalse(Strings.isEmail(null)); + Assert.assertFalse(Strings.isEmail("")); + Assert.assertFalse(Strings.isEmail("sequenceminer")); + Assert.assertFalse(Strings.isEmail("service-account-sequenceminer")); + Assert.assertFalse(Strings.isEmail("@email.com")); + Assert.assertFalse(Strings.isEmail("user@")); + } +} From 88bedd9eb36e758f9bd0cacc7a0c1bf666263bf9 Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 6 Oct 2026 22:43:46 +0000 Subject: [PATCH 3/3] fix(ENGKNOW-3998): tighten isEmail and clean up review findings - Strings.isEmail requires a single '@' and rejects whitespace and '/', as the username is put unencoded into CSA URL paths. - Drop the redundant username check in the CSA role lookup. - Remove unused stubs from UTestCsaApiUtils. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../org/gorpipe/gor/auth/utils/CsaApiUtils.java | 2 +- .../gorpipe/gor/auth/utils/UTestCsaApiUtils.java | 3 --- util/src/main/java/org/gorpipe/util/Strings.java | 13 +++++++------ .../test/java/org/gorpipe/util/UTestStrings.java | 5 +++++ 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java b/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java index 36d509015..0f528615a 100644 --- a/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java +++ b/auth/src/main/java/org/gorpipe/gor/auth/utils/CsaApiUtils.java @@ -42,7 +42,7 @@ public static GorAuthInfo updateWithCsaApi(CsaApiService csaApiService, GorAuthI Map userMap = getUserMapByEmail(csaApiService, userName); userId = updateUserId(userId, userMap); - if (userRoles.isEmpty() && !Strings.isNullOrEmpty(project) && !Strings.isNullOrEmpty(userName)) { + if (userRoles.isEmpty() && !Strings.isNullOrEmpty(project)) { List csaUserRoles = getUserRoleList(csaApiService, project, userName); updateUserRoles(userRoles, csaUserRoles); } diff --git a/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java b/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java index ef92037a5..7e75703af 100644 --- a/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java +++ b/auth/src/test/java/org/gorpipe/gor/auth/utils/UTestCsaApiUtils.java @@ -37,9 +37,6 @@ public void setUp() throws IOException { @Test public void nonEmailUserIsNotLookedUpInCsa() throws IOException { - doThrow(new IOException("Not Found")).when(csaApiService).getUserByEmail(anyString()); - doThrow(new IOException("Not Found")).when(csaApiService).getUserRoleList(anyString(), anyString()); - GorAuthInfo info = CsaApiUtils.updateWithCsaApi(csaApiService, authInfo("sequenceminer")); verify(csaApiService, never()).getUserByEmail(anyString()); diff --git a/util/src/main/java/org/gorpipe/util/Strings.java b/util/src/main/java/org/gorpipe/util/Strings.java index 91eb53979..a3cd0a39f 100644 --- a/util/src/main/java/org/gorpipe/util/Strings.java +++ b/util/src/main/java/org/gorpipe/util/Strings.java @@ -1,7 +1,12 @@ package org.gorpipe.util; +import java.util.regex.Pattern; + public class Strings { + // One '@' with a non-empty local part and domain, no whitespace or '/' (the value may end up in a URL path). + private static final Pattern EMAIL = Pattern.compile("[^\\s@/]+@[^\\s@/]+"); + /** * @param s string to check * @return returns true if the String is null or blank after trimming, otherwise returns false. @@ -30,17 +35,13 @@ public static String blankNull(String s) { } /** - * Loose check, only looks for an '@' between a non-empty local part and domain. + * Loose check: a single '@' between a non-empty local part and domain, without whitespace or '/'. * * @param s string to check * @return returns true if the String looks like an email address, otherwise returns false. */ public static boolean isEmail(String s) { - if (s == null) { - return false; - } - int at = s.indexOf('@'); - return at > 0 && at < s.length() - 1; + return s != null && EMAIL.matcher(s).matches(); } } diff --git a/util/src/test/java/org/gorpipe/util/UTestStrings.java b/util/src/test/java/org/gorpipe/util/UTestStrings.java index 3e08ef077..050808418 100644 --- a/util/src/test/java/org/gorpipe/util/UTestStrings.java +++ b/util/src/test/java/org/gorpipe/util/UTestStrings.java @@ -43,5 +43,10 @@ public void testIsEmail() { Assert.assertFalse(Strings.isEmail("service-account-sequenceminer")); Assert.assertFalse(Strings.isEmail("@email.com")); Assert.assertFalse(Strings.isEmail("user@")); + Assert.assertFalse(Strings.isEmail(" user@email.com ")); + Assert.assertFalse(Strings.isEmail("user name@email.com")); + Assert.assertFalse(Strings.isEmail("a@b@c")); + Assert.assertFalse(Strings.isEmail("../../x@y")); + Assert.assertFalse(Strings.isEmail("user@email.com/x")); } }