-
Notifications
You must be signed in to change notification settings - Fork 291
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
Changes from 11 commits
5cd247e
840cd91
29eb172
6592fc1
3f5cb60
726434c
b030480
6333487
66f51ba
b39b6c3
6242068
3006233
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Review transactional boundaries and isolation levels. The 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 The method |
||
|
||
@Modifying | ||
@Transactional // ok because of modifying query | ||
@Query(""" | ||
|
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; | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe 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? |
||
public class GitlabInfoContributor implements InfoContributor { | ||
|
||
@Value("${artemis.version-control.url}") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe 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 { | ||
|
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); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logger class mismatch. The logger is incorrectly associated with - private static final Logger log = LoggerFactory.getLogger(UserResource.class);
+ private static final Logger log = LoggerFactory.getLogger(LocalVCPersonalAccessTokenManagementService.class); Committable suggestion
Suggested change
|
||||||
|
||||||
private final UserRepository userRepository; | ||||||
|
||||||
@Value("${artemis.version-control.version-control-access-token:#{false}}") | ||||||
private Boolean versionControlAccessToken; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe rename the attribute. It's not |
||||||
|
||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider moving the |
||
} | ||
|
||
/** | ||
|
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; | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||||||||
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; | ||||||||||||||||||||
|
@@ -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; | ||||||||||||||||||||
|
@@ -41,6 +46,8 @@ | |||||||||||||||||||
@Service | ||||||||||||||||||||
public class UserCreationService { | ||||||||||||||||||||
|
||||||||||||||||||||
private final ProfileService profileService; | ||||||||||||||||||||
|
||||||||||||||||||||
@Value("${artemis.user-management.use-external}") | ||||||||||||||||||||
private Boolean useExternalUserManagement; | ||||||||||||||||||||
|
||||||||||||||||||||
|
@@ -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; | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use |
||||||||||||||||||||
|
||||||||||||||||||||
@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; | ||||||||||||||||||||
|
@@ -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; | ||||||||||||||||||||
|
@@ -85,6 +98,7 @@ public UserCreationService(UserRepository userRepository, PasswordService passwo | |||||||||||||||||||
this.optionalCIUserManagementService = optionalCIUserManagementService; | ||||||||||||||||||||
this.cacheManager = cacheManager; | ||||||||||||||||||||
this.organizationRepository = organizationRepository; | ||||||||||||||||||||
this.profileService = profileService; | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
/** | ||||||||||||||||||||
|
@@ -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(...)) | ||||||||||||||||||||
|
@@ -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); | ||||||||||||||||||||
|
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.
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.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:
Length of output: 1149