Skip to content

feat: add authz support for GitHub teams#4139

Merged
rchincha merged 10 commits into
project-zot:mainfrom
rchincha:nForced-uk-feature-github-teams
Jun 17, 2026
Merged

feat: add authz support for GitHub teams#4139
rchincha merged 10 commits into
project-zot:mainfrom
rchincha:nForced-uk-feature-github-teams

Conversation

@rchincha

Copy link
Copy Markdown
Contributor

What type of PR is this?

Continuation of #4124

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds support for using GitHub Team membership as authorization groups by fetching a user’s orgs and teams (with pagination) during GitHub auth, and documenting how to reference team groups in access-control policy.

Changes:

  • Update GetGithubUserInfo to page through GitHub org and team memberships and gracefully skip 403s (e.g., missing read:org scope).
  • Extend controller tests with paginated org/team responses and new team error/forbidden scenarios.
  • Document how GitHub team groups (<org>/<team-slug>) can be used in access-control policies.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
pkg/api/authn.go Fetches GitHub orgs + teams with pagination; treats 403 as “no groups” instead of fatal.
pkg/api/controller_test.go Updates GitHub mock expectations to cover pagination and team-related scenarios.
examples/README.md Adds configuration guidance for using GitHub team groups in access control.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/api/controller_test.go Outdated
Comment thread pkg/api/controller_test.go Outdated
Comment thread pkg/api/controller_test.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

Comment thread pkg/api/authn.go
Comment thread pkg/api/authn.go
Comment thread examples/README.md Outdated
Comment thread pkg/api/controller_test.go Outdated
Comment thread pkg/api/controller_test.go Outdated
Comment thread pkg/api/controller_test.go Outdated
@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.11111% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.61%. Comparing base (55b6822) to head (0b1d932).

Files with missing lines Patch % Lines
pkg/api/authn.go 91.11% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4139      +/-   ##
==========================================
- Coverage   91.61%   91.61%   -0.01%     
==========================================
  Files         200      200              
  Lines       29514    29551      +37     
==========================================
+ Hits        27040    27073      +33     
- Misses       1592     1594       +2     
- Partials      882      884       +2     

☔ View full report in Codecov by Harness.
📢 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

Comment thread pkg/api/controller_test.go Outdated
Comment thread pkg/api/controller_test.go Outdated
Comment thread pkg/api/controller_test.go Outdated
Comment thread pkg/api/controller_test.go Outdated
Comment thread pkg/api/controller_test.go Outdated
Comment thread examples/README.md Outdated
Comment thread pkg/api/controller_test.go Outdated
Caffe1neAdd1ct and others added 8 commits June 16, 2026 23:20
Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
- apply the same optional-scope strategy to org lookup: paginate org pages and
  treat 403 Forbidden as non-fatal
- keep non-403 org/team API errors as hard failures
- preserve provider-returned casing for org/team-derived group values
- add anonymized debug logging (counts/page metadata only)
- extend tests for org pagination, org 403 optional behavior, team pagination,
  team 403 optional behavior, and team 5xx hard-fail behavior

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
…tion

- rename two Convey blocks so names match the mocked failing API call
- assert org-forbidden case does not include "MyOrg" (real org group) instead
  of "testOrg"

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
Use MyOrg consistently across mocked /user/orgs and /user/teams payloads in the
same success scenario, and align expected team-derived group assertions.

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
… group checks

Use MyOrg in the mocked /user/teams payload for the ListOrgs-forbidden
scenario and assert MyOrg/infra accordingly to keep test casing semantics
consistent.

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
Align negative team-group assertion with MyOrg casing used by org mocks and
other case-sensitive authz group checks.

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
Use consistent org casing in the README example (myorg -> myorg/infra) to
reflect that group strings follow GitHub login values and are not lowercased by
zot.

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
@rchincha
rchincha force-pushed the nForced-uk-feature-github-teams branch from 393af95 to 755e95c Compare June 17, 2026 06:23
@rchincha
rchincha marked this pull request as ready for review June 17, 2026 16:55
@rchincha
rchincha requested a review from andaaron as a code owner June 17, 2026 16:55
@rchincha
rchincha requested a review from Copilot June 17, 2026 16:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread pkg/api/authn.go
Comment thread examples/README.md
Document that org/team group strings use GitHub login/slug casing as-is (no
normalization), so policy entries must match exact case.

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread pkg/api/authn.go
Log the underlying error and use an operation-accurate message when
client.Users.ListEmails fails in GetGithubUserInfo.

Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@rchincha
rchincha merged commit 43a5f15 into project-zot:main Jun 17, 2026
43 of 45 checks passed
@Caffe1neAdd1ct

Copy link
Copy Markdown
Contributor

thanks @rchincha i was typing a response about the DCO signed commits and rebasing the branch as you've merged.

Great news i'll take a copy of main branch tomorrow and try it out.

Thanks very much for taking this one forward, really appreciate it. Time this side has been limited since our new arrival..

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.

4 participants