- 
                Notifications
    You must be signed in to change notification settings 
- Fork 90
refactor: use GraphQL-only for GitLab permission verification with comprehensive test coverage #905
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
Conversation
…up access are null Co-authored-by: fgreinacher <[email protected]>
| @copilot Use the GitLab GraphQL API, like in #855 | 
…of REST endpoint testing Co-authored-by: fgreinacher <[email protected]>
| Implemented the GraphQL approach as suggested! I've replaced the REST API endpoint testing with GitLab GraphQL API permission queries, following the pattern from PR #855. The solution now uses a GraphQL query to check  Changes made in commit 118d29b: 
 All 140 tests pass including 4 new GraphQL-specific test cases. | 
Co-authored-by: fgreinacher <[email protected]>
Co-authored-by: fgreinacher <[email protected]>
Co-authored-by: fgreinacher <[email protected]>
| 
 @copilot Do it | 
Co-authored-by: fgreinacher <[email protected]>
| 
 Removed all 6 obsolete REST API permission tests in commit 0168cb9: ✅ Removed Tests: 
 These tests verified REST API  | 
…issions Co-authored-by: fgreinacher <[email protected]>
        
          
                test/verify.test.js
              
                Outdated
          
        
      | data: { | ||
| project: { | ||
| userPermissions: { | ||
| pushToRepository: true, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot For this test case pushToRepository should be false, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely correct! Fixed in commit e3775a2. The dry run positive test now uses pushToRepository: false and readRepository: true, correctly demonstrating that dry run mode only requires read permissions and doesn't check push permissions (as per the logic: if (dryRun && !readRepository) vs else if (!pushToRepository)).
        
          
                test/verify.test.js
              
                Outdated
          
        
      | data: { | ||
| project: { | ||
| userPermissions: { | ||
| pushToRepository: true, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot This is not realistic. Make it false as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit f6388c1. Changed the dry run test to use pushToRepository: false, readRepository: false which is more realistic - someone with only read permissions wouldn't have push permissions either.
        
          
                test/verify.test.js
              
                Outdated
          
        
      | ); | ||
|  | ||
| test.serial( | ||
| "Throw SemanticReleaseError when GraphQL returns insufficient permissions", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Name this "Throw SemanticReleaseError when GraphQL returns only read permissions" and add "Throw SemanticReleaseError when GraphQL returns neither read not write permissions"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented both changes in commit f6388c1:
- Renamed test to "Throw SemanticReleaseError when GraphQL returns only read permissions"
- Added new test "Throw SemanticReleaseError when GraphQL returns neither read nor write permissions" with pushToRepository: false, readRepository: false
This provides comprehensive coverage of all permission scenarios.
Co-authored-by: fgreinacher <[email protected]>
Co-authored-by: fgreinacher <[email protected]>
| Not needed anymore: #165 (comment) | 
Fix GitLab Token Permission Verification for Group Access Tokens
Problem: The current permission verification logic fails when using Group Access Tokens that have permissions through group membership (
shared_with_groups). The GitLab API returnsnullfor bothproject_accessandgroup_accessin the permissions object, even when the token has valid permissions through group sharing.Solution: Implemented a GraphQL-only permission verification approach as suggested by @fgreinacher and completely cleaned up obsolete test cases.
Changes Made:
lib/verify.jsgitlabGraphQlApiUrlconfiguration toresolve-config.jsuserPermissions.pushToRepositoryandreadRepositoryfieldsgitlabGraphQlApiUrlfieldshared_with_groupsfields from test mocks since they're no longer usedpermissionsobjects from REST API mocks since they're not read by the codepushToRepository: falsedemonstrating that push permissions are not required in dry run modeImplementation Details:
The solution now uses a simplified approach:
userPermissions.pushToRepositoryandreadRepositoryThis approach is cleaner, more consistent, and handles all permission scenarios (direct, group, or shared) through a single GraphQL interface.
Test Coverage:
pushToRepository: true, readRepository: truepushToRepository: false, readRepository: true(demonstrates dry run only needs read permissions)pushToRepository: false, readRepository: true(fails in normal mode)pushToRepository: false, readRepository: false(fails in all modes)pushToRepository: false, readRepository: false(realistic scenario, fails in dry run)Obsolete Tests Completely Removed ✅:
Verify token and repository access (project_access 30)- tested specific REST API level 30Verify token and repository access (project_access 40)- tested specific REST API level 40Verify token and repository access (group_access 30)- tested specific REST API level 30Verify token and repository access (group_access 40)- tested specific REST API level 40Throw SemanticReleaseError if token doesn't have the push permission on the repository- tested insufficient REST levels 10/20Throw SemanticReleaseError if token doesn't have the pull permission on the repository- tested insufficient REST levels 5/5Files Modified:
lib/verify.js- Simplified to GraphQL-only permission verificationtest/verify.test.js- Updated test cases, removed all irrelevant data and obsolete tests, added comprehensive and realistic GraphQL permission test coveragelib/resolve-config.js- Added GraphQL API URL configurationtest/resolve-config.test.js- Updated tests to include new config fieldThis approach addresses the GitLab API inconsistency mentioned in issues #512930 and #22316 by using GraphQL exclusively for permission verification.
Fixes #165.
Addresses feedback from @fgreinacher in comments #2354683096, #2354739456, #2354789276, #2354805502, #2354806795, #2354807577, #3302009733, #2354826562, #2354829882, #3302093473, #2354913104, #2354938408, #2354940970, and #2354944295.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.