Skip to content

Fix approval gate treating org members as external contributors#11435

Open
sylvainsf wants to merge 2 commits intomainfrom
fix-approvals
Open

Fix approval gate treating org members as external contributors#11435
sylvainsf wants to merge 2 commits intomainfrom
fix-approvals

Conversation

@sylvainsf
Copy link
Contributor

Description

The approval gate in functional-test-cloud.yaml incorrectly treats org members as external contributors, requiring manual approval for every PR — even from org owners.

Root Cause

The workflow uses github.event.pull_request.author_association to determine trust. However, GitHub webhook payloads compute author_association in an unauthenticated context. For org members with private membership visibility, this returns CONTRIBUTOR instead of MEMBER, causing the approval gate to trigger.

Evidence from workflow logs:

  • brooke-hamilton (public org member) → author_association: MEMBER → gate skipped ✅
  • DariuszPorowski (public org member) → author_association: MEMBER → gate skipped ✅
  • sylvainsf (private org member, owner) → author_association: CONTRIBUTOR → gate triggered ❌
  • sk593 (private org member) → author_association: CONTRIBUTOR → gate triggered ❌
  • willdavsmith (private org member) → author_association: CONTRIBUTOR → gate triggered ❌

Fix

Replace the author_association check with a new check-trust job that reliably determines trust:

  1. Same-repo PRs (head repo == base repo): Trusted immediately. Only users with write access can push branches to the repo.
  2. Fork PRs from org members: Trusted via GET /orgs/{org}/members/{username} API call (works regardless of membership visibility).
  3. Fork PRs from non-members: External — requires manual approval via external-contributor-approval environment.

This also correctly handles org members who prefer to work from forks.

Type of change

  • This pull request fixes a bug in Radius and has an approved issue (issue link required).

Contributor checklist

Please verify that the PR meets the following requirements, where applicable:

  • An overview of proposed schema changes is included in a linked GitHub issue.
    • Yes
    • Not applicable
  • A design document PR is created in the design-notes repository, if new APIs are being introduced.
    • Yes
    • Not applicable
  • The design document has been reviewed and approved by Radius maintainers/approvers.
    • Yes
    • Not applicable
  • A PR for the samples repository is created, if existing samples are affected by the changes in this PR.
    • Yes
    • Not applicable
  • A PR for the documentation repository is created, if the changes in this PR affect the documentation or any user facing updates are made.
    • Yes
    • Not applicable
  • A PR for the recipes repository is created, if existing recipes are affected by the changes in this PR.
    • Yes
    • Not applicable

Replace unreliable author_association check with org membership API
call. Webhook payloads compute author_association in an unauthenticated
context, which returns CONTRIBUTOR instead of MEMBER for org members
with private membership visibility.

The new approach uses a check-trust job that:
1. Fast-paths same-repo PRs as trusted (write access required to push)
2. For fork PRs, calls GET /orgs/{org}/members/{username} to verify
   org membership regardless of visibility settings
3. Falls back to requiring approval for unrecognized users
Copilot AI review requested due to automatic review settings March 15, 2026 22:25
@sylvainsf sylvainsf requested review from a team as code owners March 15, 2026 22:25
@sylvainsf sylvainsf requested a deployment to external-contributor-approval March 15, 2026 22:25 — with GitHub Actions Waiting
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the cloud functional test GitHub Actions workflow to replace author_association-based trust checks with an explicit “check trust” job that determines whether a pull_request_target PR author is external (fork + non-org-member), and gates execution behind an environment approval when needed.

Changes:

  • Add a check-trust job that classifies fork PRs as external by querying GitHub org membership.
  • Update approval-gate to depend on check-trust output rather than author_association.
  • Adjust setup job dependencies/conditions to incorporate the new trust-check flow.

Comment on lines +111 to +114
outputs:
is-external: ${{ steps.check.outputs.is-external }}
permissions: {}
steps:
(github.event_name != 'pull_request_target' || needs.approval-gate.result == 'success' || needs.approval-gate.result == 'skipped') &&
!cancelled() &&
needs.check-trust.result != 'failure' &&
needs.approval-gate.result != 'failure' &&
@github-actions
Copy link

github-actions bot commented Mar 15, 2026

Unit Tests

    2 files  ±0    415 suites  ±0   6m 41s ⏱️ -12s
4 881 tests ±0  4 879 ✅ ±0  2 💤 ±0  0 ❌ ±0 
5 783 runs  ±0  5 781 ✅ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit f7ade56. ± Comparison against base commit c30a4e2.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Mar 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.22%. Comparing base (c30a4e2) to head (f7ade56).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11435      +/-   ##
==========================================
- Coverage   51.24%   51.22%   -0.02%     
==========================================
  Files         699      699              
  Lines       44062    44062              
==========================================
- Hits        22580    22572       -8     
- Misses      19326    19330       +4     
- Partials     2156     2160       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Sylvain Niles <sylvainniles@microsoft.com>
@sylvainsf sylvainsf requested a deployment to external-contributor-approval March 15, 2026 23:15 — with GitHub Actions Waiting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants