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

Development: Improve exercise configuration for GitLab CI #9608

Merged
merged 62 commits into from
Oct 27, 2024

Conversation

bensofficial
Copy link
Member

@bensofficial bensofficial commented Oct 27, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I documented the Java code using JavaDoc style.

Motivation and Context

This is the last PR as part of the project for bringing experimental support for GitLab CI to Artemis.

As proposed in my thesis, the GitLab CI variables required for the build and sending the results back to Artemis should primarily be group-wide. This means that the configuration is mainly done when creating the exercise and therefore minimizes the effort required for individual participation.

Also, we don't want to use the GitLab admin token to clone the test repository. We have added support for project tokens in GitLab4J (gitlab4j/gitlab4j-api#1018) to create a token for each test repository with a limited scope.

Description

In the GitLabCIService class, we now differentiate between group and project variables and set them accordingly. We have introduced a method to create the project tokens for the test repositories.
We adapted the .gitlab-ci.yml template and the tests to keep them up to date with the changes.

The findByIdWithBuildConfigElseThrow method was moved from the ProgrammingExerciseTestRepository to the ProgrammingExerciseRepository as we need it for our implementation.

Steps for Testing

  1. Log in to Artemis
  2. Navigate to Course Administration
  3. Create a new exercise
  4. Participate in the exercise
  5. Check that the result is shown correctly.

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.







Review Progress

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced repository functionality with new methods to retrieve programming exercises along with their build configurations.
    • Improved GitLab CI configuration management with methods for setting up configurations at both repository and group levels.
    • Introduced a method to create project access tokens, enhancing API interaction capabilities.
  • Bug Fixes

    • Refined error handling and logging for CI configuration processes.
  • Tests

    • Expanded test coverage for GitLab CI service functionalities, ensuring robust verification of interactions with the GitLab API.

@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) template core Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module labels Oct 27, 2024
@bensofficial bensofficial marked this pull request as ready for review October 27, 2024 11:21
@bensofficial bensofficial requested a review from a team as a code owner October 27, 2024 11:21
Copy link

coderabbitai bot commented Oct 27, 2024

Walkthrough

The changes in this pull request introduce new methods to the ProgrammingExerciseRepository and GitLabCIService classes, enhancing functionality related to programming exercises and GitLab CI configurations. The ProgrammingExerciseRepository now includes methods for retrieving exercises with their build configurations. The GitLabCIService class has been modified to manage CI configurations for both repositories and groups, with improved error handling and logging. Additionally, the test classes have been updated to reflect these changes, ensuring comprehensive testing of the new functionalities.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseRepository.java - Added method: Optional<ProgrammingExercise> findWithBuildConfigById(long exerciseId)
- Added method: ProgrammingExercise findByIdWithBuildConfigElseThrow(long programmingExerciseId)
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java - Added method: private void setupGitLabCIConfigurationForGroup(ProgrammingExercise exercise, boolean overwrite)
- Updated constructor to include ProgrammingExerciseRepository
- Added method: private String generateGitLabTestToken(ProgrammingExercise programmingExercise)
- Updated method: private void addBuildPlanToProgrammingExercise(ProgrammingExercise programmingExercise, boolean overwrite)
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIUserManagementService.java - Changed log level from error to debug in logUnsupportedAction method.
src/test/java/de/tum/cit/aet/artemis/core/connector/GitlabRequestMockProvider.java - Added method: public void mockCreateProjectAccessToken(boolean shouldFail)
src/test/java/de/tum/cit/aet/artemis/programming/service/GitlabCIServiceTest.java - Enhanced tests with new assertions and verifications in methods: testConfigureBuildPlanSuccess, testCreateBuildPlanForExercise, and testTriggerBuildSuccess.
src/test/java/de/tum/cit/aet/artemis/programming/test_repository/ProgrammingExerciseTestRepository.java - Removed method: default ProgrammingExercise findByIdWithBuildConfigElseThrow(long programmingExerciseId)
- Removed method: Optional<ProgrammingExercise> findWithBuildConfigById(long exerciseId)
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationGitlabCIGitlabSamlTest.java - Added line to mockAddBuildPlanToGitLabRepositoryConfiguration method to call gitlabRequestMockProvider.mockCreateProjectAccessToken(shouldFail);

Suggested labels

client, database, documentation

Suggested reviewers

  • SimonEntholzer
  • JohannesStoehr
  • coolchock
  • BBesrour

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIUserManagementService.java (1)

Line range hint 1-67: Enhance deprecation documentation with migration details.

While the class includes a deprecation notice, it would be helpful to add more detailed JavaDoc explaining:

  1. Why these operations are unsupported
  2. Step-by-step migration instructions
  3. Benefits of migrating to LocalVC

Add this documentation above the class declaration:

/**
 * GitLab CI implementation of user management operations.
 * All operations are unsupported as GitLab CI handles user management through its own interface.
 *
 * @deprecated Since 7.5.0, scheduled for removal in 8.0.0.
 * Migration guide:
 * 1. Review the LocalVC migration PR: https://github.com/ls1intum/Artemis/pull/8972
 * 2. Update your CI configuration to use LocalVC
 * 3. Test user management operations in your staging environment
 * 4. Remove GitLab CI specific configurations
 *
 * Benefits of LocalVC:
 * - Integrated user management
 * - Improved security
 * - Better performance
 */
src/test/java/de/tum/cit/aet/artemis/programming/service/GitlabCIServiceTest.java (4)

174-185: LGTM! Consider adding assertion for repository path.

The changes improve test readability and verification coverage. The extraction of repositoryPath enhances maintainability.

Consider adding an assertion to validate the extracted repository path:

assertThat(repositoryPath).isNotBlank().contains(participation.getVcsRepositoryUri());

210-219: Consider grouping related verifications together.

The verifications cover both project and group API interactions, but the order could be improved for better readability.

Consider reorganizing the verifications to group related calls:

 verify(gitlab, atLeastOnce()).getProjectApi();
 verify(gitlab, atLeastOnce()).getGroupApi();

-verify(gitlab.getProjectApi(), atLeastOnce()).getProject(eq(repositoryPath));
-verify(gitlab.getProjectApi(), atLeastOnce()).updateProject(any(Project.class));
-verify(gitlab.getProjectApi(), atLeastOnce()).getOptionalVariable(any(), anyString());
-verify(gitlab.getProjectApi(), atLeastOnce()).createVariable(anyString(), anyString(), anyString(), any(), anyBoolean(), anyBoolean());
-
-verify(gitlab.getGroupApi(), atLeastOnce()).getOptionalVariable(any(), anyString());
-verify(gitlab.getGroupApi(), atLeastOnce()).createVariable(anyString(), anyString(), anyString(), anyBoolean(), anyBoolean());

+// Project API verifications
+verify(gitlab.getProjectApi(), atLeastOnce()).getProject(eq(repositoryPath));
+verify(gitlab.getProjectApi(), atLeastOnce()).updateProject(any(Project.class));
+verify(gitlab.getProjectApi(), atLeastOnce()).getOptionalVariable(any(), anyString());
+verify(gitlab.getProjectApi(), atLeastOnce()).createVariable(anyString(), anyString(), anyString(), any(), anyBoolean(), anyBoolean());
+
+// Group API verifications
+verify(gitlab.getGroupApi(), atLeastOnce()).getOptionalVariable(any(), anyString());
+verify(gitlab.getGroupApi(), atLeastOnce()).createVariable(anyString(), anyString(), anyString(), anyBoolean(), anyBoolean());

Line range hint 92-93: Address TODO: Add assertions for build log statistics.

The test is missing assertions for average data and could benefit from more realistic build log entries.

Would you like me to help implement:

  1. Assertions for verifying the average build time statistics?
  2. More realistic build log entries that better represent actual CI/CD output?

Line range hint 1-255: Consider enhancing test names and assertions for better clarity.

While the test class generally follows good practices, consider these improvements to better align with coding guidelines:

  1. Make test names more descriptive by including the scenario being tested:
-void testConfigureBuildPlanSuccess()
+void shouldConfigureBuildPlanSuccessfullyWhenValidParticipationProvided()

-void testCreateBuildPlanForExercise()
+void shouldCreateBuildPlanForExerciseWithBothProjectAndGroupConfigurations()
  1. Add more specific assertions in methods like testHealth() to verify exact version information.
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseRepository.java (1)

749-760: Minor: Enhance JavaDoc with @return description.

The method implementation looks good, but consider adding a description for the @return tag in the JavaDoc to maintain consistency with other similar methods in the repository.

Apply this diff to improve the documentation:

     /**
      * Find a programming exercise by its id, with eagerly loaded build config.
      *
      * @param programmingExerciseId of the programming exercise.
-     * @return The programming exercise related to the given id
+     * @return The programming exercise with eagerly loaded build config
      * @throws EntityNotFoundException the programming exercise could not be found.
      */
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6e1c562 and f2e7dc5.

⛔ Files ignored due to path filters (3)
  • src/main/resources/templates/gitlabci/empty/regularRuns/.gitlab-ci.yml is excluded by !**/*.yml
  • src/main/resources/templates/gitlabci/java/maven/regularRuns/.gitlab-ci.yml is excluded by !**/*.yml
  • src/main/resources/templates/gitlabci/rust/regularRuns/.gitlab-ci.yml is excluded by !**/*.yml
📒 Files selected for processing (7)
  • src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseRepository.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java (9 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIUserManagementService.java (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/core/connector/GitlabRequestMockProvider.java (3 hunks)
  • src/test/java/de/tum/cit/aet/artemis/programming/service/GitlabCIServiceTest.java (2 hunks)
  • src/test/java/de/tum/cit/aet/artemis/programming/test_repository/ProgrammingExerciseTestRepository.java (0 hunks)
  • src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationGitlabCIGitlabSamlTest.java (1 hunks)
💤 Files with no reviewable changes (1)
  • src/test/java/de/tum/cit/aet/artemis/programming/test_repository/ProgrammingExerciseTestRepository.java
🧰 Additional context used
📓 Path-based instructions (6)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseRepository.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIUserManagementService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/test/java/de/tum/cit/aet/artemis/core/connector/GitlabRequestMockProvider.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/test/java/de/tum/cit/aet/artemis/programming/service/GitlabCIServiceTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationGitlabCIGitlabSamlTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

🪛 ast-grep
src/test/java/de/tum/cit/aet/artemis/core/connector/GitlabRequestMockProvider.java

[warning] 925-925: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (new GitLabApiException("Internal Error", 500))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 928-928: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (new ProjectAccessToken())
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 925-925: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new GitLabApiException("Internal Error", 500))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 928-928: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new ProjectAccessToken())
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

🔇 Additional comments (3)
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationGitlabCIGitlabSamlTest.java (1)

329-329: LGTM! Verify test coverage in child classes.

The addition of mockCreateProjectAccessToken aligns with the PR's objective of implementing project token support and follows the class's consistent mocking pattern.

Let's verify that this mock is properly tested in child classes:

✅ Verification successful

Let me analyze the review comment and the codebase to verify if the mock mockCreateProjectAccessToken is properly tested in child classes.


Let me search for test classes that extend AbstractSpringIntegrationGitlabCIGitlabSamlTest and use the mock through the parent class's method.


Let me try a different search approach to find the test classes and their usage patterns.


Let me check the implementation of the test classes to verify how they use the mock.


Based on the analysis of the codebase, I can now provide a final response:

The mock is properly tested through inheritance and test coverage

The mockCreateProjectAccessToken is well-tested through the class hierarchy:

  1. It's defined in AbstractSpringIntegrationGitlabCIGitlabSamlTest and used within mockAddBuildPlanToGitLabRepositoryConfiguration
  2. AbstractProgrammingIntegrationGitlabCIGitlabSamlTest extends the base class and inherits the mock setup
  3. GitlabCIServiceTest extends AbstractProgrammingIntegrationGitlabCIGitlabSamlTest and tests both success and failure scenarios:
    • Tests failure case with mockAddBuildPlanToGitLabRepositoryConfiguration(true)
    • Tests success case with mockAddBuildPlanToGitLabRepositoryConfiguration(false)

The mock is properly integrated into the test hierarchy and exercised through both positive and negative test cases.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find test classes that extend AbstractSpringIntegrationGitlabCIGitlabSamlTest
# and verify they exercise the new mock

# Find all test classes that extend our base class
ast-grep --pattern 'class $_ extends AbstractSpringIntegrationGitlabCIGitlabSamlTest {
  $$$
}'

# Find test methods that call mockAddBuildPlanToGitLabRepositoryConfiguration
rg -l "mockAddBuildPlanToGitLabRepositoryConfiguration.*true|false.*"

Length of output: 233104


Script:

#!/bin/bash
# Search for test classes that use mockCreateProjectAccessToken
rg -A 3 "mockCreateProjectAccessToken" 

Length of output: 1151


Script:

#!/bin/bash
# Find test classes that extend AbstractSpringIntegrationGitlabCIGitlabSamlTest and use mockAddBuildPlanToGitLabRepositoryConfiguration
rg -l "extends AbstractSpringIntegrationGitlabCIGitlabSamlTest" | xargs rg -l "mockAddBuildPlanToGitLabRepositoryConfiguration"

Length of output: 127


Script:

#!/bin/bash
# First, find all test classes that extend the abstract class
echo "=== Classes extending AbstractSpringIntegrationGitlabCIGitlabSamlTest ==="
rg -B 2 -A 2 "class \w+ extends AbstractSpringIntegrationGitlabCIGitlabSamlTest"

# Then look for usages of the mock setup method
echo -e "\n=== Usages of mockAddBuildPlanToGitLabRepositoryConfiguration ==="
rg -B 2 -A 2 "mockAddBuildPlanToGitLabRepositoryConfiguration\(.*\)"

Length of output: 6396


Script:

#!/bin/bash
# Check the implementation of GitlabCIServiceTest
echo "=== GitlabCIServiceTest implementation ==="
rg -A 20 "class GitlabCIServiceTest" src/test/java/de/tum/cit/aet/artemis/programming/service/GitlabCIServiceTest.java

# Check the implementation of AbstractProgrammingIntegrationGitlabCIGitlabSamlTest
echo -e "\n=== AbstractProgrammingIntegrationGitlabCIGitlabSamlTest implementation ==="
rg -A 20 "class AbstractProgrammingIntegrationGitlabCIGitlabSamlTest" src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationGitlabCIGitlabSamlTest.java

Length of output: 2022

src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseRepository.java (1)

124-125: LGTM! Method follows repository patterns.

The new query method is well-designed, following Spring Data JPA best practices and maintaining consistency with existing repository patterns.

src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java (1)

298-298: Ensure exception handling for missing programming exercises.

The method findByIdWithBuildConfigElseThrow may throw an exception if the ProgrammingExercise is not found:

ProgrammingExercise programmingExercise = programmingExerciseRepository.findByIdWithBuildConfigElseThrow(participation.getProgrammingExercise().getId());

Confirm that this exception is appropriately handled upstream to prevent unhandled exceptions.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f2e7dc5 and 47bb320.

📒 Files selected for processing (2)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java (9 hunks)
  • src/test/java/de/tum/cit/aet/artemis/core/connector/GitlabRequestMockProvider.java (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/test/java/de/tum/cit/aet/artemis/core/connector/GitlabRequestMockProvider.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

🪛 ast-grep
src/test/java/de/tum/cit/aet/artemis/core/connector/GitlabRequestMockProvider.java

[warning] 925-925: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (new GitLabApiException("Internal Error", 500))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 928-928: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (new ProjectAccessToken())
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 925-925: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new GitLabApiException("Internal Error", 500))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 928-928: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new ProjectAccessToken())
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java (3)

125-133: LGTM: Constructor changes follow best practices.

The addition of ProgrammingExerciseRepository follows constructor injection pattern and maintains immutability.


260-265: LGTM: Clean refactoring of build plan setup.

The addition of the overwrite parameter provides better control over build plan management while maintaining clean code principles.


232-233: Verify token cleanup mechanism.

The code creates project access tokens but doesn't handle cleanup of expired tokens. This could lead to token accumulation.

✅ Verification successful

Let me gather more specific information about the GitLab token cleanup mechanisms.


Token cleanup is properly handled - no action needed

The codebase has robust token management through the VcsTokenRenewalService which:

  • Automatically renews tokens before they expire via a scheduled task
  • Uses configurable expiry dates (6 months by default, or course end date)
  • Includes cleanup mechanisms through deleteParticipationVcsAccessToken and related methods

The project access tokens are self-limiting through their expiration dates and don't require manual cleanup.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any token cleanup mechanisms in the codebase
echo "Searching for token cleanup implementations..."
rg -i "cleanup.*token|delete.*token|remove.*token" --type java

# Check for scheduled tasks that might handle token cleanup
echo "Searching for scheduled tasks..."
ast-grep --pattern 'class $_ implements Scheduled { $$$ }'

Length of output: 3868


Script:

#!/bin/bash
# Check for GitLab token cleanup in GitLabCIService and related classes
echo "Searching for GitLab token cleanup..."
rg -i "projectAccessToken|GITLAB_TEST_TOKEN" -A 5 -B 5 --type java

# Check for token expiry handling
echo "Searching for token expiry handling..."
rg -i "expiryDate|token.*expir" --type java

Length of output: 28612

src/test/java/de/tum/cit/aet/artemis/core/connector/GitlabRequestMockProvider.java (2)

64-64: LGTM!

The import for ProjectAccessToken is correctly added to support the new mock method.


924-931: LGTM!

The mockCreateProjectAccessToken method follows the class's established pattern for mocking GitLab API calls:

  • Consistent parameter naming and usage
  • Proper error handling with 500 status code
  • Consistent use of argument matchers
  • Returns appropriate mock responses for both success and failure cases
🧰 Tools
🪛 ast-grep

[warning] 925-925: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (new GitLabApiException("Internal Error", 500))
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 928-928: Detected a cookie where the HttpOnly flag is either missing or disabled. The HttpOnly cookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set the HttpOnly flag to true` in all other cases.
Context: (new ProjectAccessToken())
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 925-925: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new GitLabApiException("Internal Error", 500))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration


[warning] 928-928: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new ProjectAccessToken())
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration

Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

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

Tested on TS4. I created a text exercise and a programming exercise and participated in them. The results of the exercises are shown correctly.

Screenshot 2024-10-27 182107
Screenshot 2024-10-27 182138

@krusche krusche added this to the 7.6.4 milestone Oct 27, 2024
@krusche krusche merged commit 3b1b15e into develop Oct 27, 2024
38 of 46 checks passed
@krusche krusche deleted the fix/gitlab-ci branch October 27, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module ready for review server Pull requests that update Java code. (Added Automatically!) template tests
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants