fix: checkUserPushPermission push failure on name#1110
fix: checkUserPushPermission push failure on name#1110andypols wants to merge 6 commits intofinos:mainfrom
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1110 +/- ##
==========================================
- Coverage 77.40% 75.33% -2.07%
==========================================
Files 56 58 +2
Lines 2288 2368 +80
Branches 258 271 +13
==========================================
+ Hits 1771 1784 +13
- Misses 487 554 +67
Partials 30 30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Again I think this PR is a partial solution to the issues addressed in #1043 - it'll resolve a problem in the proxy but not the API or UI. I think it also needs to be adjusted to use repo URLs that include .git (as per comments on #1109) - #1043 validates its presence in the UI form, but should perhaps also do so through the API... That said I'm ntt enitrely sure if its required in the git protocol or just a convention.
The tests for the DB client code (which mock the underlying mongo/neDB libs) are worth having as they test smaller units. I'd prefer to have those going in on top of #1043 (as I've been maintaining it it for quite a while).
I agree that the DB tests could be/should be refactored to not carry over state - worse the cypress tests rely on state from the mocha tests which also needed fixing.
kriswest
left a comment
kriswest
left a comment
There was a problem hiding this comment.
Needs to be rebased and simplified now that we retrieve both repos and users differently, hopefully eliminating the original issues. Test changes may still be valuable.
andypols
force-pushed
the
check-push-with-url
branch
from
ffcf17b to
4a0fe55
Compare
# Conflicts: # src/db/file/index.ts # src/db/file/repo.ts # src/db/index.ts # src/db/mongo/index.ts # src/db/mongo/repo.ts # src/proxy/processors/push-action/checkRepoInAuthorisedList.ts # src/proxy/processors/push-action/checkUserPushPermission.ts # test/processors/checkUserPushPermission.test.js # test/testCheckRepoInAuthList.test.js
|
Checking this - it no longer adds any value not that #1109 has been merged, co closing it |
3 participants
Summary
This PR builds on #1109 to address a bug in
checkUserPushPermission. The bug caused permission checks to fail when the repository could not be found by name. It also incorrectly assumed that repository names are globally unique, which prevented users from forking the same repository into different projects/organisations within the same git-proxy platform.This fix is split into a separate PR to keep the scope focused and to unblock the security team, who are currently running penetration tests using GitLab.
Fix
This PR:
getRepoByUrlmethod (introduced in test: improve repo DB tests and CheckRepoInAuthList tests #1109) to simplify logic and reliably load the correct repository.getRepoByUrltests accordingly.Notes
A potential future improvement would be to load the repository at the start of the action chain and pass it through to each step, avoiding repeated lookups.
The
isUserPushAllowedfunction is no longer used in the main code path, but cannot yet be removed because its tests intestDb.testimplicitly set up state that other tests depend on. Specifically, each test intestDb.testwrites to a file-based database, and subsequent tests rely on that data — making them fragile due to implicit ordering. Removing this function breaks unrelated tests. Refactoring these tests to be isolated and self-contained would be required before safely removingisUserPushAllowed, but that is out of scope for this PR.