Skip to content

Commit

Permalink
add suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
SimonEntholzer committed Dec 22, 2024
1 parent 72fb576 commit 40be7f9
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -452,13 +452,23 @@ public void pullChanges(Repository repository) {
*
* @param repository for which to execute the commit.
* @param user the user who has committed the changes in the online editor
* @param domainId the id of the domain Object (participation) owning the repository
* @return an Optional of a preliminary VcsAccessLog
* @throws GitAPIException if the staging/committing process fails.
*/
public Optional<VcsAccessLog> commitChanges(Repository repository, User user, Long domainId) throws GitAPIException {
public void commitChanges(Repository repository, User user) throws GitAPIException {
gitService.stageAllChanges(repository);
gitService.commitAndPush(repository, "Changes by Online Editor", true, user);
}

/**
* Saves a preliminary access log for a push from the code editor, and returns it
*
* @param repository for which to execute the commit.
* @param user the user who has committed the changes in the online editor
* @param domainId the id that serves as an abstract identifier for retrieving the repository.
* @return an Optional of a preliminary VcsAccessLog
* @throws GitAPIException if accessing the repository fails
*/
public Optional<VcsAccessLog> savePreliminaryCodeEditorAccessLog(Repository repository, User user, Long domainId) throws GitAPIException {
return vcsAccessLogService.isPresent() ? vcsAccessLogService.get().createPreliminaryCodeEditorAccessLog(repository, user, domainId) : Optional.empty();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,12 +257,22 @@ public void authenticateAndAuthorizeGitRequest(HttpServletRequest request, Repos
}

var optionalParticipation = authorizeUser(repositoryTypeOrUserName, user, exercise, repositoryAction, localVCRepositoryUri, false);
storePreliminaryVcsAccessLogForHTTPs(request, localVCRepositoryUri, user, repositoryAction, optionalParticipation);
savePreliminaryVcsAccessLogForHTTPs(request, localVCRepositoryUri, user, repositoryAction, optionalParticipation);

log.debug("Authorizing user {} for repository {} took {}", user.getLogin(), localVCRepositoryUri, TimeLogUtil.formatDurationFrom(timeNanoStart));
}

private void storePreliminaryVcsAccessLogForHTTPs(HttpServletRequest request, LocalVCRepositoryUri localVCRepositoryUri, User user, RepositoryActionType repositoryAction,
/**
* Determines whether a given request to access a local VC repository (either via fetch of push) is authenticated and authorized.
*
* @param request The object containing all information about the incoming request.
* @param localVCRepositoryUri The uri of the requested repository
* @param user The user
* @param repositoryAction Indicates whether the method should authenticate a fetch or a push request. For a push request, additional checks are conducted.
* @param optionalParticipation The participation for which the access log should be stored. If an empty Optional is provided, the method does nothing
* @throws LocalVCAuthException If the user authentication fails or the user is not authorized to access a certain repository.
*/
private void savePreliminaryVcsAccessLogForHTTPs(HttpServletRequest request, LocalVCRepositoryUri localVCRepositoryUri, User user, RepositoryActionType repositoryAction,
Optional<ProgrammingExerciseParticipation> optionalParticipation) throws LocalVCAuthException {
if (optionalParticipation.isPresent()) {
ProgrammingExerciseParticipation participation = optionalParticipation.get();
Expand All @@ -279,7 +289,7 @@ private void storePreliminaryVcsAccessLogForHTTPs(HttpServletRequest request, Lo

String finalCommitHash = commitHash;
RepositoryActionType finalRepositoryAction = repositoryAction == RepositoryActionType.WRITE ? RepositoryActionType.PUSH : RepositoryActionType.PULL;
vcsAccessLogService.ifPresent(service -> service.storeAccessLog(user, participation, finalRepositoryAction, authenticationMechanism, finalCommitHash, ipAddress));
vcsAccessLogService.ifPresent(service -> service.saveAccessLog(user, participation, finalRepositoryAction, authenticationMechanism, finalCommitHash, ipAddress));
}
}

Expand Down Expand Up @@ -371,7 +381,6 @@ private boolean tryAuthenticationWithParticipationVCSAccessToken(User user, Stri
try {

// check participation vcs access token
// var part = programmingExerciseParticipationService.findTeamParticipationByExerciseAndTeamShortNameOrThrow()
List<ProgrammingExerciseStudentParticipation> participations;
Optional<ProgrammingExerciseStudentParticipation> studentParticipation;
if (exercise.isTeamMode()) {
Expand Down Expand Up @@ -525,24 +534,30 @@ private void checkAccessForRepository(ProgrammingExerciseParticipation participa

/**
* Checks if the provided repository is an auxiliary or test repository.
* Only load auxiliary repositories if a user is at least teaching assistant; students are not allowed to access them
* But: for students it only checks for test repository, and assumes the requested repository is not an auxiliary repository.
* This avoids an unnecessary database call, and postpones the actual check to
* {@link LocalVCServletService#tryToLoadParticipation(boolean, String, LocalVCRepositoryUri, ProgrammingExercise)}
* and only checks it if it is really needed.
*
* @param exercise the exercise, where the repository belongs to
* @param repositoryTypeOrUserName the type or username of the repository
* @param repositoryActionType the action that should be performed on of the repository
* @param user the user who tries to access the repository
* @return true if user is TA and
* @throws LocalVCForbiddenException
* @return true if the repository is an Auxiliary or Test repository, and the user has access to it.
* false for students if the repository is possibly an auxiliary repository, or
* false for TAs if the repository is neither auxiliary nor test
* @throws LocalVCForbiddenException if the user has no access rights for the requested repository
*/
private boolean checkIfRepositoryIsAuxiliaryOrTestRepository(ProgrammingExercise exercise, String repositoryTypeOrUserName, RepositoryActionType repositoryActionType,
User user) throws LocalVCForbiddenException {

// Students should not be able to access Test or Aux repositories.
// To save on db queries we do not check that, until after we tried to fetch the user repository as a user repository
// Students are not able to access Test or Aux repositories.
// To save on db queries we do not check whether it is an Aux repo here, as we would need to fetch them first.
if (!authorizationCheckService.isAtLeastTeachingAssistantForExercise(exercise, user)) {
if (repositoryTypeOrUserName.equals(RepositoryType.TESTS.toString())) {
throw new LocalVCForbiddenException();
}
// The user is a student, and the repository is not a test repository
return false;
}

Expand All @@ -555,8 +570,10 @@ private boolean checkIfRepositoryIsAuxiliaryOrTestRepository(ProgrammingExercise
catch (AccessForbiddenException e) {
throw new LocalVCForbiddenException(e);
}
// The user is at least TA, it is either an Auxiliary repository or a Test repository, and the user has access to it
return true;
}
// The repository is neither an Auxiliary repository nor a Test repository
return false;
}

Expand Down Expand Up @@ -653,7 +670,6 @@ public void processNewPush(String commitHash, Repository repository, Optional<Pr
ProgrammingExercise exercise = cachedExercise.orElseGet(() -> getProgrammingExercise(projectKey));
ProgrammingExerciseParticipation participation;
RepositoryType repositoryType = getRepositoryTypeWithoutAuxiliary(repositoryTypeOrUserName, exercise);
;

try {
participation = cachedParticipation.orElseGet(() -> programmingExerciseParticipationService
Expand Down Expand Up @@ -948,7 +964,7 @@ public void createVCSAccessLogForFailedAuthenticationAttempt(HttpServletRequest
var participation = programmingExerciseParticipationService.fetchParticipationWithSubmissionsByRepository(localVCRepositoryUri.getRepositoryTypeOrUserName(),
localVCRepositoryUri.toString(), null);
var ipAddress = servletRequest.getRemoteAddr();
vcsAccessLogService.ifPresent(service -> service.storeAccessLog(user, participation, RepositoryActionType.CLONE_FAIL, mechanism, "", ipAddress));
vcsAccessLogService.ifPresent(service -> service.saveAccessLog(user, participation, RepositoryActionType.CLONE_FAIL, mechanism, "", ipAddress));
}
catch (LocalVCAuthException | EntityNotFoundException ignored) {
// Caught when: 1) no user, or 2) no participation was found. In both cases it does not make sense to write a log
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public class VcsAccessLogService {
* @param ipAddress The ip address of the user accessing the repository
*/
@Async
public void storeAccessLog(User user, ProgrammingExerciseParticipation participation, RepositoryActionType actionType, AuthenticationMechanism authenticationMechanism,
public void saveAccessLog(User user, ProgrammingExerciseParticipation participation, RepositoryActionType actionType, AuthenticationMechanism authenticationMechanism,
String commitHash, String ipAddress) {
log.debug("Storing access operation for user {}", user);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,9 @@ public ResponseEntity<Void> commitChanges(Long domainId) {

return executeAndCheckForExceptions(() -> {
Repository repository = getRepository(domainId, RepositoryActionType.WRITE, true);
var vcsAccessLog = repositoryService.commitChanges(repository, user, domainId);
repositoryService.commitChanges(repository, user);
var vcsAccessLog = repositoryService.savePreliminaryCodeEditorAccessLog(repository, user, domainId);

// Trigger a build, and process the result. Only implemented for local CI.
// For GitLab + Jenkins, webhooks were added when creating the repository,
// that notify the CI system when the commit happens and thus trigger the build.
Expand Down

0 comments on commit 40be7f9

Please sign in to comment.