Programming exercises: Fix access tokens not showing up in repository view and student view
#9736
Conversation
github-actions
bot
added
tests
server
SimonEntholzer
changed the title
Programming Exercises: Fix access tokens not showing up in repository view and student viewProgramming exercises: Fix access tokens not showing up in repository view and student view
WalkthroughThe pull request introduces modifications primarily to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmdsrc/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
src/main/webapp/app/shared/components/code-button/code-button.component.html (1)
Line range hint
1-116: Well-structured template with proper separation of concerns.The template successfully implements the token visibility fixes while maintaining:
- Consistent use of new Angular
@ifsyntax- Clear separation between different functional sections
- Proper security practices for external links
- Flexible handling of different repository types and access methods
Consider adding automated tests to verify the token visibility logic across different user scenarios.
src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java (1)
Line range hint
236-242: Fix inconsistent documentationThe endpoint documentation states it "adds" a token, but the parameter description still mentions that the token should be "fetched". This inconsistency should be fixed.
Apply this diff to fix the documentation:
/** * PUT account/participation-vcs-access-token : add a vcsToken for of a user for a participation * - * @param participationId the participation for which the access token should be fetched + * @param participationId the participation for which the access token should be created * * @return the versionControlAccessToken belonging to the provided participation and user */src/main/webapp/app/shared/components/code-button/code-button.component.ts (2)
126-126: Consider optimizing duplicate loadParticipationVcsAccessTokens callsThe method is called in both
ngOnInitandngOnChanges, which could lead to duplicate API calls. Consider adding a check inngOnChangesto only callloadParticipationVcsAccessTokenswhen participations have actually changed.ngOnChanges() { + const hasParticipationsChanged = this.participations?.length !== undefined; if (this.participations?.length) { const shouldPreferPractice = this.participationService.shouldPreferPractice(this.exercise); this.activeParticipation = this.participationService.getSpecificStudentParticipation(this.participations, shouldPreferPractice) ?? this.participations[0]; this.isPracticeMode = isPracticeMode(this.activeParticipation); this.cloneHeadline = this.isPracticeMode && !this.exercise?.exerciseGroup ? 'artemisApp.exerciseActions.clonePracticeRepository' : 'artemisApp.exerciseActions.cloneRatedRepository'; this.isTeamParticipation = !!this.activeParticipation?.team; + if (hasParticipationsChanged) { this.loadParticipationVcsAccessTokens(); + } } else if (this.repositoryUri) { this.cloneHeadline = 'artemisApp.exerciseActions.cloneExerciseRepository'; } - this.loadParticipationVcsAccessTokens(); }
236-238: Add user feedback for unauthorized accessWhile the error handling is correct, consider notifying the user when they don't have permission to access the VCS token.
if (error.status == 403) { this.useParticipationVcsAccessToken = false; + // Use the translation service for i18n + const message = this.translateService.instant('artemisApp.exerciseActions.vcsTokenUnauthorized'); + // Assuming you have a notification service + this.notificationService.showError(message); }src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.java (1)
835-835: LGTM! Consider adding Javadoc param annotations.The change to use the
Userobject directly instead of just the user ID improves code clarity. The implementation correctly delegates to the specialized service.Add
@throwsannotation to document the exception case:/** * Get the vcs access token associated with a user and a participation * * @param user the user associated with the vcs access token * @param participationId the participation's participationId associated with the vcs access token + * @throws de.tum.cit.aet.artemis.core.exception.EntityNotFoundException if no token exists for the given user and participation * @return the users participation vcs access token, or throws an exception if it does not exist */src/main/java/de/tum/cit/aet/artemis/programming/service/ParticipationVcsAccessTokenService.java (5)
50-50: Clarify JavaDoc description for readabilityConsider rephrasing the method description for improved clarity. Suggested wording:
Retrieves the ParticipationVCSAccessToken for a User and Participation pair if it exists and the user owns the participation.
52-52: Correct grammatical inconsistency in@paramdescriptionPlease change "the user which is owner of the token" to "the user who owns the token" for grammatical correctness.
56-63: Reevaluate the necessity of ownership checksAccording to previous learnings, the VCS access token is bound to both the user and the participation, ensuring that only the owning user can access the token. Therefore, the additional ownership check may be redundant and could be removed to simplify the code.
Let me know if you'd like assistance in refactoring this method to remove the redundant check.
67-67: Improve JavaDoc description for clarityConsider rephrasing the method description to enhance understanding. Suggested revision:
Checks if a ParticipationVCSAccessToken exists for a User and Participation pair, and creates a new one if not, provided the user owns the participation.
76-81: Reevaluate the necessity of ownership checksSimilar to the previous method, the additional check for participation ownership may be unnecessary due to the existing binding between the user and the ParticipationVCSAccessToken. Removing the redundant check could streamline the code.
Would you like assistance in refactoring this method to eliminate the redundant ownership check?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ParticipationVcsAccessTokenService.java(3 hunks)src/main/webapp/app/localvc/repository-view/repository-view.component.html(1 hunks)src/main/webapp/app/shared/components/code-button/code-button.component.html(1 hunks)src/main/webapp/app/shared/components/code-button/code-button.component.ts(4 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCITestService.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/main/java/de/tum/cit/aet/artemis/core/service/user/UserService.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/core/web/AccountResource.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/ParticipationVcsAccessTokenService.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/webapp/app/localvc/repository-view/repository-view.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/components/code-button/code-button.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/components/code-button/code-button.component.ts (1)
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCIntegrationTest.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/icl/LocalVCLocalCITestService.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
📓 Learnings (2)
src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java (2)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#8929
File: src/main/java/de/tum/in/www1/artemis/web/rest/UserResource.java:229-243
Timestamp: 2024-07-12T20:42:10.924Z
Learning: In the Artemis project, the VCS access token is bound to both the user and the participation, ensuring that only the owning user can access the token. Therefore, additional authorization checks in the `getVcsAccessToken` method are unnecessary.
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#8929
File: src/main/java/de/tum/in/www1/artemis/web/rest/UserResource.java:229-243
Timestamp: 2024-10-08T15:35:42.972Z
Learning: In the Artemis project, the VCS access token is bound to both the user and the participation, ensuring that only the owning user can access the token. Therefore, additional authorization checks in the `getVcsAccessToken` method are unnecessary.
src/main/java/de/tum/cit/aet/artemis/programming/service/ParticipationVcsAccessTokenService.java (2)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#8929
File: src/main/java/de/tum/in/www1/artemis/web/rest/UserResource.java:229-243
Timestamp: 2024-07-12T20:42:10.924Z
Learning: In the Artemis project, the VCS access token is bound to both the user and the participation, ensuring that only the owning user can access the token. Therefore, additional authorization checks in the `getVcsAccessToken` method are unnecessary.
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#8929
File: src/main/java/de/tum/in/www1/artemis/web/rest/UserResource.java:229-243
Timestamp: 2024-10-08T15:35:42.972Z
Learning: In the Artemis project, the VCS access token is bound to both the user and the participation, ensuring that only the owning user can access the token. Therefore, additional authorization checks in the `getVcsAccessToken` method are unnecessary.
🔇 Additional comments (12)
src/main/webapp/app/localvc/repository-view/repository-view.component.html (1)
50-51: LGTM! Consider simplifying the participations input.
The changes correctly implement the access token functionality by enabling participation VCS access tokens. This addresses the issue where tokens weren't showing up in the repository view.
Let's verify if the array syntax for participations is necessary:
src/main/webapp/app/shared/components/code-button/code-button.component.html (4)
20-24: LGTM! Clean token warning implementation.
The token warning conditions are properly implemented using the new @if syntax and have been simplified to focus on the essential token status checks.
Line range hint 72-89: LGTM! Well-structured repository action buttons.
The implementation properly handles different repository types and follows security best practices with noopener for external links. The conditional rendering using @if/@else is correctly implemented.
Line range hint 41-43: Verify token option visibility conditions.
The condition for showing the token option has been updated to handle both general and participation-specific VCS access tokens. This aligns with the PR objective of fixing token visibility in repository views.
#!/bin/bash
# Search for related token visibility conditions
rg -A 5 'useParticipationVcsAccessToken|accessTokensEnabled' 'src/main/webapp'Line range hint 25-31: Verify mdbCheckbox directive usage across the application.
While the practice mode switch implementation is correct, let's ensure consistent usage of the mdbCheckbox directive across similar toggle components.
✅ Verification successful
The provided output seems to be for a different search pattern. Let me search specifically for mdbCheckbox usage.
The mdbCheckbox directive usage is consistent across practice mode switches
The verification shows that mdbCheckbox is used consistently in both instances where practice mode switches appear:
- In
code-button.component.html - In
open-code-editor-button.component.html
Both implementations follow the same pattern with identical structure using form-check-input class, proper label association, and consistent event bindings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of mdbCheckbox to verify consistency
rg -A 2 'mdbCheckbox' 'src/main/webapp'
Length of output: 8443
Script:
#!/bin/bash
# Search for mdbCheckbox directive usage
rg -A 2 'mdbCheckbox' 'src/main/webapp'
Length of output: 1214
src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java (1)
Line range hint 243-249: LGTM! Verify token creation in UserService
The implementation follows best practices:
- Uses proper authorization with
@EnforceAtLeastStudent - Follows REST principles with PUT method
- Includes appropriate logging
- Correctly delegates token creation to UserService
Let's verify the token creation implementation in UserService:
✅ Verification successful
LGTM! Token creation implementation is secure and follows best practices
The token creation implementation in ParticipationVcsAccessTokenService is well-designed and secure:
- Properly validates user ownership of participation before creating token
- Uses secure token generation via
LocalVCPersonalAccessTokenManagementService.generateSecureVCSAccessToken() - Includes appropriate error handling for duplicate tokens and unauthorized access
- Follows repository pattern for persistence
- Includes proper logging
The implementation correctly handles:
- Authorization checks
- Token uniqueness
- Secure random token generation
- Data persistence
- Error cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the token creation implementation in UserService
# Test: Search for the token creation method implementation
ast-grep --pattern 'createParticipationVcsAccessTokenForUserAndParticipationIdOrElseThrow(User $_, Long $_) {
$$$
}'
# Test: Check for proper error handling
rg -A 5 'createParticipationVcsAccessTokenForUserAndParticipationIdOrElseThrow.*throws'
Length of output: 210
Script:
#!/bin/bash
# Let's try a different approach to find the token creation method
# Search for the method name in Java files
rg -t java "createParticipationVcsAccessTokenForUserAndParticipationIdOrElseThrow" -A 10
# Also search for related VCS token creation methods
rg -t java "VcsAccessToken" -A 5
# Look for UserService class implementation
fd "UserService.java" --exec cat {}
Length of output: 138644
Script:
#!/bin/bash
# Let's verify the token creation implementation in ParticipationVcsAccessTokenService
# Search for the token creation method implementation
rg -t java "createVcsAccessTokenForUserAndParticipationIdOrElseThrow" -A 15
# Look for token generation implementation
rg -t java "generateSecureVCSAccessToken" -A 5
Length of output: 6916
src/main/webapp/app/shared/components/code-button/code-button.component.ts (3)
162-162: LGTM! Improved condition readability
The simplified condition for copyEnabled is more maintainable as useToken is always true in this context.
256-260: Add user feedback for unauthorized access
Similar to the previous comment on loadVcsAccessToken, consider adding user feedback here as well.
236-238: Verify VCS token error handling across the application
Let's verify that VCS token error handling is consistent across the application.
Also applies to: 256-260
✅ Verification successful
Let me try a different search strategy to better understand the VCS token handling across the codebase.
Let me do one final check to understand the complete context of the VCS token usage in the code-button component.
VCS token error handling is correctly implemented
The code correctly handles 403 errors for VCS token access by setting useParticipationVcsAccessToken = false in two scenarios:
- When loading an existing VCS token fails with 403
- When creating a new VCS token fails with 403
This is the appropriate behavior as it prevents further attempts to use invalid tokens and falls back to alternative authentication methods (HTTPS without token or SSH). The implementation is consistent with the component's design which provides multiple authentication options through its UI.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for VCS token error handling patterns
echo "Searching for VCS token error handling..."
rg -A 5 "error.status.*403.*vcs.*token" || echo "No direct matches found"
# Search for similar components that might need the same error handling
echo "Searching for similar components..."
ast-grep --pattern 'class $_ implements OnInit {
$$$
useParticipationVcsAccessToken = $_;
$$$
}'
Length of output: 501
Script:
#!/bin/bash
# Search for VCS token related code more broadly
echo "Searching for VCS token usage..."
rg -l "useParticipationVcsAccessToken"
echo -e "\nSearching for 403 error handling..."
rg -A 5 "error\.status.*403"
echo -e "\nSearching for VCS token class properties..."
ast-grep --pattern 'useParticipationVcsAccessToken = $_'
Length of output: 6849
Script:
#!/bin/bash
# Get the complete context of code-button component
echo "Checking code-button component implementation..."
cat "src/main/webapp/app/shared/components/code-button/code-button.component.ts"
echo -e "\nChecking code-button component template..."
cat "src/main/webapp/app/shared/components/code-button/code-button.component.html"
Length of output: 21321
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCIntegrationTest.java (2)
113-113: LGTM! Change aligns with PR objectives.
The update to use the complete student object instead of just the ID improves code clarity and matches the service layer changes for better token handling.
Line range hint 112-141: Test method comprehensively verifies token functionality.
The test method testFetchPush_usingVcsAccessToken thoroughly validates the VCS access token behavior by:
- Testing successful fetch/push with participation token
- Testing successful fetch/push with user token
- Verifying token invalidation scenarios
- Checking behavior with removed participation
This comprehensive coverage aligns well with the PR's goal of fixing token display issues.
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCITestService.java (1)
679-685: LGTM! Improved type safety with User object parameter.
The change from using a userId to a User object parameter enhances type safety and makes the method signature more explicit about its requirements. The implementation correctly delegates to the service layer method.
HawKhiem
added
deploy:artemis-test2
deploy:artemis-test6
and removed
deploy:artemis-test2
labels
HawKhiem
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
On some test servers the token is disabled, TS6 is one of them, as it uses GitlabCI, and not the LocalVC/CI setup. At least on Ts1 - Ts4 it's enabled |
...main/java/de/tum/cit/aet/artemis/programming/service/ParticipationVcsAccessTokenService.java
Show resolved
Hide resolved
vinceclifford
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
On TS6 the token feature is disabled, therefore no tokens are visible. That's not related to this PR :) |
krusche
deleted the
bugfix/repository-view/fix-code-button-token-usage
branch
Checklist
General
Server
Client
Changes affecting Programming Exercises
Motivation and Context
The token in the clone link is not correctly shown in the repository view.
And the bug tokens are generally not shown any more.
Description
Fixes #9380
Steps for Testing
Prerequisites:
a) there is no token, if the account has no personal access token set:
b) there is a token, if the account has a personal access token
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
Manual Tests
Test Coverage
Screenshots
Before the fix:
After the fix:
Summary by CodeRabbit
Release Notes
New Features
Userobjects for token retrieval and creation.Bug Fixes
Improvements