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: Add access tokens for authentication to LocalVC repositories #8929

Merged

Conversation

SimonEntholzer
Copy link
Contributor

@SimonEntholzer SimonEntholzer commented Jul 1, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) 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 added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).

Motivation and Context

We want to allow users to use access tokens to clone/use localVC repositories.

Description

We added another table which stores tokens for every participation.
image

Added database table:
image

How tokens are created:
Case 1: When the student starts the participation, a vcs-access token is created with it. The user then fetches it from the server.
Case 2: When the exercise has started already (e.g. from before this PR), or the token was deleted (for whatever reason), and therefore no token is available, a new token is created when the user tries to fetch it.

A token is now always associated with a user, and a participation.

Steps for Testing

Note: only testable on TS3 and TS4 (only there, the feature is enabled already, and they use localVC)

  1. With a student, access the programming exercise details page, and start the exercise.
  2. Copy the Token clone link.
  3. Make sure the copied link contains a access token, and that cloning/pushing works
  4. Copy the HTTPS clone link, which has no token
  5. Make sure that cloning with this link prompts with a password authentication request, which works when the user's correct password is provided.

Exam Mode Testing

  1. Do the same as above for a programming exercise in an exam

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

Performance Review

  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Test Coverage

e2e tests: https://bamboo.ase.in.tum.de/browse/ARTEMIS-AEPTMA702-4

Screenshots

Drop down menu to choose between URLs with HTTPS, SSH and Token:

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced management of VCS access tokens for user participations.
    • Added methods to retrieve and create VCS access tokens via API endpoints.
    • Enhanced dropdown functionality for selecting authentication methods in the UI, including a new token-based option.
  • Enhancements

    • Improved security and flexibility for user authentication using VCS access tokens.
    • Enhanced participation-related operations with integrated VCS access token management.
    • Streamlined loading of VCS access tokens for user participations.
    • Renamed properties for clearer intent regarding VCS access tokens.
  • Bug Fixes

    • Corrected a grammatical error in a comment within the Exercise Deletion Service.
  • Tests

    • Added new test methods for scenarios involving VCS access tokens, enhancing overall test coverage.

@SimonEntholzer SimonEntholzer requested a review from a team as a code owner July 1, 2024 06:16
@SimonEntholzer SimonEntholzer self-assigned this Jul 1, 2024
@github-actions github-actions bot added server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. labels Jul 1, 2024
Copy link

coderabbitai bot commented Jul 1, 2024

Walkthrough

The updates enhance the management of Version Control System (VCS) access tokens in the Artemis system by renaming the configuration parameter for clarity, introducing new services for streamlined token operations, and improving user interactions with VCS access. Key functionalities now include creating, retrieving, and deleting tokens, along with new options for token-based authentication in the user interface.

Changes

Files Change Summary
.../gitlab/GitLabPersonalAccessTokenManagementService.java, .../gitlab/GitlabInfoContributor.java, .../vcs/VcsTokenRenewalService.java, .../authentication/UserJenkinsGitlabIntegrationTest.java, .../app/shared/components/code-button/code-button.component.ts Renamed versionControlAccessToken to useVersionControlAccessToken and updated related logic across services and tests.
.../ParticipationService.java Integrated ParticipationVCSAccessTokenService for managing VCS access tokens during participation creation and deletion.
.../repository/ParticipationVCSAccessTokenRepository.java, .../service/ParticipationVCSAccessTokenService.java Introduced a repository and service for managing participation VCS access tokens, including methods for creation, retrieval, and deletion operations.
.../ExerciseDeletionService.java Corrected a grammatical error in a comment and removed the deletion of competencies from the delete method.
.../UserService.java Added ParticipationVCSAccessTokenService for fetching and creating VCS access tokens for users.
.../rest/UserResource.java Added endpoints for retrieving and creating VCS access tokens for a user based on a participation ID.
.../app/shared/components/code-button/code-button.component.html Enhanced dropdown to include token-based HTTPS option, restructuring the options for better user interaction.
.../exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html Added participations input property to handle multiple participation records in the exam summary component.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UserResource
    participant ParticipationVCSAccessTokenService
    participant ParticipationVCSAccessTokenRepository

    User ->> UserResource: GET /users/vcsToken?participationId=...
    UserResource ->> ParticipationVCSAccessTokenService: getVcsAccessToken(participationId)
    ParticipationVCSAccessTokenService ->> ParticipationVCSAccessTokenRepository: findByUserIdAndParticipationId
    ParticipationVCSAccessTokenRepository -->> ParticipationVCSAccessTokenService: Optional<ParticipationVCSAccessToken>
    ParticipationVCSAccessTokenService -->> UserResource: Optional<ParticipationVCSAccessToken>
    UserResource -->> User: VCS Access Token

    User ->> UserResource: PUT /users/vcsToken?participationId=...
    UserResource ->> ParticipationVCSAccessTokenService: createVcsAccessToken(participationId)
    ParticipationVCSAccessTokenService ->> ParticipationVCSAccessTokenRepository: createParticipationVCSAccessToken
    ParticipationVCSAccessTokenRepository -->> ParticipationVCSAccessTokenService: ParticipationVCSAccessToken
    ParticipationVCSAccessTokenService -->> UserResource: ParticipationVCSAccessToken
    UserResource -->> User: Created VCS Access Token
Loading

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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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 as 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.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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.

@SimonEntholzer SimonEntholzer added ready for review and removed client Pull requests that update TypeScript code. (Added Automatically!) labels Jul 1, 2024
@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Jul 1, 2024
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: 11

Outside diff range comments (13)
src/main/webapp/app/shared/layouts/profiles/profile.service.ts (5)

Line range hint 99-99: Use strict equality check.

Use !== instead of != to avoid type coercion issues.

- return this.profileInfo.pipe(filter((x) => x != undefined) as OperatorFunction<ProfileInfo | undefined, ProfileInfo>);
+ return this.profileInfo.pipe(filter((x) => x !== undefined) as OperatorFunction<ProfileInfo | undefined, ProfileInfo>);
Tools
Biome

[error] 32-32: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)


Line range hint 102-102: Avoid using any type.

Specify a different type instead of using any.

- private mapAllowedOrionVersions(data: any, profileInfo: ProfileInfo) {
+ private mapAllowedOrionVersions(data: Record<string, unknown>, profileInfo: ProfileInfo) {
Tools
Biome

[error] 32-32: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)


Line range hint 106-106: Avoid using any type.

Specify a different type instead of using any.

- private mapTestServer(data: any, profileInfo: ProfileInfo) {
+ private mapTestServer(data: Record<string, unknown>, profileInfo: ProfileInfo) {
Tools
Biome

[error] 32-32: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)


Line range hint 110-110: Avoid using any type.

Specify a different type instead of using any.

- private mapGuidedTourConfig(data: any, profileInfo: ProfileInfo) {
+ private mapGuidedTourConfig(data: Record<string, unknown>, profileInfo: ProfileInfo) {
Tools
Biome

[error] 32-32: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)


Line range hint 119-119: Avoid using any type.

Specify a different type instead of using any.

- private static mapSaml2Config(data: any, profileInfo: ProfileInfo) {
+ private static mapSaml2Config(data: Record<string, unknown>, profileInfo: ProfileInfo) {
Tools
Biome

[error] 32-32: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)

src/main/webapp/app/core/auth/account.service.ts (7)

Line range hint 18-18: Avoid using any type.

Specify a different type instead of using any.

- save: (account: any) => Observable<HttpResponse<any>>;
+ save: (account: User) => Observable<HttpResponse<User>>;

Line range hint 81-81: Avoid using any type.

Specify a different type instead of using any.

- save(user: User): Observable<HttpResponse<any>> {
+ save(user: User): Observable<HttpResponse<User>> {

Line range hint 90-90: Avoid using non-null assertion operator.

Using non-null assertion operator (!) is forbidden. Consider using optional chaining instead.

- this.userIdentity!.groups = groups;
+ this.userIdentity?.groups = groups;

Line range hint 118-119: Avoid using non-null assertion operator.

Using non-null assertion operator (!) is forbidden. Consider using optional chaining instead.

- const authorities = id!.authorities!;
+ const authorities = id?.authorities ?? [];

Line range hint 160-160: Avoid using non-null assertion operator.

Using non-null assertion operator (!) is forbidden. Consider using optional chaining instead.

- const langKey = this.userIdentity.langKey || this.sessionStorage.retrieve('locale');
+ const langKey = this.userIdentity?.langKey || this.sessionStorage.retrieve('locale');

Line range hint 165-165: Avoid using non-null assertion operator.

Using non-null assertion operator (!) is forbidden. Consider using optional chaining instead.

- this.translateService.use(langKey!);
+ this.translateService.use(langKey);

Line range hint 287-289: Omit the else clause.

This else clause can be omitted because previous branches break early.

- } else {
-     this.userIdentity = undefined;
- }
+ this.userIdentity = undefined;
src/main/java/de/tum/in/www1/artemis/web/rest/ExerciseResource.java (1)

Line range hint 261-262: Address the TODO comments.

The TODO comments indicate that the current implementation retrieves the first token for the user. Ensure that the correct token for the participation is retrieved.

Do you want me to help address these TODO comments and implement the correct retrieval logic?

@dfuchss
Copy link
Contributor

dfuchss commented Jul 1, 2024

Why don't attach the VCS to the user and instead attaching it to the participations?

Many Credential managers for git typically save one (!) password per domain. Thus it will be hard to clone the repositories (i.e., solution, test, ...) for instructors.

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: 1

src/main/webapp/app/core/auth/account.service.ts Outdated Show resolved Hide resolved
@coderabbitai coderabbitai bot mentioned this pull request Oct 31, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) config-change Pull requests that change the config in a way that they require a deployment via Ansible. database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.