Skip to content

Commit fa4af73

Browse files
authored
fix: sync IdP-resolved roles to DB on every OIDC/LDAP login (#250)
## Summary - `provisionIdpUser()` was calling `upsertUser(username)` which hardcoded `USER` regardless of the resolved OIDC group authorities or LDAP group mappings — users with `ROLE_ADMIN` / `ROLE_SELF_CERTIFY` had correct session auth but `proxy_users.roles` stayed `USER` forever - Adds `upsertUser(String, List<String>)` to `UserStore`, `JdbcUserStore`, `MongoUserStore`, and `CompositeUserStore` — inserts with resolved roles on first login, updates on subsequent logins so IdP group changes take effect on next sign-in - `SecurityConfig.provisionIdpUser()` now extracts role names from `auth.getAuthorities()` (stripping `ROLE_` prefix) and calls the new overload for both OIDC and LDAP paths Closes #249 ## Test plan - [ ] 3 new `JdbcUserStoreIntegrationTest` cases: new user gets correct roles, subsequent login syncs updated roles, YAML-seeded user gets DB roles overwritten by IdP on login - [ ] All existing `upsertUser` tests still pass — no-arg callers (`CompositeUserStore` internal provisioning) are unaffected - [ ] Manual: OIDC or LDAP login with a user mapped to `ADMIN` or `SELF_CERTIFY` group — verify `proxy_users.roles` reflects the correct roles after login
2 parents 429339e + e714908 commit fa4af73

7 files changed

Lines changed: 77 additions & 7 deletions

File tree

git-proxy-java-core/src/main/java/org/finos/gitproxy/user/CompositeUserStore.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,11 @@ public void upsertUser(String username) {
184184
mutableStore.upsertUser(username);
185185
}
186186

187+
@Override
188+
public void upsertUser(String username, List<String> roles) {
189+
mutableStore.upsertUser(username, roles);
190+
}
191+
187192
@Override
188193
public void upsertLockedEmail(String username, String email, String authSource) {
189194
mutableStore.upsertLockedEmail(username, email, authSource);

git-proxy-java-core/src/main/java/org/finos/gitproxy/user/JdbcUserStore.java

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -208,14 +208,25 @@ public void removeScmIdentity(String username, String provider, String scmUserna
208208
* The password is left NULL so the account cannot be used for form login.
209209
*/
210210
public void upsertUser(String username) {
211+
upsertUser(username, List.of("USER"));
212+
}
213+
214+
@Override
215+
public void upsertUser(String username, List<String> roles) {
216+
String rolesStr = roles.isEmpty() ? "USER" : String.join(",", roles);
211217
boolean exists = !jdbc.queryForList(
212218
"SELECT username FROM proxy_users WHERE username = :u", Map.of("u", username), String.class)
213219
.isEmpty();
214220
if (!exists) {
215221
jdbc.update(
216-
"INSERT INTO proxy_users (username, password_hash, roles) VALUES (:u, NULL, 'USER')",
217-
Map.of("u", username));
218-
log.debug("Auto-provisioned IdP user '{}'", username);
222+
"INSERT INTO proxy_users (username, password_hash, roles) VALUES (:u, NULL, :roles)",
223+
Map.of("u", username, "roles", rolesStr));
224+
log.debug("Auto-provisioned IdP user '{}' with roles {}", username, rolesStr);
225+
} else {
226+
jdbc.update(
227+
"UPDATE proxy_users SET roles = :roles WHERE username = :u",
228+
Map.of("u", username, "roles", rolesStr));
229+
log.debug("Synced IdP roles for user '{}': {}", username, rolesStr);
219230
}
220231
}
221232

git-proxy-java-core/src/main/java/org/finos/gitproxy/user/MongoUserStore.java

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,14 +157,23 @@ public void setPassword(String username, String passwordHash) {
157157

158158
@Override
159159
public void upsertUser(String username) {
160+
upsertUser(username, List.of("USER"));
161+
}
162+
163+
@Override
164+
public void upsertUser(String username, List<String> roles) {
165+
String rolesStr = roles.isEmpty() ? "USER" : String.join(",", roles);
160166
if (getCollection().find(Filters.eq("_id", username)).first() == null) {
161167
getCollection()
162168
.insertOne(new Document("_id", username)
163169
.append("passwordHash", null)
164-
.append("roles", "USER")
170+
.append("roles", rolesStr)
165171
.append("emails", List.of())
166172
.append("scmIdentities", List.of()));
167-
log.debug("Auto-provisioned IdP user '{}'", username);
173+
log.debug("Auto-provisioned IdP user '{}' with roles {}", username, rolesStr);
174+
} else {
175+
getCollection().updateOne(Filters.eq("_id", username), Updates.set("roles", rolesStr));
176+
log.debug("Synced IdP roles for user '{}': {}", username, rolesStr);
168177
}
169178
}
170179

git-proxy-java-core/src/main/java/org/finos/gitproxy/user/UserStore.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,14 @@ public interface UserStore extends ReadOnlyUserStore {
6262
*/
6363
void upsertUser(String username);
6464

65+
/**
66+
* Ensures a user row exists and syncs the given roles on every IdP login. Roles are authoritative from the IdP —
67+
* any existing roles are overwritten so that IdP group changes take effect on next sign-in.
68+
*/
69+
default void upsertUser(String username, List<String> roles) {
70+
upsertUser(username);
71+
}
72+
6573
/** Inserts or updates an email for a user as locked (owned by the identity provider). */
6674
void upsertLockedEmail(String username, String email, String authSource);
6775

git-proxy-java-core/src/test/java/org/finos/gitproxy/user/JdbcUserStoreIntegrationTest.java

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,38 @@ void upsertUser_preservesExistingUser() {
247247
assertEquals("{noop}pw", result.get().getPasswordHash());
248248
}
249249

250+
@Test
251+
void upsertUser_withRoles_setsRolesOnNewUser() {
252+
store.upsertUser("oidcuser", List.of("USER", "ADMIN"));
253+
254+
var result = store.findByUsername("oidcuser");
255+
assertTrue(result.isPresent());
256+
assertNull(result.get().getPasswordHash());
257+
assertTrue(result.get().getRoles().contains("ADMIN"));
258+
assertTrue(result.get().getRoles().contains("USER"));
259+
}
260+
261+
@Test
262+
void upsertUser_withRoles_syncsRolesOnSubsequentLogin() {
263+
store.upsertUser("oidcuser", List.of("USER"));
264+
store.upsertUser("oidcuser", List.of("USER", "SELF_CERTIFY"));
265+
266+
var result = store.findByUsername("oidcuser");
267+
assertTrue(result.isPresent());
268+
assertTrue(result.get().getRoles().contains("SELF_CERTIFY"));
269+
}
270+
271+
@Test
272+
void upsertUser_withRoles_syncsRolesForYamlUser() {
273+
store.upsertAll(List.of(user("alice", List.of(), List.of())));
274+
store.upsertUser("alice", List.of("USER", "ADMIN"));
275+
276+
var result = store.findByUsername("alice");
277+
assertTrue(result.isPresent());
278+
assertEquals("{noop}pw", result.get().getPasswordHash());
279+
assertTrue(result.get().getRoles().contains("ADMIN"));
280+
}
281+
250282
// ---- upsertLockedEmail ----
251283

252284
@Test

git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,6 @@ function formatTime(ts: string | number | undefined) {
100100
}
101101
}
102102

103-
104103
/**
105104
* Build a direct link to the commit on the upstream SCM.
106105
* GitHub / Gitea / Codeberg: {repo}/commit/{sha}

git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/SecurityConfig.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,13 @@ private void provisionIdpUser(Authentication auth) {
474474

475475
if (authSource == null) return;
476476

477-
jdbc.upsertUser(username);
477+
List<String> roles = auth.getAuthorities().stream()
478+
.map(GrantedAuthority::getAuthority)
479+
.filter(a -> a.startsWith("ROLE_"))
480+
.map(a -> a.substring(5))
481+
.toList();
482+
if (roles.isEmpty()) roles = List.of("USER");
483+
jdbc.upsertUser(username, roles);
478484
if (email != null && !email.isBlank()) {
479485
try {
480486
jdbc.upsertLockedEmail(username, email.strip().toLowerCase(), authSource);

0 commit comments

Comments
 (0)