fix(server): provision first-login git provider users - #971
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds AuthenticatedGitProviderUserService to centralize resolving/provisioning users from JWT claims. AccountController endpoints now accept JwtAuthenticationToken and delegate user resolution; WorkspaceProvisioningService delegates authenticated-user checks to the new service. Tests and TestSecurityConfig updated to cover GitLab provisioning. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant AC as AccountController
participant AGP as AuthenticatedGitProviderUserService
participant GPR as GitProviderRepository
participant UR as UserRepository
Client->>AC: GET /user/settings (Bearer JWT)
AC->>AC: receive JwtAuthenticationToken
AC->>AGP: resolveOrProvisionCurrentUser(null)
AGP->>AGP: extract login & provider IDs from JWT
AGP->>UR: findByLogin(login)
alt user exists
UR-->>AGP: return user
else user missing
AGP->>GPR: findOrCreate(providerType, serverUrl)
GPR-->>AGP: return git provider
AGP->>UR: upsertUser(nativeId, avatar, name, provider)
UR-->>AGP: return created user
end
AGP-->>AC: Optional<User>
AC-->>Client: HTTP 200 (UserSettingsDTO)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.java (1)
259-289:⚠️ Potential issue | 🟡 MinorRemove duplicate Javadoc block.
There are two consecutive Javadoc blocks for the
ensureAuthenticatedUserExistsmethod (lines 259-273 and 275-285). The first block is orphaned and should be removed.📝 Remove the duplicate Javadoc
public Long resolveOrCreateGitLabUser(String patToken, String serverUrl, String accountLogin) { String resolvedServerUrl = resolveGitLabServerUrl(serverUrl); return syncGitLabUserForPAT(patToken, resolvedServerUrl, accountLogin); } - /** - * Ensures the currently authenticated Keycloak user has a corresponding git provider - * {`@link` User} entity so they can be assigned as workspace owner. - * - * <p>Reads identity from JWT claims to determine the user's provider: - * <ul> - * <li>{`@code` gitlab_id} → creates a GitLab user (uses the given serverUrl)</li> - * <li>{`@code` github_id} → creates a GitHub user</li> - * </ul> - * - * <p>This is needed because first-time users may not have a {`@code` User} entity yet - * (it's normally created during sync). Without this, workspace creation would fail - * because there is no owner to assign. - * - * `@param` gitLabServerUrl the GitLab server URL for GitLab users (resolved to default if blank) - */ /** * Ensures the currently authenticated Keycloak user has a corresponding git provider * {`@link` User} entity so they can be assigned as workspace owner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.java` around lines 259 - 289, Remove the duplicate Javadoc preceding the ensureAuthenticatedUserExists method so there is only one Javadoc block describing its behavior and params; keep the more accurate block that mentions GitLab identity and the IllegalStateException, delete the orphaned/first Javadoc, and ensure the method signature public void ensureAuthenticatedUserExists(String gitLabServerUrl) and its call to authenticatedGitProviderUserService.ensureCurrentGitLabUserExists(gitLabServerUrl) remain unchanged.
🧹 Nitpick comments (4)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/AuthenticatedGitProviderUserService.java (2)
21-39: Use Lombok annotations for consistency with codebase conventions.The service should use
@Slf4jfor logging and@RequiredArgsConstructorfor constructor injection per coding guidelines.♻️ Proposed refactor using Lombok
+import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +@Slf4j `@Service` +@RequiredArgsConstructor public class AuthenticatedGitProviderUserService { - private static final Logger log = LoggerFactory.getLogger(AuthenticatedGitProviderUserService.class); private static final String GITHUB_SERVER_URL = "https://github.com"; private final UserRepository userRepository; private final GitProviderRepository gitProviderRepository; private final GitLabProperties gitLabProperties; - - public AuthenticatedGitProviderUserService( - UserRepository userRepository, - GitProviderRepository gitProviderRepository, - GitLabProperties gitLabProperties - ) { - this.userRepository = userRepository; - this.gitProviderRepository = gitProviderRepository; - this.gitLabProperties = gitLabProperties; - }As per coding guidelines: "Use constructor injection via
@RequiredArgsConstructorannotation" and "Use@Slf4jlogging instead ofSystem.out.println()".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/AuthenticatedGitProviderUserService.java` around lines 21 - 39, Replace the manual logger and explicit constructor with Lombok annotations: remove the static Logger field `log` and annotate the class `AuthenticatedGitProviderUserService` with `@Slf4j` to provide a logger, and replace the explicit constructor by adding `@RequiredArgsConstructor` to enable constructor injection for `userRepository`, `gitProviderRepository`, and `gitLabProperties`; ensure all final fields remain final and delete the explicit constructor implementation while keeping other constants like `GITHUB_SERVER_URL` intact.
48-52: Simplify redundant Optional handling.After checking
isEmpty(), usingorElseThrow()is redundant. Useget()directly or restructure the flow.♻️ Suggested simplification
Optional<String> currentLogin = SecurityUtils.getCurrentUserLogin(); if (currentLogin.isEmpty()) { return Optional.empty(); } - String login = currentLogin.orElseThrow(); + String login = currentLogin.get();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/AuthenticatedGitProviderUserService.java` around lines 48 - 52, The code in AuthenticatedGitProviderUserService redundantly checks currentLogin.isEmpty() and then calls currentLogin.orElseThrow(); replace this pattern by either using currentLogin.get() after the emptiness check or, preferably, restructure to return/operate on SecurityUtils.getCurrentUserLogin().map(...) so you avoid the separate isEmpty() check and the orElseThrow() call; locate SecurityUtils.getCurrentUserLogin() and the local variable currentLogin in the method and apply one of these simplifications.server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.java (1)
214-221: Consider passing auth context directly to the service.The helper validates
authviaresolveAuthentication()but then calls the service which reads fromSecurityContextHolderindependently. While functionally correct (Spring populates both consistently), passing the JWT directly would make the data flow more explicit and testable.This is a low-priority suggestion for future improvement—current implementation works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.java` around lines 214 - 221, The method resolveOrProvisionCurrentUser in AccountController currently validates the JwtAuthenticationToken via resolveAuthentication(auth) but then calls authenticatedGitProviderUserService.resolveOrProvisionCurrentUser(null) which makes the service read from SecurityContextHolder; change the call to pass the JwtAuthenticationToken (e.g., authenticatedGitProviderUserService.resolveOrProvisionCurrentUser(auth)) and update the service method signature (and any internal usages) to accept a JwtAuthenticationToken so the authentication context is explicit and testable; ensure any references to resolveOrProvisionCurrentUser in the service and its tests are adjusted accordingly.server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java (1)
42-46: Extract provisioned user to avoid repeatedorElseThrow()calls.The same Optional is unwrapped three times. Extract once for clarity and performance.
♻️ Suggested simplification
var provisionedUser = userRepository.findByLogin("gitlabuser"); assertThat(provisionedUser).isPresent(); - assertThat(provisionedUser.orElseThrow().getNativeId()).isEqualTo(18024L); - assertThat(provisionedUser.orElseThrow().getProvider().getType()).isEqualTo(GitProviderType.GITLAB); - assertThat(provisionedUser.orElseThrow().getProvider().getServerUrl()).isEqualTo("https://gitlab.com"); + var user = provisionedUser.orElseThrow(); + assertThat(user.getNativeId()).isEqualTo(18024L); + assertThat(user.getProvider().getType()).isEqualTo(GitProviderType.GITLAB); + assertThat(user.getProvider().getServerUrl()).isEqualTo("https://gitlab.com"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java` around lines 42 - 46, Extract the Optional once and reuse the unwrapped user to avoid repeated orElseThrow() calls: in AccountControllerIntegrationTest where you declare var provisionedUser = userRepository.findByLogin("gitlabuser"), immediately do var user = provisionedUser.orElseThrow() and then assert on user.getNativeId(), user.getProvider().getType(), and user.getProvider().getServerUrl() instead of calling orElseThrow() for each assertion; this improves clarity and performance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.java`:
- Around line 286-289: Update the Javadoc for
WorkspaceProvisioningService.ensureAuthenticatedUserExists to reflect the actual
exception type and scenario: replace or augment the existing `@throws`
IllegalStateException with `@throws` ResponseStatusException (HTTP 409) to
indicate the method delegates to
authenticatedGitProviderUserService.ensureCurrentGitLabUserExists and will throw
a ResponseStatusException when the GitLab identity is missing or conflicts;
mention the HTTP 409 status and the missing GitLab identity condition so the
documentation matches the delegated method's behavior.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java`:
- Around line 24-27: Add a per-test cleanup by adding a `@BeforeEach` method in
AccountControllerIntegrationTest that calls databaseTestUtils.cleanDatabase() so
the test getUserSettingsProvisionsGitLabUserWhenMissing() starts from a clean
DB; locate the test class and create a method (annotated with `@BeforeEach`) that
invokes databaseTestUtils.cleanDatabase() before each test run.
- Around line 37-40: The test in AccountControllerIntegrationTest expecting
participateInResearch:false is incorrect because UserPreferences defaults
participateInResearch to true and AccountService.getUserSettings() provisions
defaults via getOrCreatePreferences() and toDTO(); update the assertion in
AccountControllerIntegrationTest to expect true (change .isEqualTo(false) to
.isEqualTo(true)) so the test matches the actual default behavior of
UserPreferences and the getUserSettings() flow.
---
Outside diff comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.java`:
- Around line 259-289: Remove the duplicate Javadoc preceding the
ensureAuthenticatedUserExists method so there is only one Javadoc block
describing its behavior and params; keep the more accurate block that mentions
GitLab identity and the IllegalStateException, delete the orphaned/first
Javadoc, and ensure the method signature public void
ensureAuthenticatedUserExists(String gitLabServerUrl) and its call to
authenticatedGitProviderUserService.ensureCurrentGitLabUserExists(gitLabServerUrl)
remain unchanged.
---
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.java`:
- Around line 214-221: The method resolveOrProvisionCurrentUser in
AccountController currently validates the JwtAuthenticationToken via
resolveAuthentication(auth) but then calls
authenticatedGitProviderUserService.resolveOrProvisionCurrentUser(null) which
makes the service read from SecurityContextHolder; change the call to pass the
JwtAuthenticationToken (e.g.,
authenticatedGitProviderUserService.resolveOrProvisionCurrentUser(auth)) and
update the service method signature (and any internal usages) to accept a
JwtAuthenticationToken so the authentication context is explicit and testable;
ensure any references to resolveOrProvisionCurrentUser in the service and its
tests are adjusted accordingly.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/AuthenticatedGitProviderUserService.java`:
- Around line 21-39: Replace the manual logger and explicit constructor with
Lombok annotations: remove the static Logger field `log` and annotate the class
`AuthenticatedGitProviderUserService` with `@Slf4j` to provide a logger, and
replace the explicit constructor by adding `@RequiredArgsConstructor` to enable
constructor injection for `userRepository`, `gitProviderRepository`, and
`gitLabProperties`; ensure all final fields remain final and delete the explicit
constructor implementation while keeping other constants like
`GITHUB_SERVER_URL` intact.
- Around line 48-52: The code in AuthenticatedGitProviderUserService redundantly
checks currentLogin.isEmpty() and then calls currentLogin.orElseThrow(); replace
this pattern by either using currentLogin.get() after the emptiness check or,
preferably, restructure to return/operate on
SecurityUtils.getCurrentUserLogin().map(...) so you avoid the separate isEmpty()
check and the orElseThrow() call; locate SecurityUtils.getCurrentUserLogin() and
the local variable currentLogin in the method and apply one of these
simplifications.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java`:
- Around line 42-46: Extract the Optional once and reuse the unwrapped user to
avoid repeated orElseThrow() calls: in AccountControllerIntegrationTest where
you declare var provisionedUser = userRepository.findByLogin("gitlabuser"),
immediately do var user = provisionedUser.orElseThrow() and then assert on
user.getNativeId(), user.getProvider().getType(), and
user.getProvider().getServerUrl() instead of calling orElseThrow() for each
assertion; this improves clarity and performance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2de6ca40-30f9-4f9f-b712-522fe1c198c9
📒 Files selected for processing (5)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/AuthenticatedGitProviderUserService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/TestSecurityConfig.java
Comment on lines
286
to
289
| @Transactional | ||
| public void ensureAuthenticatedUserExists(String gitLabServerUrl) { | ||
| String login = SecurityUtils.getCurrentUserLoginOrThrow(); | ||
|
|
||
| // Fast path: if user already exists in ANY provider, we're done | ||
| if (userRepository.findByLogin(login).isPresent()) { | ||
| return; | ||
| } | ||
|
|
||
| // Read identity from JWT | ||
| Authentication auth = | ||
| org.springframework.security.core.context.SecurityContextHolder.getContext().getAuthentication(); | ||
| if (auth == null || !(auth.getPrincipal() instanceof org.springframework.security.oauth2.jwt.Jwt jwt)) { | ||
| throw new IllegalStateException("No JWT found for authenticated user"); | ||
| } | ||
|
|
||
| Long gitlabId = jwt.getClaim("gitlab_id"); | ||
| if (gitlabId != null) { | ||
| String resolvedUrl = resolveGitLabServerUrl(gitLabServerUrl); | ||
| upsertGitLabUser(gitlabId, login, login, "", resolvedUrl + "/" + login, resolvedUrl, User.Type.USER); | ||
| return; | ||
| } | ||
|
|
||
| // No GitLab identity — check if they at least have a GitHub identity | ||
| Long githubId = jwt.getClaim("github_id"); | ||
| if (githubId != null) { | ||
| throw new org.springframework.web.server.ResponseStatusException( | ||
| org.springframework.http.HttpStatus.CONFLICT, | ||
| "You need to link your GitLab account before creating a GitLab workspace. " + | ||
| "Go to Settings → Linked Accounts to connect your GitLab identity." | ||
| ); | ||
| } | ||
|
|
||
| throw new org.springframework.web.server.ResponseStatusException( | ||
| org.springframework.http.HttpStatus.CONFLICT, | ||
| "No GitLab identity found. Please link your GitLab account in Settings → Linked Accounts." | ||
| ); | ||
| authenticatedGitProviderUserService.ensureCurrentGitLabUserExists(gitLabServerUrl); | ||
| } |
Contributor
There was a problem hiding this comment.
Update Javadoc to reflect new exception types.
The remaining Javadoc at line 284 states @throws IllegalStateException but the delegated method ensureCurrentGitLabUserExists throws ResponseStatusException (HTTP 409) for missing GitLab identity scenarios. Update the documentation to reflect the actual exception behavior.
📝 Fix throws documentation
* `@param` gitLabServerUrl the GitLab server URL (resolved to default if blank)
- * `@throws` IllegalStateException if the user has no GitLab identity linked
+ * `@throws` ResponseStatusException with HTTP 409 CONFLICT if the user has no GitLab identity linked
+ * `@throws` IllegalStateException if no JWT is found for the authenticated user
*/
`@Transactional`
public void ensureAuthenticatedUserExists(String gitLabServerUrl) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.java`
around lines 286 - 289, Update the Javadoc for
WorkspaceProvisioningService.ensureAuthenticatedUserExists to reflect the actual
exception type and scenario: replace or augment the existing `@throws`
IllegalStateException with `@throws` ResponseStatusException (HTTP 409) to
indicate the method delegates to
authenticatedGitProviderUserService.ensureCurrentGitLabUserExists and will throw
a ResponseStatusException when the GitLab identity is missing or conflicts;
mention the HTTP 409 status and the missing GitLab identity condition so the
documentation matches the delegated method's behavior.
Comment on lines
+24
to
+27
| @Test | ||
| @DisplayName("GET /user/settings provisions a GitLab user from JWT claims when no user row exists yet") | ||
| void getUserSettingsProvisionsGitLabUserWhenMissing() { | ||
| assertThat(userRepository.findByLogin("gitlabuser")).isEmpty(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if BaseIntegrationTest has `@BeforeEach` cleanup
rg -n "@BeforeEach|@AfterEach|databaseTestUtils" --type java server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.java
# Check how other integration tests handle cleanup
rg -n "@BeforeEach" --type java -A 3 server/application-server/src/test/java/de/tum/in/www1/hephaestus/ | head -50Repository: ls1intum/Hephaestus
Length of output: 6399
🏁 Script executed:
# Get full BaseIntegrationTest to check for `@BeforeEach` cleanup
cat -n server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/BaseIntegrationTest.javaRepository: ls1intum/Hephaestus
Length of output: 2843
🏁 Script executed:
# Check the AccountControllerIntegrationTest file to see if it has `@BeforeEach`
cat -n server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.javaRepository: ls1intum/Hephaestus
Length of output: 2362
🏁 Script executed:
# Check DatabaseTestUtils to understand cleanup requirements
cat -n server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/DatabaseTestUtils.java | head -100Repository: ls1intum/Hephaestus
Length of output: 4626
Add @BeforeEach cleanup to prevent test flakiness.
The test getUserSettingsProvisionsGitLabUserWhenMissing() asserts an empty precondition (findByLogin("gitlabuser").isEmpty()) but lacks @BeforeEach to invoke databaseTestUtils.cleanDatabase(). While BaseIntegrationTest provides the utility, cleanup must be explicitly called per test—following the pattern used in other integration tests like WorkspaceRepositoryCoverageIntegrationTest. Without it, the test will fail when previous test runs leave residual "gitlabuser" data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java`
around lines 24 - 27, Add a per-test cleanup by adding a `@BeforeEach` method in
AccountControllerIntegrationTest that calls databaseTestUtils.cleanDatabase() so
the test getUserSettingsProvisionsGitLabUserWhenMissing() starts from a clean
DB; locate the test class and create a method (annotated with `@BeforeEach`) that
invokes databaseTestUtils.cleanDatabase() before each test run.
Contributor
There was a problem hiding this comment.
Pull request overview
Provision git-provider users (especially GitLab) on first access to /user/settings so first-time logins don’t hit a 404 before any workspace-creation flow has created a User row.
Changes:
- Introduce
AuthenticatedGitProviderUserServiceto resolve/provision the current Git provider user from JWT claims. - Reuse the new service from both
AccountController(settings endpoints) andWorkspaceProvisioningService(GitLab workspace creation gating). - Add an integration test and extend the test JWT decoder with GitLab claims.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/AuthenticatedGitProviderUserService.java | New service that provisions GitLab/GitHub User rows from JWT claims. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/account/AccountController.java | Settings endpoints now resolve/provision a git-provider user instead of returning 404 when missing. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.java | Delegates GitLab “ensure user exists/linked” behavior to the new service. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java | New integration test asserting /user/settings provisions a GitLab user. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/TestSecurityConfig.java | Adds a mock GitLab JWT token with gitlab_id claim for tests. |
Comments suppressed due to low confidence (1)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/testconfig/TestSecurityConfig.java:86
- TestSecurityConfig’s class-level comment says the mock JWT contains the same realm_access structure as a real Keycloak token, but for tokens with an empty roles array (including the newly added GitLab token) the code omits the realm_access claim entirely. Either always include realm_access (with an empty roles list) or adjust the documentation so tests don’t accidentally rely on a non-production structure.
// Create a mock JWT that matches the structure expected by the main SecurityConfig
Map<String, Object> claims = new HashMap<>();
claims.put("sub", userId);
claims.put("preferred_username", username);
claims.put("iss", "https://test-issuer");
claims.put("aud", "test-audience");
if ("mock-jwt-token-for-gitlab-user".equals(token)) {
claims.put("gitlab_id", 18024L);
claims.put("identity_provider", "gitlab-lrz");
}
// Add realm_access with roles (same structure as Keycloak)
if (roles.length > 0) {
Map<String, Object> realmAccess = new HashMap<>();
realmAccess.put("roles", Arrays.asList(roles));
claims.put("realm_access", realmAccess);
}
| @@ -65,7 +66,8 @@ | |||
| WorkspaceMembershipRepository workspaceMembershipRepository, | |||
| WorkspaceMembershipService workspaceMembershipService, | |||
| WorkspaceScopeFilter workspaceScopeFilter, | |||
There was a problem hiding this comment.
The constructor signature was changed to include AuthenticatedGitProviderUserService; there are direct instantiations of WorkspaceProvisioningService in tests (e.g., WorkspaceProvisioningServiceTest) that will no longer compile until updated to pass this dependency (likely as a mock).
Suggested change
| WorkspaceScopeFilter workspaceScopeFilter, | |
| WorkspaceScopeFilter workspaceScopeFilter, | |
| GitLabProperties gitLabProperties | |
| ) { | |
| this( | |
| workspaceProperties, | |
| workspaceRepository, | |
| repositoryToMonitorRepository, | |
| workspaceService, | |
| workspaceInstallationService, | |
| workspaceRepositoryMonitorService, | |
| gitHubAppTokenService, | |
| userRepository, | |
| gitProviderRepository, | |
| workspaceMembershipRepository, | |
| workspaceMembershipService, | |
| workspaceScopeFilter, | |
| gitLabProperties, | |
| null | |
| ); | |
| } | |
| public WorkspaceProvisioningService( | |
| WorkspaceProperties workspaceProperties, | |
| WorkspaceRepository workspaceRepository, | |
| RepositoryToMonitorRepository repositoryToMonitorRepository, | |
| WorkspaceService workspaceService, | |
| WorkspaceInstallationService workspaceInstallationService, | |
| WorkspaceRepositoryMonitorService workspaceRepositoryMonitorService, | |
| GitHubAppTokenService gitHubAppTokenService, | |
| UserRepository userRepository, | |
| GitProviderRepository gitProviderRepository, | |
| WorkspaceMembershipRepository workspaceMembershipRepository, | |
| WorkspaceMembershipService workspaceMembershipService, | |
| WorkspaceScopeFilter workspaceScopeFilter, |
Comment on lines
259
to
286
| /** | ||
| * Ensures the currently authenticated Keycloak user has a corresponding git provider | ||
| * {@link User} entity so they can be assigned as workspace owner. | ||
| * | ||
| * <p>Reads identity from JWT claims to determine the user's provider: | ||
| * <ul> | ||
| * <li>{@code gitlab_id} → creates a GitLab user (uses the given serverUrl)</li> | ||
| * <li>{@code github_id} → creates a GitHub user</li> | ||
| * </ul> | ||
| * | ||
| * <p>This is needed because first-time users may not have a {@code User} entity yet | ||
| * (it's normally created during sync). Without this, workspace creation would fail | ||
| * because there is no owner to assign. | ||
| * | ||
| * @param gitLabServerUrl the GitLab server URL for GitLab users (resolved to default if blank) | ||
| */ | ||
| /** | ||
| * Ensures the currently authenticated Keycloak user has a corresponding git provider | ||
| * {@link User} entity so they can be assigned as workspace owner. | ||
| * | ||
| * <p>For GitLab workspaces, the user must have a linked GitLab identity ({@code gitlab_id} | ||
| * in their JWT). If they logged in via GitHub without linking GitLab, this method throws | ||
| * so the frontend can prompt them to link their account first. | ||
| * | ||
| * @param gitLabServerUrl the GitLab server URL (resolved to default if blank) | ||
| * @throws IllegalStateException if the user has no GitLab identity linked | ||
| */ | ||
| @Transactional |
There was a problem hiding this comment.
There are two consecutive Javadoc blocks for ensureAuthenticatedUserExists; the first one (describing provisioning both GitLab and GitHub users) appears to be leftover and now contradicts the method behavior (it delegates to ensureCurrentGitLabUserExists). Please remove/merge the duplicate Javadoc so the method has a single, accurate description.
Comment on lines
+86
to
+88
| if (userRepository.findByLogin(login).isPresent()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
ensureCurrentGitLabUserExists returns early if any User with the current login exists, even if that user belongs to a non-GitLab provider. This can bypass the intended 409 conflict when a GitHub-authenticated user (no gitlab_id) already has a GitHub user row and then tries to create a GitLab PAT workspace. Consider only short-circuiting when a GitLab-scoped user exists (matching the resolved GitLab provider), or always validating presence of gitlab_id regardless of existing users.
Suggested change
| if (userRepository.findByLogin(login).isPresent()) { | |
| return; | |
| } |
Comment on lines
+14
to
+28
| @AutoConfigureWebTestClient | ||
| @DisplayName("Account controller integration") | ||
| class AccountControllerIntegrationTest extends BaseIntegrationTest { | ||
|
|
||
| @Autowired | ||
| private WebTestClient webTestClient; | ||
|
|
||
| @Autowired | ||
| private UserRepository userRepository; | ||
|
|
||
| @Test | ||
| @DisplayName("GET /user/settings provisions a GitLab user from JWT claims when no user row exists yet") | ||
| void getUserSettingsProvisionsGitLabUserWhenMissing() { | ||
| assertThat(userRepository.findByLogin("gitlabuser")).isEmpty(); | ||
|
|
There was a problem hiding this comment.
This integration test assumes the database does not already contain a user with login "gitlabuser". Other integration tests in this codebase call databaseTestUtils.cleanDatabase() in @BeforeEach to remain independent under parallel execution; this test should do the same (or use a unique per-test login) to avoid flakes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningServiceTest.java`:
- Around line 62-64: Add unit tests in WorkspaceProvisioningServiceTest that
exercise the new delegation path: mock AuthenticatedGitProviderUserService and
verify WorkspaceProvisioningService.ensureAuthenticatedUserExists(String) calls
the injected service; add one test where the mocked
AuthenticatedGitProviderUserService returns normally and asserts no exception is
thrown and interactions occurred, and add tests where the mock throws a
ResponseStatusException and an IllegalStateException respectively, asserting the
same exceptions propagate from ensureAuthenticatedUserExists. Use the existing
`@Mock` authenticatedGitProviderUserService and the service under test
(WorkspaceProvisioningService) to set up behavior and verify interactions for
these edge cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19a33c88-45d8-4350-bea8-0a34fc070331
📒 Files selected for processing (1)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningServiceTest.java
Comment on lines
+62
to
+64
| @Mock | ||
| private AuthenticatedGitProviderUserService authenticatedGitProviderUserService; | ||
|
|
Contributor
There was a problem hiding this comment.
Add unit coverage for the new authenticated-user delegation path.
AuthenticatedGitProviderUserService is now injected, but this test class does not exercise WorkspaceProvisioningService.ensureAuthenticatedUserExists(String) at all. That leaves delegation and exception propagation (ResponseStatusException / IllegalStateException) unverified, even though this is central to the PR behavior.
✅ Proposed test additions
@@
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@@
+import org.springframework.http.HttpStatus;
+import org.springframework.web.server.ResponseStatusException;
@@
class WorkspaceProvisioningServiceTest {
@@
`@Test`
+ void shouldDelegateToAuthenticatedGitProviderUserServiceWhenEnsuringAuthenticatedUserExists() {
+ // Arrange
+ String gitLabServerUrl = "https://gitlab.com";
+
+ // Act
+ provisioningService.ensureAuthenticatedUserExists(gitLabServerUrl);
+
+ // Assert
+ verify(authenticatedGitProviderUserService).ensureCurrentGitLabUserExists(gitLabServerUrl);
+ }
+
+ `@Test`
+ void shouldPropagateConflictWhenAuthenticatedGitProviderUserServiceThrowsConflict() {
+ // Arrange
+ String gitLabServerUrl = "https://gitlab.com";
+ ResponseStatusException conflict = new ResponseStatusException(HttpStatus.CONFLICT, "link account");
+ when(authenticatedGitProviderUserService.ensureCurrentGitLabUserExists(gitLabServerUrl)).thenThrow(conflict);
+
+ // Act + Assert
+ assertThatThrownBy(() -> provisioningService.ensureAuthenticatedUserExists(gitLabServerUrl))
+ .isSameAs(conflict);
+ }
+
+ `@Test`
+ void shouldPropagateIllegalStateWhenAuthenticatedGitProviderUserServiceThrowsIllegalState() {
+ // Arrange
+ String gitLabServerUrl = "https://gitlab.com";
+ IllegalStateException illegalState = new IllegalStateException("No JWT found");
+ when(authenticatedGitProviderUserService.ensureCurrentGitLabUserExists(gitLabServerUrl)).thenThrow(illegalState);
+
+ // Act + Assert
+ assertThatThrownBy(() -> provisioningService.ensureAuthenticatedUserExists(gitLabServerUrl))
+ .isSameAs(illegalState);
+ }
+
+ `@Test`
void bootstrapDefaultPatWorkspace_addsAdminAsMember() throws Exception {As per coding guidelines: "Focus on risk: cover critical flows and edge cases first when writing tests."
Also applies to: 99-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningServiceTest.java`
around lines 62 - 64, Add unit tests in WorkspaceProvisioningServiceTest that
exercise the new delegation path: mock AuthenticatedGitProviderUserService and
verify WorkspaceProvisioningService.ensureAuthenticatedUserExists(String) calls
the injected service; add one test where the mocked
AuthenticatedGitProviderUserService returns normally and asserts no exception is
thrown and interactions occurred, and add tests where the mock throws a
ResponseStatusException and an IllegalStateException respectively, asserting the
same exceptions propagate from ensureAuthenticatedUserExists. Use the existing
`@Mock` authenticatedGitProviderUserService and the service under test
(WorkspaceProvisioningService) to set up behavior and verify interactions for
these edge cases.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java (1)
25-29:⚠️ Potential issue | 🔴 CriticalAdd
@BeforeEachcleanup to ensure test isolation.The precondition assertion at line 29 (
assertThat(userRepository.findByLogin("gitlabuser")).isEmpty()) assumes a clean database state. Per coding guidelines, tests "may run in parallel" and should "assume that there might be data from previous tests." Without explicit cleanup viadatabaseTestUtils.cleanDatabase()in a@BeforeEachmethod, this test will fail if another test creates a "gitlabuser" row before this test runs.🛠️ Proposed fix
class AccountControllerIntegrationTest extends BaseIntegrationTest { + `@Autowired` + private DatabaseTestUtils databaseTestUtils; + `@Autowired` private WebTestClient webTestClient; `@Autowired` private UserRepository userRepository; + `@BeforeEach` + void setUp() { + databaseTestUtils.cleanDatabase(); + } + `@Test` `@Transactional`Also add the imports:
import de.tum.in.www1.hephaestus.testconfig.DatabaseTestUtils; import org.junit.jupiter.api.BeforeEach;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java` around lines 25 - 29, Add a `@BeforeEach` method in AccountControllerIntegrationTest that calls DatabaseTestUtils.cleanDatabase() to ensure a clean DB before each test; import de.tum.in.www1.hephaestus.testconfig.DatabaseTestUtils and org.junit.jupiter.api.BeforeEach, inject or use the existing DatabaseTestUtils instance in the test class, and run databaseTestUtils.cleanDatabase() so the precondition asserting userRepository.findByLogin("gitlabuser") isEmpty() will be reliable.
🧹 Nitpick comments (1)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java (1)
44-48: Extract user from Optional once to avoid repeatedorElseThrow()calls.Lines 46-48 call
orElseThrow()three times on the sameOptional. Extract the user once for cleaner assertions.♻️ Proposed refactor
var provisionedUser = userRepository.findByLogin("gitlabuser"); assertThat(provisionedUser).isPresent(); - assertThat(provisionedUser.orElseThrow().getNativeId()).isEqualTo(18024L); - assertThat(provisionedUser.orElseThrow().getProvider().getType()).isEqualTo(GitProviderType.GITLAB); - assertThat(provisionedUser.orElseThrow().getProvider().getServerUrl()).isEqualTo("https://gitlab.lrz.de"); + var user = provisionedUser.orElseThrow(); + assertThat(user.getNativeId()).isEqualTo(18024L); + assertThat(user.getProvider().getType()).isEqualTo(GitProviderType.GITLAB); + assertThat(user.getProvider().getServerUrl()).isEqualTo("https://gitlab.lrz.de");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java` around lines 44 - 48, In AccountControllerIntegrationTest, avoid calling orElseThrow() multiple times on the same Optional returned by userRepository.findByLogin("gitlabuser"); instead extract the value once (e.g. assign provisionedUser.orElseThrow() to a local variable) and use that variable for the subsequent assertions on getNativeId(), getProvider().getType(), and getProvider().getServerUrl() to make the test clearer and avoid repeated unwrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java`:
- Around line 25-29: Add a `@BeforeEach` method in
AccountControllerIntegrationTest that calls DatabaseTestUtils.cleanDatabase() to
ensure a clean DB before each test; import
de.tum.in.www1.hephaestus.testconfig.DatabaseTestUtils and
org.junit.jupiter.api.BeforeEach, inject or use the existing DatabaseTestUtils
instance in the test class, and run databaseTestUtils.cleanDatabase() so the
precondition asserting userRepository.findByLogin("gitlabuser") isEmpty() will
be reliable.
---
Nitpick comments:
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java`:
- Around line 44-48: In AccountControllerIntegrationTest, avoid calling
orElseThrow() multiple times on the same Optional returned by
userRepository.findByLogin("gitlabuser"); instead extract the value once (e.g.
assign provisionedUser.orElseThrow() to a local variable) and use that variable
for the subsequent assertions on getNativeId(), getProvider().getType(), and
getProvider().getServerUrl() to make the test clearer and avoid repeated
unwrapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ea1c2dd-8944-4bce-8b75-a717a05dd8ce
📒 Files selected for processing (1)
server/application-server/src/test/java/de/tum/in/www1/hephaestus/account/AccountControllerIntegrationTest.java
Contributor
📚 Documentation Preview
|
Collaborator
Author
|
🎉 This PR is included in version 0.56.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Provision the authenticated git-provider user on first access to account settings so GitLab users do not get a 404 before any workspace creation path has materialized their user row.
The bootstrap logic is extracted into a dedicated service and reused by workspace provisioning, avoiding controller-to-workspace-service coupling.
How to Test
npm run format && npm run check.npm run build:intelligence-service.npm run test:intelligence-service:unit.cd server/application-server && ./mvnw test -Dsurefire.includedGroups="unit" -Dmaven.test.skip=false -T 2C --batch-mode -q.cd server/application-server && ./mvnw -q -Dtest=AccountControllerIntegrationTest,GitLabWorkspaceCreationIntegrationTest test.Summary by CodeRabbit
New Features
Refactor
Tests