Integrated code lifecycle: Disable access to personal VCS access tokens in account settings for students

[SimonEntholzer]

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• This is a small issue that I tested locally and was confirmed by another developer on a test server.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• I chose a title conforming to the naming conventions for pull requests.

Client

• Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
• I strictly followed the client coding and design guidelines.
• Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
• I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
• I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
• I documented the TypeScript code using JSDoc style.
• I added multiple screenshots/screencasts of my UI changes.
• I translated all newly inserted strings into English and German.

Motivation and Context

The newly introduced personal access tokens, could confuse students, as they do not really need them, as they can only access their own repositories.

Description

Only enable VCS access token creation for users which are at least tutors.

Steps for Testing

Prerequisites:

1. Log in with an account which is tutor, instructor, admin or editor, and check in the settings if you are still able to view the VCS access token settings.
2. Log in with a student account and verify you do not see the VCS access token settings.

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

Test Coverage

Screenshots

As privileged user:

Users who is only a student:

Summary by CodeRabbit

• New Features
  • Conditional rendering of the "VCS Access Token Settings" link based on user role and permissions.
  • Added a new method to check if the user has at least tutor-level authority.
• Bug Fixes
  • Improved authorization logic for displaying user settings based on user roles.
• Refactor
  • Updated the UserSettingsContainerComponent to utilize Angular's standalone component feature.
• Tests
  • Updated test configurations and scenarios for the UserSettingsContainerComponent.

[coderabbitai]

Walkthrough

The changes involve modifications to the UserSettingsContainerComponent in an Angular application, focusing on conditional rendering based on user roles and the adoption of standalone component features. The link to "VCS Access Token Settings" is now conditionally displayed based on user role checks. Additionally, the component's structure has been updated to streamline dependency injection and improve code organization. The UserSettingsModule has removed the component from its declarations, and corresponding test cases have been adjusted to reflect these changes.

Changes

File	Change Summary
src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html	Conditional rendering of "VCS Access Token Settings" link modified to include isAtLeastTutor check.
src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts	Utilized standalone component feature, removed constructor, added isAtLeastTutor property, and integrated user role logic into ngOnInit.
src/main/webapp/app/shared/user-settings/user-settings.module.ts	Removed UserSettingsContainerComponent from the module's declarations.
src/test/javascript/spec/component/account/user-settings-container.component.spec.ts	Adjusted test setup, removed TranslatePipeMock, and modified profile service mock behavior.
src/main/webapp/app/core/auth/account.service.ts	Added isAtLeastTutor() method to check user roles against predefined permissions.

Possibly related PRs

• Integrated code lifecycle: Add access log for repositories #9184: The changes in the main PR involve conditional rendering based on user roles, which relates to the access logging functionality introduced in this PR, as both deal with user permissions and access to specific features.
• General: Improve tab navigation on login page #9373: The main PR's updates to user settings and role-based access are relevant to the improvements in tab navigation and user experience on the login page, as both aim to enhance user interaction based on roles and permissions.

Suggested labels

component:Programming, bugfix, priority:high

Suggested reviewers

• krusche
• pzdr7
• JohannesWt
• eceeeren
• florian-glombik

---

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

• X
• Mastodon
• Reddit
• LinkedIn

🪧 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.

[coderabbitai]

Actionable comments posted: 4

🧹 Outside diff range comments (2)

src/test/javascript/spec/component/account/user-settings-container.component.spec.ts (1)

Line range hint 45-59: LGTM: Comprehensive test cases added

The new test cases effectively cover the scenarios with and without the localVC profile, aligning with the PR objectives. The use of toBeTrue() and toBeFalse() matchers is consistent with the coding guidelines.

For consistency with other tests and improved readability, consider using async/await syntax in both test cases:

it('should initialize with localVC profile', async () => {
  await comp.ngOnInit();
  expect(profileServiceMock.getProfileInfo).toHaveBeenCalled();
  expect(comp.localVCEnabled).toBeTrue();
});

it('should initialize with no localVC profile set', async () => {
  profileServiceMock.getProfileInfo.mockReturnValue(of({ activeProfiles: [] }));
  await comp.ngOnInit();
  expect(profileServiceMock.getProfileInfo).toHaveBeenCalled();
  expect(comp.localVCEnabled).toBeFalse();
});

This ensures consistent handling of asynchronous operations across all tests.

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html (1)

Line range hint 1-62: *Consider updating older ngIf syntax in future refactoring.

While not directly related to the current changes, I noticed that other parts of the file are still using the older *ngIf syntax. For consistency and to align with the latest Angular best practices, consider updating these instances to use the @if syntax in a future refactoring task.

This would involve changing occurrences like:

<div *ngIf="someCondition">

to:

@if (someCondition) {
  <div>
}

This suggestion is purely for future consideration and doesn't affect the current PR's objectives.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 4932dc9 and ebcad87.

📒 Files selected for processing (4)

• src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html (1 hunks)
• src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (2 hunks)
• src/main/webapp/app/shared/user-settings/user-settings.module.ts (0 hunks)
• src/test/javascript/spec/component/account/user-settings-container.component.spec.ts (2 hunks)

💤 Files with no reviewable changes (1)

• src/main/webapp/app/shared/user-settings/user-settings.module.ts

🧰 Additional context used

📓 Path-based instructions (3)

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html (1)

Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/test/javascript/spec/component/account/user-settings-container.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

🔇 Additional comments (6)

src/test/javascript/spec/component/account/user-settings-container.component.spec.ts (2)

7-7: LGTM: Appropriate mock service import added

The addition of MockTranslateService is consistent with the test setup and aligns with the guideline to mock irrelevant dependencies.

---

Line range hint 1-59: Overall assessment: Changes align with PR objectives and improve test quality

The modifications to this test file effectively support the PR objective of handling different user roles (with and without localVC profile). The changes improve test coverage, clarity, and adhere to coding guidelines. The suggestions provided are minor and aimed at further enhancing consistency and readability.

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.html (1)

34-42: LGTM! Changes align well with PR objectives.

The modifications effectively implement the requirement to restrict access to VCS Access Token Settings for students. By adding the isAtLeastTutor condition, the link is now only visible to users with tutor or higher roles, which directly addresses the PR's main objective.

The implementation:

1. Uses the correct @if syntax as per the coding guidelines.
2. Maintains proper indentation and structure.
3. Correctly uses the translation key for internationalization.

These changes should successfully prevent confusion among students regarding personal access tokens while maintaining access for appropriate user roles.

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (3)

8-10: Imports are correctly added for standalone component

The imports for TranslateDirective, RouterModule, and FontAwesomeModule are appropriately included, and they are necessary for the component functionality.

---

18-20: Component metadata correctly updated for standalone component

The standalone: true property and the imports array are properly configured, enabling the component to function as a standalone component. This aligns with Angular's best practices for standalone components.

---

31-31: Property isAtLeastTutor is appropriately initialized

The property isAtLeastTutor is correctly initialized to false, preparing it for use in access control logic.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between ebcad87 and 4f9ef32.

📒 Files selected for processing (1)

• src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

📓 Learnings (1)

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (1)

Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9397
File: src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts:1-1
Timestamp: 2024-10-01T13:58:26.805Z
Learning: In this project, refrain from suggesting the replacement of the `inject()` function with constructor injection in components.

🔇 Additional comments (3)

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (3)

18-20: Component correctly converted to standalone with proper imports

The component is appropriately marked as standalone, and the necessary modules are correctly imported within the imports array.

---

23-24: Dependencies are correctly injected using inject()

The services ProfileService and AccountService are properly injected using the inject() function, adhering to the project's guidelines.

---

49-51: Previous suggestion on role checking logic remains applicable

The earlier review comment regarding the role checking logic in authorizeTutor() is still valid. Refactoring the logic to explicitly check for tutor and higher roles would ensure accurate access control.

[b-fein]

Should the relevant server endpoint(s) also be changed to be accessible only to tutors and above?

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 4f9ef32 and 75a0c6c.

📒 Files selected for processing (2)

• src/main/webapp/app/core/auth/account.service.ts (1 hunks)
• src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

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

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

📓 Learnings (1)

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (1)

Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9397
File: src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts:1-1
Timestamp: 2024-10-01T13:58:26.805Z
Learning: In this project, refrain from suggesting the replacement of the `inject()` function with constructor injection in components.

🔇 Additional comments (7)

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

227-229: Verify complete implementation of PR objectives

The isAtLeastTutor() method provides a good foundation for implementing the PR objective of restricting access to VCS tokens for students. However, this method alone doesn't fully implement the described functionality.

To ensure the complete implementation of the PR objectives, please run the following script to check for usage of this method in relation to VCS token access:

This script will help verify if the isAtLeastTutor() method is being used to restrict access to VCS tokens as intended in the PR objectives.

src/main/webapp/app/shared/user-settings/user-settings-container/user-settings-container.component.ts (6)

1-1: Import of 'inject' function is appropriate

Including the inject function from @angular/core aligns with the usage of dependency injection in this component.

---

8-10: Necessary module imports added for standalone component

The imports of TranslateDirective, RouterModule, and FontAwesomeModule are correctly included to support the standalone component functionality.

---

18-20: Standalone component configuration is correctly applied

Setting standalone: true and specifying the imports array in the component decorator is appropriate and follows Angular's guidelines for standalone components.

---

23-24: Dependency injection using 'inject()' function is properly implemented

Using the inject() function to obtain instances of ProfileService and AccountService is suitable for this context.

---

31-31: Initialization of 'isAtLeastTutor' property

Initializing isAtLeastTutor to false ensures a defined state before any user authentication logic is executed.

---

40-45: Authentication state handling and role assignment are correctly implemented

The use of the tap operator to assign currentUser and determine isAtLeastTutor using this.accountService.isAtLeastTutor() is appropriate and enhances code readability.

[chrisknedl]

Tested on TS4. Works as expected!

[JohannesStoehr]

Code

[iyannsch]

Code LGTM

[chrisknedl]

Just a note about security: As far as I know, when students (or any user) create their own tokens, they can set an expiration date, which is not possible on the participation tokens. So when a participation token gets leaked, everyone who has access to the token can access the submission as long as the exercise exists.

I agree with hiding the tokens from the students, but maybe then there should be a feature to control the expiration date of the participation tokens to prevent such cases.

[cremertim]

Tested in testing session, works as expected

[dmytropolityka]

code

[SimonEntholzer]

Just a note about security: As far as I know, when students (or any user) create their own tokens, they can set an expiration date, which is not possible on the participation tokens. So when a participation token gets leaked, everyone who has access to the token can access the submission as long as the exercise exists.

I agree with hiding the tokens from the students, but maybe then there should be a feature to control the expiration date of the participation tokens to prevent such cases.

That's a good point, I'll keep that in mind for a follow up 👍

[krusche]

Just a note about security: As far as I know, when students (or any user) create their own tokens, they can set an expiration date, which is not possible on the participation tokens. So when a participation token gets leaked, everyone who has access to the token can access the submission as long as the exercise exists.
I agree with hiding the tokens from the students, but maybe then there should be a feature to control the expiration date of the participation tokens to prevent such cases.

That's a good point, I'll keep that in mind for a follow up 👍

I would not implement this. The idea of an access token per participation is that the scope is really tiny. If a student really accidentally leaks such a token, only the affected exercise could be manipulated, nothing else. An exercise is typically open for a short amount of time when it's graded. For practice exercises, there is almost no harm, as they are not graded at all.

When we add an expiration date for graded exercises that is shorter than the due date, it could be the case that students suddenly get permission errors (in the worst case shortly before the deadline) without understanding what's going. Getting a new token and using it in the respective context would be quite cumbersome and some students might not even understand the issue.