-
Notifications
You must be signed in to change notification settings - Fork 2
fix(server): provision first-login git provider users #971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a1038b2
48f6689
a53be72
03b744c
03aeee6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,208 @@ | ||
| package de.tum.in.www1.hephaestus.gitprovider.user; | ||
|
|
||
| import de.tum.in.www1.hephaestus.SecurityUtils; | ||
| import de.tum.in.www1.hephaestus.core.LoggingUtils; | ||
| import de.tum.in.www1.hephaestus.gitprovider.common.GitProvider; | ||
| import de.tum.in.www1.hephaestus.gitprovider.common.GitProviderRepository; | ||
| import de.tum.in.www1.hephaestus.gitprovider.common.GitProviderType; | ||
| import de.tum.in.www1.hephaestus.gitprovider.common.gitlab.GitLabProperties; | ||
| import java.util.Optional; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.lang.Nullable; | ||
| import org.springframework.security.core.Authentication; | ||
| import org.springframework.security.core.context.SecurityContextHolder; | ||
| import org.springframework.security.oauth2.jwt.Jwt; | ||
| import org.springframework.stereotype.Service; | ||
| import org.springframework.transaction.annotation.Transactional; | ||
| import org.springframework.web.server.ResponseStatusException; | ||
|
|
||
| @Service | ||
| 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; | ||
| } | ||
|
|
||
| @Transactional | ||
| public Optional<User> resolveOrProvisionCurrentUser(@Nullable String gitLabServerUrl) { | ||
| var currentUser = userRepository.getCurrentUser(); | ||
| if (currentUser.isPresent()) { | ||
| return currentUser; | ||
| } | ||
|
|
||
| Optional<String> currentLogin = SecurityUtils.getCurrentUserLogin(); | ||
| if (currentLogin.isEmpty()) { | ||
| return Optional.empty(); | ||
| } | ||
| String login = currentLogin.orElseThrow(); | ||
|
|
||
| Jwt jwt = getCurrentJwt(); | ||
| if (jwt == null) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| Long gitlabId = jwt.getClaim("gitlab_id"); | ||
| if (gitlabId != null) { | ||
| String resolvedUrl = resolveGitLabServerUrl(gitLabServerUrl); | ||
| Long userId = upsertGitLabUser( | ||
| gitlabId, | ||
| login, | ||
| login, | ||
| "", | ||
| resolvedUrl + "/" + login, | ||
| resolvedUrl, | ||
| User.Type.USER | ||
| ); | ||
| return userRepository.findById(userId); | ||
| } | ||
|
|
||
| Long githubId = jwt.getClaim("github_id"); | ||
| if (githubId != null) { | ||
| Long userId = upsertGitHubUser(githubId, login, login, "", GITHUB_SERVER_URL + "/" + login, User.Type.USER); | ||
| return userRepository.findById(userId); | ||
| } | ||
|
|
||
| return Optional.empty(); | ||
| } | ||
|
|
||
| @Transactional | ||
| public void ensureCurrentGitLabUserExists(@Nullable String gitLabServerUrl) { | ||
| String login = SecurityUtils.getCurrentUserLoginOrThrow(); | ||
| if (userRepository.findByLogin(login).isPresent()) { | ||
| return; | ||
| } | ||
|
|
||
| Jwt jwt = getCurrentJwt(); | ||
| if (jwt == null) { | ||
| 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; | ||
| } | ||
|
|
||
| Long githubId = jwt.getClaim("github_id"); | ||
| if (githubId != null) { | ||
| throw new ResponseStatusException( | ||
| 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 ResponseStatusException( | ||
| HttpStatus.CONFLICT, | ||
| "No GitLab identity found. Please link your GitLab account in Settings → Linked Accounts." | ||
| ); | ||
| } | ||
|
|
||
| @Nullable | ||
| private Jwt getCurrentJwt() { | ||
| Authentication auth = SecurityContextHolder.getContext().getAuthentication(); | ||
| if (auth == null || !(auth.getPrincipal() instanceof Jwt jwt)) { | ||
| return null; | ||
| } | ||
| return jwt; | ||
| } | ||
|
|
||
| private String resolveGitLabServerUrl(@Nullable String configServerUrl) { | ||
| if (configServerUrl != null && !configServerUrl.isBlank()) { | ||
| String url = configServerUrl.trim(); | ||
| return url.endsWith("/") ? url.substring(0, url.length() - 1) : url; | ||
| } | ||
| return gitLabProperties.defaultServerUrl(); | ||
| } | ||
|
|
||
| private Long upsertGitHubUser( | ||
| Long nativeId, | ||
| String login, | ||
| String name, | ||
| String avatarUrl, | ||
| String webUrl, | ||
| User.Type userType | ||
| ) { | ||
| GitProvider provider = gitProviderRepository | ||
| .findByTypeAndServerUrl(GitProviderType.GITHUB, GITHUB_SERVER_URL) | ||
| .orElseGet(() -> gitProviderRepository.save(new GitProvider(GitProviderType.GITHUB, GITHUB_SERVER_URL))); | ||
|
|
||
| return upsertUser(nativeId, login, name, avatarUrl, webUrl, userType, provider); | ||
| } | ||
|
|
||
| private Long upsertGitLabUser( | ||
| Long nativeId, | ||
| String login, | ||
| String name, | ||
| String avatarUrl, | ||
| String webUrl, | ||
| String serverUrl, | ||
| User.Type userType | ||
| ) { | ||
| String safeAvatar = avatarUrl != null ? (avatarUrl.startsWith("/") ? serverUrl + avatarUrl : avatarUrl) : ""; | ||
| GitProvider provider = gitProviderRepository | ||
| .findByTypeAndServerUrl(GitProviderType.GITLAB, serverUrl) | ||
| .orElseGet(() -> { | ||
| log.info("Creating GitProvider for self-hosted GitLab: serverUrl={}", serverUrl); | ||
| return gitProviderRepository.save(new GitProvider(GitProviderType.GITLAB, serverUrl)); | ||
| }); | ||
|
|
||
| return upsertUser(nativeId, login, name, safeAvatar, webUrl, userType, provider); | ||
| } | ||
|
|
||
| private Long upsertUser( | ||
| Long nativeId, | ||
| String login, | ||
| String name, | ||
| String avatarUrl, | ||
| String webUrl, | ||
| User.Type userType, | ||
| GitProvider provider | ||
| ) { | ||
| String safeName = name != null ? name : login; | ||
| String safeAvatar = avatarUrl != null ? avatarUrl : ""; | ||
| String safeWebUrl = webUrl != null ? webUrl : ""; | ||
| Long providerId = provider.getId(); | ||
|
|
||
| userRepository.acquireLoginLock(login, providerId); | ||
| userRepository.freeLoginConflicts(login, nativeId, providerId); | ||
| userRepository.upsertUser( | ||
| nativeId, | ||
| providerId, | ||
| login, | ||
| safeName, | ||
| safeAvatar, | ||
| safeWebUrl, | ||
| userType.name(), | ||
| null, | ||
| null, | ||
| null | ||
| ); | ||
| log.info( | ||
| "Upserted authenticated git provider user: userLogin={}, nativeId={}, providerType={}, type={}", | ||
| LoggingUtils.sanitizeForLog(login), | ||
| nativeId, | ||
| provider.getType(), | ||
| userType | ||
| ); | ||
| return userRepository | ||
| .findByLoginAndProviderId(login, providerId) | ||
| .map(User::getId) | ||
| .orElseThrow(() -> new IllegalStateException("User not found after upsert: login=" + login)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import de.tum.in.www1.hephaestus.gitprovider.common.github.app.GitHubAppTokenService; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import de.tum.in.www1.hephaestus.gitprovider.common.gitlab.GitLabProperties; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import de.tum.in.www1.hephaestus.gitprovider.common.spi.ProvisioningListener; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import de.tum.in.www1.hephaestus.gitprovider.user.AuthenticatedGitProviderUserService; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import de.tum.in.www1.hephaestus.gitprovider.user.User; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import de.tum.in.www1.hephaestus.gitprovider.user.UserRepository; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import de.tum.in.www1.hephaestus.workspace.WorkspaceMembership.WorkspaceRole; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -21,7 +22,6 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.slf4j.Logger; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.slf4j.LoggerFactory; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.http.HttpHeaders; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.security.core.Authentication; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.stereotype.Service; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.transaction.annotation.Transactional; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.springframework.web.reactive.function.client.WebClient; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -50,6 +50,7 @@ public class WorkspaceProvisioningService { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final WorkspaceMembershipService workspaceMembershipService; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final WorkspaceScopeFilter workspaceScopeFilter; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final GitLabProperties gitLabProperties; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final AuthenticatedGitProviderUserService authenticatedGitProviderUserService; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final WebClient webClient; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public WorkspaceProvisioningService( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -65,7 +66,8 @@ public WorkspaceProvisioningService( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WorkspaceMembershipRepository workspaceMembershipRepository, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WorkspaceMembershipService workspaceMembershipService, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WorkspaceScopeFilter workspaceScopeFilter, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.