diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/PushRecord.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/PushRecord.java index 37cda572..161726ca 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/PushRecord.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/PushRecord.java @@ -114,4 +114,13 @@ public class PushRecord { /** Attestation record if the push was manually reviewed. */ private Attestation attestation; + + /** + * Transient flag computed by the dashboard {@code PushController#getById} endpoint indicating whether the currently + * authenticated user is permitted to self-approve this specific push (i.e. they are the resolved pusher, have + * {@code ROLE_SELF_CERTIFY}, and have a {@code SELF_CERTIFY} repo permission for this push's path). Not persisted + * to the database. Used by the frontend to gate the self-certify banner and approve button. + */ + @Builder.Default + private boolean canCurrentUserSelfCertify = false; } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/ApprovalPreReceiveHook.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/ApprovalPreReceiveHook.java index 8305c18c..c4c9141c 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/ApprovalPreReceiveHook.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/ApprovalPreReceiveHook.java @@ -103,6 +103,12 @@ var record = pushStore.findById(validationRecordId).orElse(null); // Safety net: already approved before this hook ran (race condition or re-push) if (record.getStatus() == PushStatus.APPROVED) { + if (!verifySelfApprovalEntitled(record)) { + String reason = "Self-approved push rejected: no SELF_CERTIFY permission for this repository"; + sendAndFlush(rp, msgOut, color(RED, "" + sym(CROSS_MARK) + " " + reason)); + rejectAll(commands, reason); + return; + } sendAndFlush(rp, msgOut, color(GREEN, "" + sym(HEAVY_CHECK_MARK) + " Push already approved - forwarding")); return; } @@ -115,34 +121,6 @@ var record = pushStore.findById(validationRecordId).orElse(null); return; } - // Trusted contributor bypass: auto-approve without human review - if (repoPermissionService != null) { - String username = record.getUser(); - String provider = record.getProvider(); - String path = record.getUrl(); - if (username != null - && provider != null - && path != null - && repoPermissionService.isBypassReviewAllowed(username, provider, path)) { - pushStore.approve( - validationRecordId, - Attestation.builder() - .pushId(validationRecordId) - .type(Attestation.Type.APPROVAL) - .reviewerUsername(username) - .reason("Self-certified by " + username) - .automated(true) - .build()); - sendAndFlush( - rp, - msgOut, - color( - GREEN, - "" + sym(HEAVY_CHECK_MARK) + " Self-certified — push approved automatically")); - return; - } - } - sendAndFlush( rp, msgOut, color(YELLOW, "" + sym(WARNING) + " Push requires review. Waiting for approval...")); sendAndFlush(rp, msgOut, color(CYAN, "" + sym(KEY) + " Push ID: " + validationRecordId)); @@ -172,8 +150,16 @@ var record = pushStore.findById(validationRecordId).orElse(null); } switch (result) { - case APPROVED -> + case APPROVED -> { + var approvedRecord = pushStore.findById(validationRecordId).orElse(null); + if (approvedRecord != null && !verifySelfApprovalEntitled(approvedRecord)) { + String reason = "Self-approved push rejected: no SELF_CERTIFY permission for this repository"; + sendAndFlush(rp, msgOut, color(RED, "" + sym(CROSS_MARK) + " " + reason)); + rejectAll(commands, reason); + return; + } sendAndFlush(rp, msgOut, color(GREEN, "" + sym(HEAVY_CHECK_MARK) + " Push approved by reviewer")); + } case REJECTED -> { var updated = pushStore.findById(validationRecordId).orElse(null); String reason = updated != null && updated.getAttestation() != null @@ -200,6 +186,39 @@ var record = pushStore.findById(validationRecordId).orElse(null); } } + /** + * Defense in depth: if the approver is the pusher, re-verify that a {@code SELF_CERTIFY} repo permission still + * exists for the pusher on this push's path. {@link org.finos.gitproxy.dashboard.controller.PushController#approve} + * already enforces this at approval time, but re-checking here protects against future code paths or bugs that mark + * a record APPROVED without going through that gate. + * + *

The {@code ROLE_SELF_CERTIFY} role check is intentionally NOT performed at the hook layer — it requires Spring + * Security context (only available in the dashboard) and may live in IdP-derived authorities that aren't persisted + * to the user store. The hook re-verifies only the per-repo permission, which is the more granular authoritative + * gate. + * + * @return {@code true} if the push may proceed; {@code false} if the approver was the pusher and no + * {@code SELF_CERTIFY} permission row exists. + */ + private boolean verifySelfApprovalEntitled(org.finos.gitproxy.db.model.PushRecord record) { + if (repoPermissionService == null) return true; + Attestation att = record.getAttestation(); + if (att == null) return true; + String pusher = record.getResolvedUser(); + String approver = att.getReviewerUsername(); + if (pusher == null || approver == null || !pusher.equals(approver)) return true; + if (record.getProvider() == null || record.getUrl() == null) return true; + boolean entitled = repoPermissionService.isBypassReviewAllowed(pusher, record.getProvider(), record.getUrl()); + if (!entitled) { + log.warn( + "Self-approval rejected at hook: pusher={} provider={} path={} has no SELF_CERTIFY permission", + pusher, + record.getProvider(), + record.getUrl()); + } + return entitled; + } + private void rejectAll(Collection commands, String reason) { for (ReceiveCommand cmd : commands) { if (cmd.getResult() == ReceiveCommand.Result.NOT_ATTEMPTED) { diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/LocalRepositoryCache.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/LocalRepositoryCache.java index 2a80e8b6..22214a8c 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/LocalRepositoryCache.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/git/LocalRepositoryCache.java @@ -122,9 +122,7 @@ public Repository getOrClone(String remoteUrl, CredentialsProvider credentials) CachedRepository cached = cache.get(cacheKey); if (cached != null && cached.isValid()) { log.debug("Using cached repository for: {}", remoteUrl); - if (credentials != null) { - refreshIfStale(cached, cacheKey, credentials); - } + refreshIfStale(cached, cacheKey, credentials); cached.repository.incrementOpen(); return cached.repository; } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/GitProxyServlet.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/GitProxyServlet.java index 06ffab45..bd0d1ed0 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/GitProxyServlet.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/GitProxyServlet.java @@ -23,11 +23,6 @@ public class GitProxyServlet extends AsyncProxyServlet.Transparent { public static final String GIT_REQUEST_ATTR = "gitproxy.gitRequest"; public static final String ERROR_ATTR = "gitproxy.error"; public static final String PRE_APPROVED_ATTR = "gitproxy.preApproved"; - /** - * Request attribute set by {@code PushFinalizerFilter} when a self-certify grant was applied. Value is the resolved - * proxy username. - */ - public static final String SELF_CERTIFY_USER_ATTR = "gitproxy.selfCertifyUser"; /** Request attribute holding the UUID of the original APPROVED push record for a transparent-proxy re-push. */ public static final String APPROVED_PUSH_ID_ATTR = "gitproxy.approvedPushId"; diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/CheckHiddenCommitsFilter.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/CheckHiddenCommitsFilter.java index e856ae28..c0499acc 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/CheckHiddenCommitsFilter.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/CheckHiddenCommitsFilter.java @@ -85,7 +85,7 @@ public void doHttpFilter(HttpServletRequest request, HttpServletResponse respons return; } - Set allNew = collectAllNewCommits(repository, toCommit); + Set allNew = collectAllNewCommits(repository, toCommit, requestDetails.getCommitFrom()); Set hidden = new HashSet<>(allNew); hidden.removeAll(introduced); @@ -115,9 +115,14 @@ public void doHttpFilter(HttpServletRequest request, HttpServletResponse respons /** * Collect all commits reachable from {@code toCommit} that are not reachable from any existing ref in the upstream - * clone. These correspond to the commits that were new to the upstream in this pack. + * clone or from {@code fromCommit} (the old branch tip). These correspond to the commits that were genuinely new + * and not already present at the upstream. + * + *

{@code fromCommit} is marked uninteresting explicitly because the local clone's refs may be stale (e.g. after + * a recently forwarded push the remote ref has advanced but the cache hasn't been re-fetched). Without this, + * commits reachable only from the old tip — but not from any cached ref — would be falsely flagged as hidden. */ - private Set collectAllNewCommits(Repository repo, String toCommit) throws IOException { + private Set collectAllNewCommits(Repository repo, String toCommit, String fromCommit) throws IOException { Set result = new HashSet<>(); try (RevWalk walk = new RevWalk(repo)) { @@ -139,6 +144,19 @@ private Set collectAllNewCommits(Repository repo, String toCommit) throw } } + // Always mark commitFrom (old branch tip) as uninteresting — it is already at the + // upstream by definition, even when the local clone's refs are stale. + if (fromCommit != null && !fromCommit.isBlank() && !fromCommit.matches("^0+$")) { + try { + ObjectId fromId = repo.resolve(fromCommit); + if (fromId != null) { + walk.markUninteresting(walk.parseCommit(fromId)); + } + } catch (Exception e) { + log.debug("Could not resolve commitFrom {} as uninteresting boundary — skipping", fromCommit); + } + } + for (RevCommit commit : walk) { result.add(commit.getName()); } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilter.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilter.java index ded96fc3..05a29196 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilter.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilter.java @@ -20,7 +20,6 @@ import org.finos.gitproxy.approval.ApprovalGateway; import org.finos.gitproxy.git.GitRequestDetails; import org.finos.gitproxy.git.HttpOperation; -import org.finos.gitproxy.permission.RepoPermissionService; /** * Terminal filter for push operations that determines the final result: @@ -47,18 +46,11 @@ public class PushFinalizerFilter extends AbstractGitProxyFilter { private final String serviceUrl; private final ApprovalGateway approvalGateway; - private final RepoPermissionService repoPermissionService; public PushFinalizerFilter(String serviceUrl, ApprovalGateway approvalGateway) { - this(serviceUrl, approvalGateway, null); - } - - public PushFinalizerFilter( - String serviceUrl, ApprovalGateway approvalGateway, RepoPermissionService repoPermissionService) { super(ORDER, Set.of(HttpOperation.PUSH)); this.serviceUrl = serviceUrl; this.approvalGateway = approvalGateway; - this.repoPermissionService = repoPermissionService; } /** @@ -123,23 +115,12 @@ public void doHttpFilter(HttpServletRequest request, HttpServletResponse respons return; } - // Trusted contributor bypass: auto-approve without human review - if (repoPermissionService != null) { - String username = details.getResolvedUser(); - String provider = - details.getProvider() != null ? details.getProvider().getProviderId() : null; - String path = details.getRepoRef() != null ? details.getRepoRef().getSlug() : null; - if (username != null - && provider != null - && path != null - && repoPermissionService.isBypassReviewAllowed(username, provider, path)) { - request.setAttribute(SELF_CERTIFY_USER_ATTR, username); - details.setResult(GitRequestDetails.GitResult.ALLOWED); - return; - } - } - - // First push that passed validation - block pending review (dashboard/ServiceNow mode) + // First push that passed validation - block pending review (dashboard/ServiceNow mode). + // Self-certify is intentionally NOT enforced here: the role check requires Spring Security context + // which the proxy filter chain does not have. Self-approval is gated entirely in the dashboard + // (PushController.checkReviewerIdentity), where both ROLE_SELF_CERTIFY and the SELF_CERTIFY repo + // permission are required. The pre-receive hook re-verifies the per-repo permission as defense in + // depth before forwarding an approved self-review. details.setResult(GitRequestDetails.GitResult.REVIEW); String pushId = details.getId().toString(); String summary = buildValidationSummary(details.getSteps()); diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilter.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilter.java index 6c3df12f..77662ba9 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilter.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilter.java @@ -2,7 +2,6 @@ import static org.finos.gitproxy.servlet.GitProxyServlet.GIT_REQUEST_ATTR; import static org.finos.gitproxy.servlet.GitProxyServlet.PRE_APPROVED_ATTR; -import static org.finos.gitproxy.servlet.GitProxyServlet.SELF_CERTIFY_USER_ATTR; import jakarta.servlet.*; import jakarta.servlet.http.HttpServletRequest; @@ -11,7 +10,6 @@ import lombok.extern.slf4j.Slf4j; import org.finos.gitproxy.db.PushRecordMapper; import org.finos.gitproxy.db.PushStore; -import org.finos.gitproxy.db.model.Attestation; import org.finos.gitproxy.db.model.PushRecord; import org.finos.gitproxy.git.GitRequestDetails; import org.finos.gitproxy.git.HttpOperation; @@ -56,22 +54,6 @@ private void persistIfPush(ServletRequest request, ServletResponse response) { PushRecord record = PushRecordMapper.fromRequestDetails(requestDetails); pushStore.save(record); - // If a self-certify grant was applied, stamp the automated attestation now that - // the record exists. PushFinalizerFilter already set the result to ALLOWED so the - // saved status is APPROVED. - String bypassUser = (String) httpRequest.getAttribute(SELF_CERTIFY_USER_ATTR); - if (bypassUser != null) { - pushStore.approve( - record.getId(), - Attestation.builder() - .pushId(record.getId()) - .type(Attestation.Type.APPROVAL) - .reviewerUsername(bypassUser) - .reason("Self-certified by " + bypassUser) - .automated(true) - .build()); - } - log.info( "Persisted push record: id={}, repo={}, status={}", record.getId(), diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/ApprovalPreReceiveHookTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/ApprovalPreReceiveHookTest.java index 3a5422c5..ac0ae805 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/ApprovalPreReceiveHookTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/ApprovalPreReceiveHookTest.java @@ -20,8 +20,10 @@ import org.finos.gitproxy.approval.ApprovalGateway; import org.finos.gitproxy.approval.ApprovalResult; import org.finos.gitproxy.db.PushStore; +import org.finos.gitproxy.db.model.Attestation; import org.finos.gitproxy.db.model.PushRecord; import org.finos.gitproxy.db.model.PushStatus; +import org.finos.gitproxy.permission.RepoPermissionService; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -153,6 +155,157 @@ void blockedPush_gatewayRejects_commandRejected() throws Exception { assertEquals(ReceiveCommand.Result.REJECTED_OTHER_REASON, cmd.getResult()); } + // ── Defense-in-depth: hook re-verifies SELF_CERTIFY perm when approver == pusher ───────────── + + @Test + void selfApproved_alreadyApprovedAtHookStart_noPerm_rejected() throws Exception { + String recordId = UUID.randomUUID().toString(); + repo.getConfig().setString("gitproxy", null, "validationRecordId", recordId); + repo.getConfig().save(); + Attestation att = Attestation.builder() + .pushId(recordId) + .type(Attestation.Type.APPROVAL) + .reviewerUsername("alice") + .build(); + PushRecord record = PushRecord.builder() + .id(recordId) + .status(PushStatus.APPROVED) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .attestation(att) + .build(); + when(pushStore.findById(recordId)).thenReturn(Optional.of(record)); + RepoPermissionService perms = mock(RepoPermissionService.class); + when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) + .thenReturn(false); + + RevCommit c1 = createCommit("init"); + RevCommit c2 = createCommit("second"); + ReceivePack rp = new ReceivePack(repo); + ReceiveCommand cmd = new ReceiveCommand(c1.getId(), c2.getId(), "refs/heads/main", ReceiveCommand.Type.UPDATE); + + new ApprovalPreReceiveHook(pushStore, approvalGateway, Duration.ofSeconds(5), null, perms) + .onPreReceive(rp, List.of(cmd)); + + assertEquals(ReceiveCommand.Result.REJECTED_OTHER_REASON, cmd.getResult()); + verify(perms).isBypassReviewAllowed("alice", "github/github.com", "/owner/repo"); + } + + @Test + void selfApproved_alreadyApprovedAtHookStart_withPerm_passes() throws Exception { + String recordId = UUID.randomUUID().toString(); + repo.getConfig().setString("gitproxy", null, "validationRecordId", recordId); + repo.getConfig().save(); + Attestation att = Attestation.builder() + .pushId(recordId) + .type(Attestation.Type.APPROVAL) + .reviewerUsername("alice") + .build(); + PushRecord record = PushRecord.builder() + .id(recordId) + .status(PushStatus.APPROVED) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .attestation(att) + .build(); + when(pushStore.findById(recordId)).thenReturn(Optional.of(record)); + RepoPermissionService perms = mock(RepoPermissionService.class); + when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) + .thenReturn(true); + + RevCommit c1 = createCommit("init"); + RevCommit c2 = createCommit("second"); + ReceivePack rp = new ReceivePack(repo); + ReceiveCommand cmd = new ReceiveCommand(c1.getId(), c2.getId(), "refs/heads/main", ReceiveCommand.Type.UPDATE); + + new ApprovalPreReceiveHook(pushStore, approvalGateway, Duration.ofSeconds(5), null, perms) + .onPreReceive(rp, List.of(cmd)); + + assertEquals(ReceiveCommand.Result.NOT_ATTEMPTED, cmd.getResult()); + } + + @Test + void selfApproved_viaWaitForApproval_noPerm_rejected() throws Exception { + String recordId = UUID.randomUUID().toString(); + repo.getConfig().setString("gitproxy", null, "validationRecordId", recordId); + repo.getConfig().save(); + // Initial fetch returns PENDING (no attestation yet); after approval, returns APPROVED with attestation + // showing the pusher self-approved. + PushRecord pending = PushRecord.builder() + .id(recordId) + .status(PushStatus.PENDING) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .build(); + Attestation att = Attestation.builder() + .pushId(recordId) + .type(Attestation.Type.APPROVAL) + .reviewerUsername("alice") + .build(); + PushRecord approved = PushRecord.builder() + .id(recordId) + .status(PushStatus.APPROVED) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .attestation(att) + .build(); + when(pushStore.findById(recordId)).thenReturn(Optional.of(pending)).thenReturn(Optional.of(approved)); + when(approvalGateway.waitForApproval(eq(recordId), any(), any(Duration.class))) + .thenReturn(ApprovalResult.APPROVED); + RepoPermissionService perms = mock(RepoPermissionService.class); + when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) + .thenReturn(false); + + RevCommit c1 = createCommit("init"); + RevCommit c2 = createCommit("second"); + ReceivePack rp = new ReceivePack(repo); + ReceiveCommand cmd = new ReceiveCommand(c1.getId(), c2.getId(), "refs/heads/main", ReceiveCommand.Type.UPDATE); + + new ApprovalPreReceiveHook(pushStore, approvalGateway, Duration.ofSeconds(5), null, perms) + .onPreReceive(rp, List.of(cmd)); + + assertEquals(ReceiveCommand.Result.REJECTED_OTHER_REASON, cmd.getResult()); + verify(perms).isBypassReviewAllowed("alice", "github/github.com", "/owner/repo"); + } + + @Test + void differentApproverThanPusher_noReVerifyNeeded() throws Exception { + // Approver != pusher → defense-in-depth check skipped; push is forwarded. + String recordId = UUID.randomUUID().toString(); + repo.getConfig().setString("gitproxy", null, "validationRecordId", recordId); + repo.getConfig().save(); + Attestation att = Attestation.builder() + .pushId(recordId) + .type(Attestation.Type.APPROVAL) + .reviewerUsername("bob") + .build(); + PushRecord record = PushRecord.builder() + .id(recordId) + .status(PushStatus.APPROVED) + .resolvedUser("alice") + .provider("github/github.com") + .url("/owner/repo") + .attestation(att) + .build(); + when(pushStore.findById(recordId)).thenReturn(Optional.of(record)); + RepoPermissionService perms = mock(RepoPermissionService.class); + + RevCommit c1 = createCommit("init"); + RevCommit c2 = createCommit("second"); + ReceivePack rp = new ReceivePack(repo); + ReceiveCommand cmd = new ReceiveCommand(c1.getId(), c2.getId(), "refs/heads/main", ReceiveCommand.Type.UPDATE); + + new ApprovalPreReceiveHook(pushStore, approvalGateway, Duration.ofSeconds(5), null, perms) + .onPreReceive(rp, List.of(cmd)); + + assertEquals(ReceiveCommand.Result.NOT_ATTEMPTED, cmd.getResult()); + verifyNoInteractions(perms); + } + @Test void blockedPush_gatewayTimesOut_commandRejected() throws Exception { String recordId = UUID.randomUUID().toString(); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilterTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilterTest.java index 5befa05d..205bd99c 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilterTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushFinalizerFilterTest.java @@ -3,7 +3,6 @@ import static org.finos.gitproxy.git.GitClientUtils.ZERO_OID; import static org.finos.gitproxy.servlet.GitProxyServlet.GIT_REQUEST_ATTR; import static org.finos.gitproxy.servlet.GitProxyServlet.PRE_APPROVED_ATTR; -import static org.finos.gitproxy.servlet.GitProxyServlet.SELF_CERTIFY_USER_ATTR; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*; @@ -26,7 +25,6 @@ import org.finos.gitproxy.db.PushStoreFactory; import org.finos.gitproxy.git.GitRequestDetails; import org.finos.gitproxy.git.HttpOperation; -import org.finos.gitproxy.permission.RepoPermissionService; import org.finos.gitproxy.provider.GitProxyProvider; import org.junit.jupiter.api.Test; @@ -196,31 +194,13 @@ void nullDetails_doesNotThrow() throws Exception { } @Test - void selfCertify_granted_allowsPushAndSetsAttribute() throws Exception { + void selfCertify_perm_doesNotBypassReview() throws Exception { + // The push-time bypass for SELF_CERTIFY was removed: self-certify is enforced exclusively in the dashboard + // (PushController.checkReviewerIdentity). The pre-receive hook re-verifies the per-repo permission as defense + // in depth before forwarding an approved self-review. From the proxy filter chain's perspective, every clean + // push with no prior approval blocks pending review regardless of any SELF_CERTIFY grants. GitRequestDetails details = pendingPushDetailsWithUser("alice", "github"); - RepoPermissionService perms = mock(RepoPermissionService.class); - when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) - .thenReturn(true); - PushFinalizerFilter filter = - new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class), perms); - HttpServletRequest req = mockPushRequest(details); - FakeResponse fakeResponse = new FakeResponse(); - - filter.doHttpFilter(req, fakeResponse.mock); - - assertEquals(GitRequestDetails.GitResult.ALLOWED, details.getResult()); - verify(req).setAttribute(SELF_CERTIFY_USER_ATTR, "alice"); - assertFalse(fakeResponse.committed.get(), "Self-certify must not send a git error to the client"); - } - - @Test - void selfCertify_notGranted_blocksPendingReview() throws Exception { - GitRequestDetails details = pendingPushDetailsWithUser("alice", "github"); - RepoPermissionService perms = mock(RepoPermissionService.class); - when(perms.isBypassReviewAllowed("alice", "github/github.com", "/owner/repo")) - .thenReturn(false); - PushFinalizerFilter filter = - new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class), perms); + PushFinalizerFilter filter = new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class)); FakeResponse fakeResponse = new FakeResponse(); filter.doHttpFilter(mockPushRequest(details), fakeResponse.mock); @@ -229,49 +209,6 @@ void selfCertify_notGranted_blocksPendingReview() throws Exception { assertTrue(fakeResponse.committed.get()); } - @Test - void selfCertify_nullProvider_fallsThroughToReview() throws Exception { - // repoPermissionService non-null, resolvedUser set, but no provider → provider ternary takes null branch - // compound condition: username!=null && provider!=null → false → bypass skipped - GitRequestDetails details = pendingPushDetails(); - details.setResolvedUser("alice"); // so username != null; provider is still null - RepoPermissionService perms = mock(RepoPermissionService.class); - PushFinalizerFilter filter = - new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class), perms); - FakeResponse fakeResponse = new FakeResponse(); - - filter.doHttpFilter(mockPushRequest(details), fakeResponse.mock); - - assertEquals(GitRequestDetails.GitResult.REVIEW, details.getResult()); - verifyNoInteractions(perms); - } - - @Test - void selfCertify_nullRepoRef_fallsThroughToReview() throws Exception { - // repoPermissionService non-null, provider set, but repoRef is null → path ternary takes null branch → bypass - // skipped - GitRequestDetails details = new GitRequestDetails(); - details.setOperation(HttpOperation.PUSH); - details.setCommitTo("abc123"); - details.setResolvedUser("alice"); - GitProxyProvider provider = mock(GitProxyProvider.class); - when(provider.getName()).thenReturn("github"); - when(provider.getType()).thenReturn("github"); - when(provider.getUri()).thenReturn(java.net.URI.create("https://github.com")); - when(provider.getProviderId()).thenReturn("github/github.com"); - details.setProvider(provider); - // repoRef intentionally not set - RepoPermissionService perms = mock(RepoPermissionService.class); - PushFinalizerFilter filter = - new PushFinalizerFilter("http://localhost:8080", mock(ApprovalGateway.class), perms); - FakeResponse fakeResponse = new FakeResponse(); - - filter.doHttpFilter(mockPushRequest(details), fakeResponse.mock); - - assertEquals(GitRequestDetails.GitResult.REVIEW, details.getResult()); - verifyNoInteractions(perms); - } - @Test void autoApprovalGateway_allowsPushWithoutBlocking() throws Exception { GitRequestDetails details = pendingPushDetails(); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilterTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilterTest.java index fd61f76b..17e720d5 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilterTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/PushStoreAuditFilterTest.java @@ -2,7 +2,6 @@ import static org.finos.gitproxy.servlet.GitProxyServlet.GIT_REQUEST_ATTR; import static org.finos.gitproxy.servlet.GitProxyServlet.PRE_APPROVED_ATTR; -import static org.finos.gitproxy.servlet.GitProxyServlet.SELF_CERTIFY_USER_ATTR; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*; @@ -12,7 +11,6 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import org.finos.gitproxy.db.PushStore; -import org.finos.gitproxy.db.model.Attestation; import org.finos.gitproxy.db.model.PushRecord; import org.finos.gitproxy.git.GitRequestDetails; import org.finos.gitproxy.git.HttpOperation; @@ -141,38 +139,6 @@ void preApprovedRePush_notPersisted() throws Exception { verifyNoInteractions(pushStore); } - // ---- self-certify bypass stamps automated attestation ---- - - @Test - void selfCertifyBypass_stampsAutomatedAttestation() throws Exception { - FilterChain chain = mock(FilterChain.class); - HttpServletRequest req = pushRequest(pushDetails()); - when(req.getAttribute(SELF_CERTIFY_USER_ATTR)).thenReturn("alice"); - - filter.doFilter(req, mock(HttpServletResponse.class), chain); - - verify(pushStore).save(any(PushRecord.class)); - verify(pushStore) - .approve( - any(), - argThat((Attestation a) -> a.isAutomated() - && "alice".equals(a.getReviewerUsername()) - && a.getReason() != null - && a.getReason().contains("alice"))); - } - - @Test - void noBypasAttribute_noApproveCall() throws Exception { - FilterChain chain = mock(FilterChain.class); - HttpServletRequest req = pushRequest(pushDetails()); - when(req.getAttribute(SELF_CERTIFY_USER_ATTR)).thenReturn(null); - - filter.doFilter(req, mock(HttpServletResponse.class), chain); - - verify(pushStore).save(any(PushRecord.class)); - verify(pushStore, never()).approve(any(), any()); - } - // ---- store failure does not propagate (audit must not break pushes) ---- @Test diff --git a/git-proxy-java-dashboard/frontend/src/pages/Admin.tsx b/git-proxy-java-dashboard/frontend/src/pages/Admin.tsx index f29d41ff..778a2db9 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/Admin.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/Admin.tsx @@ -62,9 +62,7 @@ function HttpBadge({ http }: { http: HttpResult | null }) { function GitProbeBadge({ label, result }: { label: string; result: GitProbeResult }) { const ok = result.status === 'ok' - const detail = ok - ? `${result.httpStatus} ${ms(result.durationMs)}` - : (result.error ?? 'ERROR') + const detail = ok ? `${result.httpStatus} ${ms(result.durationMs)}` : (result.error ?? 'ERROR') return ( )} - {result.gitProbe.receivePack.status === 'ok' && result.gitProbe.receivePack.contentType && ( -

- push content-type: {result.gitProbe.receivePack.contentType} -
- )} + {result.gitProbe.receivePack.status === 'ok' && + result.gitProbe.receivePack.contentType && ( +
+ push content-type: {result.gitProbe.receivePack.contentType} +
+ )} )} {result.steps && result.steps.length > 0 && } @@ -227,7 +226,9 @@ export function Admin() { const [repoPath, setRepoPath] = useState('') const [targetStatus, setTargetStatus] = useState<'idle' | 'loading' | 'done' | 'error'>('idle') const [targetCheckedAt, setTargetCheckedAt] = useState(null) - const [targetResults, setTargetResults] = useState | null>(null) + const [targetResults, setTargetResults] = useState | null>( + null, + ) const [targetError, setTargetError] = useState(null) useEffect(() => { @@ -391,7 +392,9 @@ export function Admin() {
diff --git a/git-proxy-java-dashboard/frontend/src/pages/Profile.tsx b/git-proxy-java-dashboard/frontend/src/pages/Profile.tsx index eb41c9a1..630ad3ae 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/Profile.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/Profile.tsx @@ -145,12 +145,20 @@ export function Profile() { : label === 'SELF_CERTIFY' ? 'bg-amber-100 text-amber-700' : 'bg-gray-100 text-gray-600' + const isSelfCertify = label === 'SELF_CERTIFY' return ( - - {label} + + + {label} + + {isSelfCertify && ( + + This role allows you to self-approve your own pushes, but an admin must also grant the Self-certify permission on each individual repository. + + + )} ) })} diff --git a/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx b/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx index b3b1a903..775c1141 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/PushDetail.tsx @@ -883,13 +883,17 @@ export function PushDetail({ currentUser }: PushDetailProps) { {record.status === 'PENDING' && (() => { const isAdmin = currentUser?.authorities?.includes('ROLE_ADMIN') ?? false - const isSelfCertifier = - currentUser?.authorities?.includes('ROLE_SELF_CERTIFY') ?? false + // canCurrentUserSelfCertify is computed server-side and requires BOTH the + // ROLE_SELF_CERTIFY authority AND a SELF_CERTIFY repo permission row for this push's + // path. A user with the role but no per-repo permission gets `false` here, so the + // self-certify banner stays hidden and the approve button is gated behind the + // standard self-review block. + const canSelfCertify = record.canCurrentUserSelfCertify ?? false const isPusher = !!currentUser?.username && !!record.resolvedUser && currentUser.username === record.resolvedUser - const isSelfReview = isPusher && !isAdmin && !isSelfCertifier + const isSelfReview = isPusher && !isAdmin && !canSelfCertify const canCancel = isAdmin || isPusher const attestationsComplete = attestationQuestions .filter((q) => q.required) @@ -927,7 +931,7 @@ export function PushDetail({ currentUser }: PushDetailProps) {
)} - {isSelfCertifier && isPusher && !isAdmin && ( + {canSelfCertify && isPusher && !isAdmin && (
ℹ diff --git a/git-proxy-java-dashboard/frontend/src/types.ts b/git-proxy-java-dashboard/frontend/src/types.ts index cb0276bd..6ce9abee 100644 --- a/git-proxy-java-dashboard/frontend/src/types.ts +++ b/git-proxy-java-dashboard/frontend/src/types.ts @@ -71,6 +71,12 @@ export interface PushRecord { attestation?: Attestation commits?: Commit[] steps?: Step[] + /** + * Server-computed flag (only set on GET /api/push/{id}): the current authenticated user is the resolved pusher, + * holds ROLE_SELF_CERTIFY, AND has a SELF_CERTIFY repo permission for this push's path. Gates the self-certify + * banner and approve button in the UI. + */ + canCurrentUserSelfCertify?: boolean } export interface Provider { diff --git a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/ConnectivityController.java b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/ConnectivityController.java index 7236f076..1a335368 100644 --- a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/ConnectivityController.java +++ b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/ConnectivityController.java @@ -45,10 +45,9 @@ *
  • TLS — complete the TLS handshake (HTTPS providers only). Reports negotiated protocol and cipher, or the * specific exception class for certificate / SNI failures. *
  • HTTP — send {@code GET /} and record the status code and response time. - *
  • Git probe (targeted check only) — send {@code GET /info/refs?service=git-upload-pack} and - * {@code GET /info/refs?service=git-receive-pack} with {@code User-Agent: git/2.x.x} to a specific repo URL. - * Distinguishes appliances that pass generic HTTP but block git-specific URL patterns, query strings, or - * user-agent strings. + *
  • Git probe (targeted check only) — send {@code GET /info/refs?service=git-upload-pack} and {@code GET + * /info/refs?service=git-receive-pack} with {@code User-Agent: git/2.x.x} to a specific repo URL. Distinguishes + * appliances that pass generic HTTP but block git-specific URL patterns, query strings, or user-agent strings. * * *

    Every step is logged at INFO level. Targeted checks additionally return a structured {@code steps} log in the API @@ -73,8 +72,8 @@ public class ConnectivityController { * optional git probe step. * * @param providerName optional — name of the provider to target; if absent all providers are checked - * @param repoPath optional — repo path (e.g. {@code /owner/repo.git}) appended to the provider base URI for - * the git probe step; requires {@code provider} + * @param repoPath optional — repo path (e.g. {@code /owner/repo.git}) appended to the provider base URI for the git + * probe step; requires {@code provider} */ @GetMapping("/api/admin/connectivity") public Map check( @@ -92,8 +91,8 @@ public Map check( GitProxyProvider provider = providers.getProviders().stream() .filter(p -> p.getName().equals(providerName)) .findFirst() - .orElseThrow(() -> new ResponseStatusException( - HttpStatus.BAD_REQUEST, "Unknown provider: " + providerName)); + .orElseThrow(() -> + new ResponseStatusException(HttpStatus.BAD_REQUEST, "Unknown provider: " + providerName)); log.info("--- Provider: {} ({}) [targeted] ---", provider.getName(), provider.getUri()); List> steps = new ArrayList<>(); Map result = checkProvider(provider, sslContext, steps); diff --git a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java index a746d005..1bdc0570 100644 --- a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java +++ b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PushController.java @@ -122,11 +122,32 @@ public ResponseEntity getById(@PathVariable String id) { .findById(id) .map(record -> { stripDiffContent(record); + record.setCanCurrentUserSelfCertify(computeCanCurrentUserSelfCertify(record)); return ResponseEntity.ok(record); }) .orElse(ResponseEntity.notFound().build()); } + /** + * Computes whether the currently authenticated user is permitted to self-approve this specific push. Mirrors the + * server-side enforcement in {@link #checkReviewerIdentity}: the user must be the resolved pusher, hold the + * {@code ROLE_SELF_CERTIFY} authority (capability gate), and have a matching {@code SELF_CERTIFY} repo permission + * row (per-repo entitlement). Admins do not need self-certify — they can approve anything via the regular path. + */ + private boolean computeCanCurrentUserSelfCertify(PushRecord record) { + Authentication auth = SecurityContextHolder.getContext().getAuthentication(); + if (auth == null) return false; + String reviewer = auth.getName(); + String pusher = record.getResolvedUser(); + if (reviewer == null || pusher == null || !pusher.equals(reviewer)) return false; + boolean hasRole = auth.getAuthorities().stream().anyMatch(a -> "ROLE_SELF_CERTIFY".equals(a.getAuthority())); + if (!hasRole) return false; + return repoPermissionService != null + && record.getProvider() != null + && record.getUrl() != null + && repoPermissionService.isBypassReviewAllowed(reviewer, record.getProvider(), record.getUrl()); + } + /** * Returns the raw unified diff for a push. Separated from the main push record response so that large diffs do not * block page load — the dashboard fetches this lazily and decides whether to render inline or link to the diff --git a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/ConnectivityErrorClassifierTest.java b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/ConnectivityErrorClassifierTest.java index a2ba6598..22fe28dd 100644 --- a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/ConnectivityErrorClassifierTest.java +++ b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/ConnectivityErrorClassifierTest.java @@ -2,10 +2,10 @@ import static org.junit.jupiter.api.Assertions.assertEquals; +import java.io.IOException; import java.net.ConnectException; import java.net.SocketException; import java.net.SocketTimeoutException; -import java.io.IOException; import javax.net.ssl.SSLHandshakeException; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -17,17 +17,21 @@ class ClassifyTcpError { @Test void socketTimeoutException_returnsTimeout() { - assertEquals("TIMEOUT", ConnectivityErrorClassifier.classifyTcpError(new SocketTimeoutException("timed out"))); + assertEquals( + "TIMEOUT", ConnectivityErrorClassifier.classifyTcpError(new SocketTimeoutException("timed out"))); } @Test void connectException_withRefusedMessage_returnsRefused() { - assertEquals("REFUSED", ConnectivityErrorClassifier.classifyTcpError(new ConnectException("Connection refused"))); + assertEquals( + "REFUSED", + ConnectivityErrorClassifier.classifyTcpError(new ConnectException("Connection refused"))); } @Test void connectException_withResetMessage_returnsReset() { - assertEquals("RESET", ConnectivityErrorClassifier.classifyTcpError(new ConnectException("Connection reset"))); + assertEquals( + "RESET", ConnectivityErrorClassifier.classifyTcpError(new ConnectException("Connection reset"))); } @Test @@ -38,17 +42,21 @@ void connectException_withBlankMessage_returnsRefused() { @Test void socketException_withResetMessage_returnsReset() { - assertEquals("RESET", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection reset"))); + assertEquals( + "RESET", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection reset"))); } @Test void socketException_withRefusedMessage_returnsRefused() { - assertEquals("REFUSED", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection refused"))); + assertEquals( + "REFUSED", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection refused"))); } @Test void socketException_withTimedOutMessage_returnsTimeout() { - assertEquals("TIMEOUT", ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection timed out"))); + assertEquals( + "TIMEOUT", + ConnectivityErrorClassifier.classifyTcpError(new SocketException("Connection timed out"))); } @Test @@ -78,7 +86,8 @@ void directSocketTimeout_returnsTimeout() { @Test void wrappedSocketTimeout_unwrapsAndReturnsTimeout() { // HttpClient wraps the real cause in an outer IOException - IOException wrapper = new IOException("HTTP request failed", new SocketTimeoutException("connect timed out")); + IOException wrapper = + new IOException("HTTP request failed", new SocketTimeoutException("connect timed out")); assertEquals("TIMEOUT", ConnectivityErrorClassifier.classifyNetworkError(wrapper)); } @@ -111,44 +120,56 @@ class ClassifyTlsError { @Test void pkixPathBuilding_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("PKIX path building failed: unable to find valid certification path"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError(new SSLHandshakeException( + "PKIX path building failed: unable to find valid certification path"))); } @Test void certificateKeyword_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("Certificate expired"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError(new SSLHandshakeException("Certificate expired"))); } @Test void certKeyword_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("self-signed cert in chain"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError( + new SSLHandshakeException("self-signed cert in chain"))); } @Test void trustAnchorKeyword_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("No trust anchor found for issuer"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError( + new SSLHandshakeException("No trust anchor found for issuer"))); } @Test void trustanchorOneWord_returnsCertInvalid() { - assertEquals("TLS_CERT_INVALID", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("trustanchor for certpath not found"))); + assertEquals( + "TLS_CERT_INVALID", + ConnectivityErrorClassifier.classifyTlsError( + new SSLHandshakeException("trustanchor for certpath not found"))); } @Test void generalAlertFromPeer_returnsHandshakeFailed() { - assertEquals("TLS_HANDSHAKE_FAILED", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("Received fatal alert: handshake_failure"))); + assertEquals( + "TLS_HANDSHAKE_FAILED", + ConnectivityErrorClassifier.classifyTlsError( + new SSLHandshakeException("Received fatal alert: handshake_failure"))); } @Test void protocolVersionMismatch_returnsHandshakeFailed() { - assertEquals("TLS_HANDSHAKE_FAILED", ConnectivityErrorClassifier.classifyTlsError( - new SSLHandshakeException("No appropriate protocol"))); + assertEquals( + "TLS_HANDSHAKE_FAILED", + ConnectivityErrorClassifier.classifyTlsError(new SSLHandshakeException("No appropriate protocol"))); } } } diff --git a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java index c7cd27c6..193749bb 100644 --- a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java +++ b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PushControllerTest.java @@ -175,6 +175,73 @@ void notFound_returns404() { when(pushStore.findById("missing")).thenReturn(Optional.empty()); assertEquals(HttpStatus.NOT_FOUND, controller.getById("missing").getStatusCode()); } + + @Test + void canCurrentUserSelfCertify_falseWhenNotAuthenticated() { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + // No SecurityContext authentication set + var resp = controller.getById("p1"); + assertEquals(HttpStatus.OK, resp.getStatusCode()); + assertEquals(false, resp.getBody().isCanCurrentUserSelfCertify()); + } + + @Test + void canCurrentUserSelfCertify_falseWhenViewerIsNotPusher() { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + loginAs("bob", false, true); // role granted, but bob is not the pusher + var resp = controller.getById("p1"); + assertEquals(false, resp.getBody().isCanCurrentUserSelfCertify()); + } + + @Test + void canCurrentUserSelfCertify_falseWhenRoleMissing() throws Exception { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + loginAs("alice", false, false); // pusher, but no ROLE_SELF_CERTIFY + // Permission service stub intentionally omitted — the role check short-circuits before + // computeCanCurrentUserSelfCertify ever consults the permission service. + repoPermissionService = mock(RepoPermissionService.class); + var field = PushController.class.getDeclaredField("repoPermissionService"); + field.setAccessible(true); + field.set(controller, repoPermissionService); + + var resp = controller.getById("p1"); + assertEquals(false, resp.getBody().isCanCurrentUserSelfCertify()); + } + + @Test + void canCurrentUserSelfCertify_falseWhenPermMissing() throws Exception { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + loginAs("alice", false, true); // pusher with role + repoPermissionService = mock(RepoPermissionService.class); + when(repoPermissionService.isBypassReviewAllowed("alice", "github", "github.com/acme/repo.git")) + .thenReturn(false); + var field = PushController.class.getDeclaredField("repoPermissionService"); + field.setAccessible(true); + field.set(controller, repoPermissionService); + + var resp = controller.getById("p1"); + assertEquals(false, resp.getBody().isCanCurrentUserSelfCertify()); + } + + @Test + void canCurrentUserSelfCertify_trueWhenRoleAndPermBothGranted() throws Exception { + var push = blockedPush("p1", "alice"); + when(pushStore.findById("p1")).thenReturn(Optional.of(push)); + loginAs("alice", false, true); // pusher with role + repoPermissionService = mock(RepoPermissionService.class); + when(repoPermissionService.isBypassReviewAllowed("alice", "github", "github.com/acme/repo.git")) + .thenReturn(true); + var field = PushController.class.getDeclaredField("repoPermissionService"); + field.setAccessible(true); + field.set(controller, repoPermissionService); + + var resp = controller.getById("p1"); + assertEquals(true, resp.getBody().isCanCurrentUserSelfCertify()); + } } // ── POST /api/push/{id}/authorise ───────────────────────────────────────────── diff --git a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/GitProxyServletRegistrar.java b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/GitProxyServletRegistrar.java index 0c240f87..ad072621 100644 --- a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/GitProxyServletRegistrar.java +++ b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/GitProxyServletRegistrar.java @@ -242,7 +242,7 @@ public static void registerCoreFilters( filters.add(new GpgSignatureFilter(GpgConfig.defaultConfig())); filters.add(new ValidationSummaryFilter()); filters.add(new FetchFinalizerFilter()); - filters.add(new PushFinalizerFilter(serviceUrl, approvalGateway, repoPermissionService)); + filters.add(new PushFinalizerFilter(serviceUrl, approvalGateway)); filters.add(new AuditLogFilter()); boolean failFast = configBuilder != null && configBuilder.isFailFast();