Integrated code lifecycle: Generate authentication tokens to authenticate against localVC
#8664
Conversation
SimonEntholzer
added
tests
server
# Conflicts: # src/test/java/de/tum/in/www1/artemis/service/ParticipationServiceTest.java
SimonEntholzer
changed the title
Integrated code lifecycle: Add authentication token to authenticate against localVCIntegrated code lifecycleGenerate authentication tokens to authenticate against localVC
SimonEntholzer
changed the title
Integrated code lifecycleGenerate authentication tokens to authenticate against localVCIntegrated code lifecycle: Generate authentication tokens to authenticate against localVC
SimonEntholzer
changed the title
Integrated code lifecycle: Generate authentication tokens to authenticate against localVCIntegrated code lifecycle: Generate authentication tokens to authenticate against localVC
# Conflicts: # src/main/resources/config/application-localvc.yml
…emis into feature/localvc-auth-tokens
github-actions
bot
added
the
config-change
SimonEntholzer
removed
the
client
WalkthroughThe updates introduce new functionalities and enhance existing ones across multiple components. Key changes include adding a new constant for GitLab profile, updating user VCS access tokens, improving maintainability by centralizing profile constants, activating access token details for LocalVC, and creating a new service for managing LocalVC access tokens. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UserCreationService as UserCreationService
participant LocalVCPersonalAccessTokenManagementService as LocalVCPersonalAccessTokenManagementService
participant UserRepository as UserRepository
User ->> UserCreationService: createUser()
alt Profile is LocalVC and VCS Access Token Enabled
UserCreationService ->> LocalVCPersonalAccessTokenManagementService: createAccessToken(user, lifetime)
LocalVCPersonalAccessTokenManagementService -->> UserCreationService: token, expiryDate
UserCreationService ->> UserRepository: updateUserVCAccessToken(userId, token, expiryDate)
end
UserCreationService -->> User: User Created
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @Modifying | ||
| @Transactional // ok because of modifying query | ||
| @Query(""" | ||
| UPDATE User user | ||
| SET user.vcsAccessToken = :vcsAccessToken, | ||
| user.vcsAccessTokenExpiryDate = :vcsAccessTokenExpiryDate | ||
| WHERE user.id = :userId | ||
| """) | ||
| void updateUserVCAccessToken(@Param("userId") long userId, @Param("vcsAccessToken") String vcsAccessToken, | ||
| @Param("vcsAccessTokenExpiryDate") ZonedDateTime vcsAccessTokenExpiryDate); |
There was a problem hiding this comment.
Review transactional boundaries and isolation levels.
The @Transactional annotation is used, but the isolation level is not specified. Consider specifying an isolation level if concurrent modifications are expected.
Validate the security of token storage and renewal processes.
Given the sensitivity of access token management, ensure that the storage and handling of tokens are secure. Consider implementing additional security measures such as encryption at rest and regular audits of token usage and expiration.
Ensure proper handling of time zones in vcsAccessTokenExpiryDate.
The method updateUserVCAccessToken uses ZonedDateTime for the vcsAccessTokenExpiryDate parameter. Ensure that the application consistently handles time zones across different layers to prevent potential time zone discrepancies.
| @Value("${artemis.version-control.version-control-access-token:#{false}}") | ||
| private Boolean versionControlAccessToken; | ||
|
|
||
| @Value("${artemis.version-control.vc-access-token-max-lifetime-in-days:365}") | ||
| private int vcMaxLifetimeInDays; |
There was a problem hiding this comment.
Review configuration property defaults and ensure they are documented.
The default values for versionControlAccessToken and vcMaxLifetimeInDays are embedded directly in the @Value annotations. It's a good practice to document these defaults either in the application properties file or in the class documentation to improve maintainability and clarity for other developers.
| @Profile(PROFILE_LOCALVC) | ||
| public class LocalVCPersonalAccessTokenManagementService extends VcsTokenManagementService { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(UserResource.class); |
There was a problem hiding this comment.
Logger class mismatch.
The logger is incorrectly associated with UserResource.class instead of LocalVCPersonalAccessTokenManagementService.class. This could lead to confusion when analyzing logs as they will incorrectly indicate they are originating from UserResource.
- private static final Logger log = LoggerFactory.getLogger(UserResource.class);
+ private static final Logger log = LoggerFactory.getLogger(LocalVCPersonalAccessTokenManagementService.class);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static final Logger log = LoggerFactory.getLogger(UserResource.class); | |
| private static final Logger log = LoggerFactory.getLogger(LocalVCPersonalAccessTokenManagementService.class); |
| /** | ||
| * The name of the Spring profile used for gitlab. | ||
| */ | ||
| public static final String PROFILE_GITLAB = "gitlab"; |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that the string "gitlab" is still hard-coded in several places in the codebase. These instances should be replaced with the new constant PROFILE_GITLAB to maintain consistency.
- Locations to update:
src/test/java/de/tum/in/www1/artemis/AbstractSpringIntegrationGitlabCIGitlabSamlTest.javasrc/test/java/de/tum/in/www1/artemis/AbstractSpringIntegrationJenkinsGitlabTest.javasrc/test/java/de/tum/in/www1/artemis/connector/GitlabRequestMockProvider.javasrc/main/java/de/tum/in/www1/artemis/service/connectors/gitlab/GitLabPersonalAccessTokenManagementService.javasrc/main/java/de/tum/in/www1/artemis/service/connectors/gitlab/GitLabService.javasrc/main/java/de/tum/in/www1/artemis/service/connectors/gitlab/GitLabUserManagementService.javasrc/main/java/de/tum/in/www1/artemis/service/connectors/gitlab/GitLabAuthorizationInterceptor.java
Please update these instances to use Constants.PROFILE_GITLAB instead of the hard-coded "gitlab".
Analysis chain
Ensure the new constant is appropriately used across the application.
The addition of PROFILE_GITLAB as a constant is a good practice for maintaining consistency and avoiding hard-coded values throughout the application. This change should be verified across all usages to ensure it replaces all previous hard-coded instances.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new constant `PROFILE_GITLAB` is used consistently across the application.
# Test: Search for the string "gitlab" that should now be replaced by the constant.
rg --type java '"gitlab"'
Length of output: 1149
| @@ -50,6 +50,7 @@ public VcsTokenRenewalService(@Value("${artemis.version-control.version-control- | |||
| this.versionControlAccessToken = versionControlAccessToken; | |||
| this.vcsTokenManagementService = vcsTokenManagementService; | |||
| this.userRepository = userRepository; | |||
| renewAllVcsAccessTokens(); | |||
There was a problem hiding this comment.
Consider moving the renewAllVcsAccessTokens() call out of the constructor to avoid potential performance issues and unexpected behavior during application startup.
| @@ -1,12 +1,15 @@ | |||
| package de.tum.in.www1.artemis.service.user; | |||
|
|
|||
| import static de.tum.in.www1.artemis.config.Constants.PROFILE_CORE; | |||
| import static de.tum.in.www1.artemis.config.Constants.PROFILE_LOCALVC; | |||
There was a problem hiding this comment.
Refactor the VCS token generation logic into a separate method to improve code readability and maintainability.
- if (versionControlAccessToken && profileService.isProfileActive(PROFILE_LOCALVC)) {
- newUser.setVcsAccessToken(LocalVCPersonalAccessTokenManagementService.generateSecureToken());
- newUser.setVcsAccessTokenExpiryDate(ZonedDateTime.now().plus(Duration.ofDays(vcMaxLifetimeInDays)));
- }
+ generateAndSetVcsAccessToken(newUser);
+ private void generateAndSetVcsAccessToken(User user) {
+ if (versionControlAccessToken && profileService.isProfileActive(PROFILE_LOCALVC)) {
+ user.setVcsAccessToken(LocalVCPersonalAccessTokenManagementService.generateSecureToken());
+ user.setVcsAccessTokenExpiryDate(ZonedDateTime.now().plus(Duration.ofDays(vcMaxLifetimeInDays)));
+ }
+ }Also applies to: 10-10, 12-12, 38-40, 49-49, 66-71, 92-92, 101-101, 147-151, 208-212
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import static de.tum.in.www1.artemis.config.Constants.PROFILE_LOCALVC; | |
| import static de.tum.in.www1.artemis.config.Constants.PROFILE_LOCALVC; | |
| private void generateAndSetVcsAccessToken(User user) { | |
| if (versionControlAccessToken && profileService.isProfileActive(PROFILE_LOCALVC)) { | |
| user.setVcsAccessToken(LocalVCPersonalAccessTokenManagementService.generateSecureToken()); | |
| user.setVcsAccessTokenExpiryDate(ZonedDateTime.now().plus(Duration.ofDays(vcMaxLifetimeInDays))); | |
| } | |
| } |
| @@ -43,7 +43,7 @@ public void contribute(Info.Builder builder) { | |||
| // TODO: only activate this when access tokens are available and make sure this does not lead to issues | |||
| // TODO: If activated, reflect this in LocalVCInfoContributorTest | |||
| // with the account.service.ts and its check if the access token is required | |||
| builder.withDetail(Constants.INFO_VERSION_CONTROL_ACCESS_TOKEN_DETAIL, false); | |||
| builder.withDetail(Constants.INFO_VERSION_CONTROL_ACCESS_TOKEN_DETAIL, true); | |||
There was a problem hiding this comment.
Is this related to the TODOs above? Can we remove them?
| private final UserRepository userRepository; | ||
|
|
||
| @Value("${artemis.version-control.version-control-access-token:#{false}}") | ||
| private Boolean versionControlAccessToken; |
There was a problem hiding this comment.
The name does not sound like it should be of type boolean. What is it supposed to mean?
There was a problem hiding this comment.
Can we also change it to the base type, since we have a default value?
| public void renewAccessToken(User user, Duration newLifetime) { | ||
| if (versionControlAccessToken) { | ||
| if (user.getVcsAccessTokenExpiryDate() != null && user.getVcsAccessTokenExpiryDate().isBefore(ZonedDateTime.now())) { | ||
| // todo create new one here and |
There was a problem hiding this comment.
Seams like the comment got cut off
| @@ -10,4 +10,4 @@ | |||
| <option name="Gradle.BeforeRunTask" enabled="false" tasks="build" externalProjectPath="$PROJECT_DIR$" vmOptions="" scriptParameters="-x webapp -x test -x jacocoTestCoverageVerification -x spotlessCheck -x checkstyleMain -x checkstyleTest" /> | |||
| </method> | |||
| </configuration> | |||
| </component> | |||
| </component> | |||
| private Boolean versionControlAccessToken; | ||
|
|
||
| @Value("${artemis.version-control.vc-access-token-max-lifetime-in-days:365}") | ||
| private int vcMaxLifetimeInDays; |
There was a problem hiding this comment.
Maybe rename the attribute. It's not vcmax time .. it's the token's lifetime (also as far as I understand the code. It's not the max lifetime; it's basically the lifetime :D )
| @@ -56,6 +63,12 @@ public class UserCreationService { | |||
| @Value("${info.guided-tour.course-group-instructors:#{null}}") | |||
| private Optional<String> tutorialGroupInstructors; | |||
|
|
|||
| @Value("${artemis.version-control.version-control-access-token:#{false}}") | |||
| private Boolean versionControlAccessToken; | |||
| @@ -12,7 +14,7 @@ | |||
| import de.tum.in.www1.artemis.config.Constants; | |||
|
|
|||
| @Component | |||
| @Profile("gitlab") | |||
| @Profile(PROFILE_GITLAB) | |||
There was a problem hiding this comment.
How is this GitLab stuff related to the feature of this Pull Request?
In general, introducing constant here is not bad, but it seems out of scope here to me.
|
This approach is outdated, we'll use a different approach to solve this. #8929 deprecates this PR |
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
To allow users to use the LocalVC repositories with access tokens, these must be generated in the first place.
Description
Added functionality in the LocalVC setup, which re-generates access tokens, when they are expiring.
Steps for Testing
VcsTokenRenewalService.javato run every minute, and then check in your database if all accounts have new Access tokens.Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Improvements
Tests