Skip to content

org base permission is none, not read#178

Merged
kans merged 1 commit into
mainfrom
kans/dont-default-to-org-read-perms
Jul 22, 2026
Merged

org base permission is none, not read#178
kans merged 1 commit into
mainfrom
kans/dont-default-to-org-read-perms

Conversation

@kans

@kans kans commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Fixes CXH-2100

This will drop 10s of millions of grants from some syncs.

Summary

  • Fix false-positive repository pull grants under direct-collaborators-only: when GET /orgs/{org} omits default_repository_permission (common without org-owner / admin:org / Organization Administration visibility), stop inventing "read" and treat it as "none" instead.
  • That fallback was expanding every org member onto every repo's pull entitlement via GrantExpandable, even when GitHub effective permission is "none".
  • Warn when the field is missing so we can see that member→repo base-permission expansion was skipped.
  • Org-admin → repo expansion is unchanged (owners still get admin on all repos).

Context

Reported on DoorDash (github_v2): C1 showed repo pull inherited from org membership for ~all members (~7.6k), while GET /repos/.../collaborators/{user}/permission returned "none". Root cause is the empty→"read" default in getOrgBasePermission.

@kans
kans requested a review from a team July 21, 2026 18:01
@linear-code

linear-code Bot commented Jul 21, 2026

Copy link
Copy Markdown

CXH-2100

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: org base permission is none, not read

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 93f61808e090.
Review mode: full
View review run

Review Summary
Scanned the full PR diff for security and correctness. This changes getOrgBasePermission to fail closed: an empty or absent default_repository_permission now maps to none instead of read, preventing false GrantExpandable pull grants for every org member on every repo under direct-collaborators-only. The fix is correct and well-covered: orgBasePermissionToRepoPermissions returns nil for both none and empty string, and the new tests exercise the empty (fail-closed) and explicit read (still-expands) paths end-to-end. zap and ctxzap are already imported, the Debug log level suits a missing-but-expected value cached per-org, and org-admin expansion is untouched. No new issues found.

Security Issues
None found.

Correctness Issues
None found.

Suggestions
None.

@github-actions github-actions Bot 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.

No blocking issues found.

@kans
kans requested review from btipling and laurenleach July 21, 2026 20:02
Comment thread pkg/connector/repository.go Outdated
perm := org.GetDefaultRepoPermission()
if perm == "" {
perm = readConst // GitHub default
l.Warn(

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.

nit (non-blocking): I'd drop this to Debug. An empty default_repository_permission here isn't an error or a connector bug — it just means the credential lacks org-owner visibility, which is a normal, expected setup for a lot of tokens. We handle it gracefully (fail closed to none) and move on, and since it's cached per org it only fires once per sync. Logging that at Warn puts an expected, non-actionable condition into warning dashboards / retained OTEL spans as if something were wrong; Debug keeps the signal without the noise. Fine to keep if you want the higher visibility, just flagging it.

Comment thread pkg/connector/repository_test.go
@kans
kans force-pushed the kans/dont-default-to-org-read-perms branch from df6dd8c to 3ed5949 Compare July 22, 2026 15:02
@kans
kans merged commit 7b8e756 into main Jul 22, 2026
9 checks passed

@github-actions github-actions Bot 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.

No blocking issues found.

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.

3 participants