Skip to content
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

Integrated code lifecycle: Generate authentication tokens to authenticate against localVC #8664

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/main/java/de/tum/in/www1/artemis/config/Constants.java
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,11 @@ public final class Constants {
*/
public static final String PROFILE_LOCALCI = "localci";

/**
* The name of the Spring profile used for gitlab.
*/
public static final String PROFILE_GITLAB = "gitlab";
Comment on lines +288 to +291
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.java
    • src/test/java/de/tum/in/www1/artemis/AbstractSpringIntegrationJenkinsGitlabTest.java
    • src/test/java/de/tum/in/www1/artemis/connector/GitlabRequestMockProvider.java
    • src/main/java/de/tum/in/www1/artemis/service/connectors/gitlab/GitLabPersonalAccessTokenManagementService.java
    • src/main/java/de/tum/in/www1/artemis/service/connectors/gitlab/GitLabService.java
    • src/main/java/de/tum/in/www1/artemis/service/connectors/gitlab/GitLabUserManagementService.java
    • src/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


/**
* The name of the Spring profile used to process build jobs in a local CI setup.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,17 @@ OR CONCAT(user.firstName, ' ', user.lastName) LIKE %:#{#loginOrName}%
""")
void updateUserSshPublicKeyHash(@Param("userId") long userId, @Param("sshPublicKeyHash") String sshPublicKeyHash, @Param("sshPublicKey") String sshPublicKey);

@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);
Comment on lines +572 to +581
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@Modifying
@Transactional // ok because of modifying query
@Query("""
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package de.tum.in.www1.artemis.service.connectors.gitlab;

import static de.tum.in.www1.artemis.config.Constants.PROFILE_GITLAB;

import java.net.URL;
import java.util.Optional;

Expand All @@ -12,7 +14,7 @@
import de.tum.in.www1.artemis.config.Constants;

@Component
@Profile("gitlab")
@Profile(PROFILE_GITLAB)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public class GitlabInfoContributor implements InfoContributor {

@Value("${artemis.version-control.url}")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to the TODOs above? Can we remove them?


// Store ssh url template
try {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package de.tum.in.www1.artemis.service.connectors.localvc;

import static de.tum.in.www1.artemis.config.Constants.PROFILE_LOCALVC;

import java.security.SecureRandom;
import java.time.Duration;
import java.time.ZonedDateTime;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Profile;
import org.springframework.stereotype.Service;

import de.tum.in.www1.artemis.domain.User;
import de.tum.in.www1.artemis.repository.UserRepository;
import de.tum.in.www1.artemis.service.connectors.vcs.VcsTokenManagementService;
import de.tum.in.www1.artemis.web.rest.UserResource;

@Service
@Profile(PROFILE_LOCALVC)
public class LocalVCPersonalAccessTokenManagementService extends VcsTokenManagementService {

private static final Logger log = LoggerFactory.getLogger(UserResource.class);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
private static final Logger log = LoggerFactory.getLogger(UserResource.class);
private static final Logger log = LoggerFactory.getLogger(LocalVCPersonalAccessTokenManagementService.class);


private final UserRepository userRepository;

@Value("${artemis.version-control.version-control-access-token:#{false}}")
private Boolean versionControlAccessToken;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name does not sound like it should be of type boolean. What is it supposed to mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also change it to the base type, since we have a default value?


@Value("${artemis.version-control.vc-access-token-max-lifetime-in-days:365}")
private int vcMaxLifetimeInDays;
Comment on lines +28 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )


private static final String TOKEN_PREFIX = "vcpat-";

private static final int RANDOM_STRING_LENGTH = 40;

private static final String CHARACTERS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";

public LocalVCPersonalAccessTokenManagementService(UserRepository userRepository) {
super();
this.userRepository = userRepository;
}

@Override
public void createAccessToken(User user, Duration lifetime) {
if (versionControlAccessToken) {
userRepository.updateUserVCAccessToken(user.getId(), generateSecureToken(), ZonedDateTime.now().plus(Duration.ofDays(vcMaxLifetimeInDays)));
log.info("Created access token for user {}", user.getId());
}
}

@Override
public void renewAccessToken(User user, Duration newLifetime) {
if (versionControlAccessToken) {
if (user.getVcsAccessTokenExpiryDate() != null && user.getVcsAccessTokenExpiryDate().isBefore(ZonedDateTime.now())) {
// todo create new one here and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seams like the comment got cut off

userRepository.updateUserVCAccessToken(user.getId(), user.getVcsAccessToken(), ZonedDateTime.now().plus(Duration.ofDays(vcMaxLifetimeInDays)));
}
log.info("Renewed access token for user {}", user.getId());
}
}

public static String generateSecureToken() {
SecureRandom secureRandom = new SecureRandom();
StringBuilder randomString = new StringBuilder(RANDOM_STRING_LENGTH);

for (int i = 0; i < RANDOM_STRING_LENGTH; i++) {
int randomIndex = secureRandom.nextInt(CHARACTERS.length());
randomString.append(CHARACTERS.charAt(randomIndex));
}
return TOKEN_PREFIX + randomString.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public VcsTokenRenewalService(@Value("${artemis.version-control.version-control-
this.versionControlAccessToken = versionControlAccessToken;
this.vcsTokenManagementService = vcsTokenManagementService;
this.userRepository = userRepository;
renewAllVcsAccessTokens();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving the renewAllVcsAccessTokens() call out of the constructor to avoid potential performance issues and unexpected behavior during application startup.

}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)));
}
}

import static de.tum.in.www1.artemis.security.Role.EDITOR;
import static de.tum.in.www1.artemis.security.Role.INSTRUCTOR;
import static de.tum.in.www1.artemis.security.Role.STUDENT;
import static de.tum.in.www1.artemis.security.Role.TEACHING_ASSISTANT;

import java.time.Duration;
import java.time.Instant;
import java.time.ZonedDateTime;
import java.util.HashSet;
import java.util.Optional;
import java.util.Set;
Expand All @@ -32,7 +35,9 @@
import de.tum.in.www1.artemis.repository.OrganizationRepository;
import de.tum.in.www1.artemis.repository.UserRepository;
import de.tum.in.www1.artemis.security.SecurityUtils;
import de.tum.in.www1.artemis.service.ProfileService;
import de.tum.in.www1.artemis.service.connectors.ci.CIUserManagementService;
import de.tum.in.www1.artemis.service.connectors.localvc.LocalVCPersonalAccessTokenManagementService;
import de.tum.in.www1.artemis.service.connectors.vcs.VcsUserManagementService;
import de.tum.in.www1.artemis.web.rest.vm.ManagedUserVM;
import tech.jhipster.security.RandomUtil;
Expand All @@ -41,6 +46,8 @@
@Service
public class UserCreationService {

private final ProfileService profileService;

@Value("${artemis.user-management.use-external}")
private Boolean useExternalUserManagement;

Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use boolean ?


@Value("${artemis.version-control.vc-access-token-max-lifetime-in-days:365}")
private int vcMaxLifetimeInDays;

private static final Logger log = LoggerFactory.getLogger(UserCreationService.class);

private final UserRepository userRepository;
Expand All @@ -76,7 +89,7 @@ public class UserCreationService {

public UserCreationService(UserRepository userRepository, PasswordService passwordService, AuthorityRepository authorityRepository, CourseRepository courseRepository,
Optional<VcsUserManagementService> optionalVcsUserManagementService, Optional<CIUserManagementService> optionalCIUserManagementService, CacheManager cacheManager,
OrganizationRepository organizationRepository) {
OrganizationRepository organizationRepository, ProfileService profileService) {
this.userRepository = userRepository;
this.passwordService = passwordService;
this.authorityRepository = authorityRepository;
Expand All @@ -85,6 +98,7 @@ public UserCreationService(UserRepository userRepository, PasswordService passwo
this.optionalCIUserManagementService = optionalCIUserManagementService;
this.cacheManager = cacheManager;
this.organizationRepository = organizationRepository;
this.profileService = profileService;
}

/**
Expand Down Expand Up @@ -130,6 +144,11 @@ public User createUser(String login, @Nullable String password, @Nullable Set<St
// new user gets registration key
newUser.setActivationKey(RandomUtil.generateActivationKey());
newUser.setInternal(isInternal);
// new users gets new VCS access tokens, for the LocalVC setup
if (versionControlAccessToken && profileService.isProfileActive(PROFILE_LOCALVC)) {
newUser.setVcsAccessToken(LocalVCPersonalAccessTokenManagementService.generateSecureToken());
newUser.setVcsAccessTokenExpiryDate(ZonedDateTime.now().plus(Duration.ofDays(vcMaxLifetimeInDays)));
}

final var authority = authorityRepository.findById(STUDENT.getAuthority()).orElseThrow();
// needs to be mutable --> new HashSet<>(Set.of(...))
Expand Down Expand Up @@ -185,6 +204,12 @@ public User createUser(ManagedUserVM userDTO) {
catch (InvalidDataAccessApiUsageException | PatternSyntaxException pse) {
log.warn("Could not retrieve matching organizations from pattern: {}", pse.getMessage());
}

// new users gets new VCS access tokens, for the LocalVC setup
if (versionControlAccessToken && profileService.isProfileActive(PROFILE_LOCALVC)) {
user.setVcsAccessToken(LocalVCPersonalAccessTokenManagementService.generateSecureToken());
user.setVcsAccessTokenExpiryDate(ZonedDateTime.now().plus(Duration.ofDays(vcMaxLifetimeInDays)));
}
user.setGroups(userDTO.getGroups());
user.setActivated(true);
user.setInternal(true);
Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/config/application-localvc.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ artemis:
url: http://localhost:8000
build-agent-git-username: buildjob_user # Replace with more secure credentials for production. Required for https access to localvc. This config must be set for build agents and localvc.
build-agent-git-password: buildjob_password # Replace with more secure credentials for production. Required for https access to localvc. This config must be set for build agents and localvc. You can also use an ssh key

vc-access-token-max-lifetime-in-days: 365

Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ void testContribute() {
}

Info info = builder.build();
assertThat((Boolean) info.getDetails().get("versionControlAccessToken")).isFalse();
assertThat((Boolean) info.getDetails().get("versionControlAccessToken")).isTrue();

}
}
Loading