From e714908d3e396926db5e6179db034977799ac947 Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Wed, 13 May 2026 00:20:33 -0400 Subject: [PATCH] fix: sync IdP-resolved roles to DB on every OIDC/LDAP login provisionIdpUser() was calling upsertUser(username) which hardcoded 'USER' regardless of the resolved OIDC group authorities or LDAP group mappings. Users with ROLE_ADMIN or ROLE_SELF_CERTIFY had the correct session authorities but their proxy_users.roles column stayed 'USER' forever. Add upsertUser(String, List) to UserStore, JdbcUserStore, MongoUserStore, and CompositeUserStore. The new overload inserts with the resolved roles on first login and updates roles on subsequent logins so IdP group changes take effect on the next sign-in without manual DB fixups. SecurityConfig.provisionIdpUser() now extracts role names from auth.getAuthorities() (stripping the ROLE_ prefix) and calls the new overload for both OIDC and LDAP auth paths. closes #249 Co-Authored-By: Claude Sonnet 4.6 --- .../gitproxy/user/CompositeUserStore.java | 5 +++ .../finos/gitproxy/user/JdbcUserStore.java | 17 ++++++++-- .../finos/gitproxy/user/MongoUserStore.java | 13 ++++++-- .../org/finos/gitproxy/user/UserStore.java | 8 +++++ .../user/JdbcUserStoreIntegrationTest.java | 32 +++++++++++++++++++ .../frontend/src/pages/PushDetail.tsx | 1 - .../gitproxy/dashboard/SecurityConfig.java | 8 ++++- 7 files changed, 77 insertions(+), 7 deletions(-) diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/CompositeUserStore.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/CompositeUserStore.java index 469755f5..a3cdbdfd 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/CompositeUserStore.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/CompositeUserStore.java @@ -184,6 +184,11 @@ public void upsertUser(String username) { mutableStore.upsertUser(username); } + @Override + public void upsertUser(String username, List roles) { + mutableStore.upsertUser(username, roles); + } + @Override public void upsertLockedEmail(String username, String email, String authSource) { mutableStore.upsertLockedEmail(username, email, authSource); diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/JdbcUserStore.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/JdbcUserStore.java index 0858ec9b..d4176f77 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/JdbcUserStore.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/JdbcUserStore.java @@ -208,14 +208,25 @@ public void removeScmIdentity(String username, String provider, String scmUserna * The password is left NULL so the account cannot be used for form login. */ public void upsertUser(String username) { + upsertUser(username, List.of("USER")); + } + + @Override + public void upsertUser(String username, List roles) { + String rolesStr = roles.isEmpty() ? "USER" : String.join(",", roles); boolean exists = !jdbc.queryForList( "SELECT username FROM proxy_users WHERE username = :u", Map.of("u", username), String.class) .isEmpty(); if (!exists) { jdbc.update( - "INSERT INTO proxy_users (username, password_hash, roles) VALUES (:u, NULL, 'USER')", - Map.of("u", username)); - log.debug("Auto-provisioned IdP user '{}'", username); + "INSERT INTO proxy_users (username, password_hash, roles) VALUES (:u, NULL, :roles)", + Map.of("u", username, "roles", rolesStr)); + log.debug("Auto-provisioned IdP user '{}' with roles {}", username, rolesStr); + } else { + jdbc.update( + "UPDATE proxy_users SET roles = :roles WHERE username = :u", + Map.of("u", username, "roles", rolesStr)); + log.debug("Synced IdP roles for user '{}': {}", username, rolesStr); } } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/MongoUserStore.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/MongoUserStore.java index c275e8ea..6b379a80 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/MongoUserStore.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/MongoUserStore.java @@ -157,14 +157,23 @@ public void setPassword(String username, String passwordHash) { @Override public void upsertUser(String username) { + upsertUser(username, List.of("USER")); + } + + @Override + public void upsertUser(String username, List roles) { + String rolesStr = roles.isEmpty() ? "USER" : String.join(",", roles); if (getCollection().find(Filters.eq("_id", username)).first() == null) { getCollection() .insertOne(new Document("_id", username) .append("passwordHash", null) - .append("roles", "USER") + .append("roles", rolesStr) .append("emails", List.of()) .append("scmIdentities", List.of())); - log.debug("Auto-provisioned IdP user '{}'", username); + log.debug("Auto-provisioned IdP user '{}' with roles {}", username, rolesStr); + } else { + getCollection().updateOne(Filters.eq("_id", username), Updates.set("roles", rolesStr)); + log.debug("Synced IdP roles for user '{}': {}", username, rolesStr); } } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/UserStore.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/UserStore.java index 6ea10946..abee8b51 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/UserStore.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/user/UserStore.java @@ -62,6 +62,14 @@ public interface UserStore extends ReadOnlyUserStore { */ void upsertUser(String username); + /** + * Ensures a user row exists and syncs the given roles on every IdP login. Roles are authoritative from the IdP — + * any existing roles are overwritten so that IdP group changes take effect on next sign-in. + */ + default void upsertUser(String username, List roles) { + upsertUser(username); + } + /** Inserts or updates an email for a user as locked (owned by the identity provider). */ void upsertLockedEmail(String username, String email, String authSource); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/user/JdbcUserStoreIntegrationTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/user/JdbcUserStoreIntegrationTest.java index ecaea950..18241ef0 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/user/JdbcUserStoreIntegrationTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/user/JdbcUserStoreIntegrationTest.java @@ -247,6 +247,38 @@ void upsertUser_preservesExistingUser() { assertEquals("{noop}pw", result.get().getPasswordHash()); } + @Test + void upsertUser_withRoles_setsRolesOnNewUser() { + store.upsertUser("oidcuser", List.of("USER", "ADMIN")); + + var result = store.findByUsername("oidcuser"); + assertTrue(result.isPresent()); + assertNull(result.get().getPasswordHash()); + assertTrue(result.get().getRoles().contains("ADMIN")); + assertTrue(result.get().getRoles().contains("USER")); + } + + @Test + void upsertUser_withRoles_syncsRolesOnSubsequentLogin() { + store.upsertUser("oidcuser", List.of("USER")); + store.upsertUser("oidcuser", List.of("USER", "SELF_CERTIFY")); + + var result = store.findByUsername("oidcuser"); + assertTrue(result.isPresent()); + assertTrue(result.get().getRoles().contains("SELF_CERTIFY")); + } + + @Test + void upsertUser_withRoles_syncsRolesForYamlUser() { + store.upsertAll(List.of(user("alice", List.of(), List.of()))); + store.upsertUser("alice", List.of("USER", "ADMIN")); + + var result = store.findByUsername("alice"); + assertTrue(result.isPresent()); + assertEquals("{noop}pw", result.get().getPasswordHash()); + assertTrue(result.get().getRoles().contains("ADMIN")); + } + // ---- upsertLockedEmail ---- @Test diff --git a/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx b/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx index 294323eb..f17822f2 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx @@ -100,7 +100,6 @@ function formatTime(ts: string | number | undefined) { } } - /** * Build a direct link to the commit on the upstream SCM. * GitHub / Gitea / Codeberg: {repo}/commit/{sha} diff --git a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/SecurityConfig.java b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/SecurityConfig.java index 277974e8..2c31d614 100644 --- a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/SecurityConfig.java +++ b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/SecurityConfig.java @@ -474,7 +474,13 @@ private void provisionIdpUser(Authentication auth) { if (authSource == null) return; - jdbc.upsertUser(username); + List roles = auth.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .filter(a -> a.startsWith("ROLE_")) + .map(a -> a.substring(5)) + .toList(); + if (roles.isEmpty()) roles = List.of("USER"); + jdbc.upsertUser(username, roles); if (email != null && !email.isBlank()) { try { jdbc.upsertLockedEmail(username, email.strip().toLowerCase(), authSource);