authz/github: repo-centric perms sync from team/org perms caches #24328

bobheadxi · 2021-08-25T20:44:40Z

Implements caching of groups permissions for repo-centric permissions sync. Follows up on the GitHub user-centric perms sync caching introduced in #23978 - see that PR for more information.

Repo sync now lists direct collaborators to a repo before querying for the organization and teams for users with access to this repo, and caching them as groups.

Group caches now includes both repositories and users. In general, the expectation is that:

User-centric sync will use and populate only Repositories in the cache
Repo-centric sync will use and populate only Users in the cache
If either comes across a prepopulated value for a field it is not meant to populate, it will attempt to add itself to the list and update the cache

Token-wise, verified that all this needs is repo and read:org

To review

Bulk of real code is in authz/github/github.go
Some API requests might be worth reading in internal/extsvc/github/v3.go
Expanded integration tests in enterprise/cmd/repo-updater/internal/authz/integration_test.go
- The recorded interactions here make it pretty visible how inefficient this is compared to just listing for users * repos < 5000 * 100 - in most smaller cases it is better to use the cacheless implementation. An argument for making this opt-in only.

Subsequent patches

(if we cut a patch release with this, we must include the following as well)

authz/github: fixes for oauth scopes check #24471

bobheadxi · 2021-08-25T20:45:33Z

internal/authz/github/cache.go

+	// Repositories entities associated with this group has access to.
+	//
+	// This should ONLY be populated on a USER-centric sync, but may be appended to if
+	// already populated.
 	Repositories []extsvc.RepoID
+	// Users associated with this group
+	//
+	// This should ONLY be populated on a REPO-centric sync, but maybe to appended to if
+	// already populated.
+	Users []extsvc.AccountID


cache values might get very large

there might be a better way to do this, but I'm opting to focus on getting the functionality right and improving the implementation in a separate pass

…github-repo-perms-caching

unknwon · 2021-08-26T07:57:41Z

internal/authz/github/cache.go

+	group.Repositories = nil
+	group.Users = nil


Doing in-place modification within a method call for a passed-in object to abandon it feels too magic in the long term, especially when the modification is not doing cache invalidation (ie. we are not saving en empty object but delete the key entirely).

Would improved docstring help, or rename this to invalidateGroup?

Couldn't we just stop using the group object all the call sites? I mean cachedGroups is and should ideally only purely for cache management.

Using group makes dealing with this a lot more convenient I think, and helps reduce a lot of repetition and things that can go wrong (e.g. forgetting to set Repos and Users or creating a new Group when invalidating a cache)

cachedGroups is and should ideally only purely for cache management.

What I'm thinking here is ensuring the cache values being used in the code is valid also falls under pure cache management 🤔

Let's improve the docstring then, not sure how we could improve elsewhere top off my head.

How does 471682e look?

internal/authz/github/client.go

unknwon · 2021-08-26T08:00:05Z

internal/authz/github/github.go

+		return userIDs, nil
+	}
+
+	// Check if repo belongs in an org


Could we also factor rest of the body into a helper method/function?

74b953d starts this, and also makes the code mirror the user perms syncing a bit more

…github-repo-perms-caching

sourcegraph-bot · 2021-08-26T22:31:32Z

Notifying subscribers in CODENOTIFY files for diff 82d260b...5af46f9.

Notify	File(s)
@christinaforney	doc/admin/external_service/github.md doc/admin/repo/permissions.md
@eseliger	internal/extsvc/github/common.go internal/extsvc/github/common_test.go internal/extsvc/github/testdata/golden/ListRepositoryTeams internal/extsvc/github/testdata/vcr/GetOrganization.yaml internal/extsvc/github/testdata/vcr/ListRepositoryCollaborators_direct-collaborator-outside-collaborator.yaml internal/extsvc/github/testdata/vcr/ListRepositoryCollaborators_direct-collaborator-repo-owner.yaml internal/extsvc/github/testdata/vcr/ListRepositoryCollaborators_private-repo.yaml internal/extsvc/github/testdata/vcr/ListRepositoryCollaborators_public-repo.yaml internal/extsvc/github/testdata/vcr/ListRepositoryTeams.yaml internal/extsvc/github/testdata/vcr/TestListMembers/org_admins.yaml internal/extsvc/github/testdata/vcr/TestListMembers/org_members.yaml internal/extsvc/github/testdata/vcr/TestListMembers/team_members.yaml internal/extsvc/github/v3.go internal/extsvc/github/v3_test.go
@indradhanush	enterprise/cmd/repo-updater/internal/authz/integration_test.go enterprise/cmd/repo-updater/internal/authz/testdata/vcr/Integration_GitHubPermissions.yaml enterprise/cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/repo-centric/groups-enabled.yaml enterprise/cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/repo-centric/no-groups.yaml enterprise/cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/user-centric/groups-enabled.yaml enterprise/cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/user-centric/no-groups.yaml
@sourcegraph/distribution	doc/admin/external_service/github.md doc/admin/repo/permissions.md
@unknwon	enterprise/cmd/frontend/internal/auth/githuboauth/provider.go enterprise/cmd/repo-updater/internal/authz/integration_test.go enterprise/cmd/repo-updater/internal/authz/testdata/vcr/Integration_GitHubPermissions.yaml enterprise/cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/repo-centric/groups-enabled.yaml enterprise/cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/repo-centric/no-groups.yaml enterprise/cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/user-centric/groups-enabled.yaml enterprise/cmd/repo-updater/internal/authz/testdata/vcr/TestIntegration_GitHubPermissions/user-centric/no-groups.yaml internal/authz/github/cache.go internal/authz/github/client.go internal/authz/github/github.go internal/authz/github/github_test.go

internal/authz/github/client.go

unknwon · 2021-08-30T15:51:47Z

internal/authz/github/github.go

+			// Just use cache if available and not invalidated
+			addUserToRepoPerms(group.Users...)
+			// Perform partial cache update to repositories iff non-empty
+			if len(group.Repositories) > 0 {


Pretty sure this if is redundant, it is OK to loop over a slice with zero element.

I had to change this around a bit, after testing (7a90087) I realized this wasn't behaving correctly. The code now looks like this:

sourcegraph/internal/authz/github/github.go

Lines 178 to 191 in 555d34f

// If this is a partial cache, add self to group

if len(group.Users) > 0 {

hasUser := false

for _, user := range group.Users {

if user == accountID {

hasUser = true

break

}

}

if !hasUser {

group.Users = append(group.Users, accountID)

p.groupsCache.setGroup(group)

}

}

Since the whole block is unnecessary if the field being checked for is empty, the check is still useful in denoting that

Co-authored-by: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>

…github-repo-perms-caching

enterprise/cmd/repo-updater/internal/authz/integration_test.go

Co-authored-by: Geoffrey Gilmore <geoffrey@sourcegraph.com>

) Implements caching of groups permissions for repo-centric permissions sync. Follows up on the GitHub user-centric perms sync caching introduced in #23978. Repo sync now lists direct collaborators to a repo before querying for the organization and teams for users with access to this repo, and caching them as groups. Also adds `allowGroupsPermissionsSync` to the GitHub OAuth provider config, which requests the additional `read:repo` scope required to enable `authorization.groupsCacheTTL` in GitHub code hosts. Co-authored-by: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com> Co-authored-by: Geoffrey Gilmore <geoffrey@sourcegraph.com>

… enabled (#24561) allowGroupsPermissionsSync, introduced in #24328, is actually a prerequisite to enabling groupsCacheTTL. This change checks if allowGroupsPermissionsSync is enabled, and if not, forcibly sets groupsCacheTTL to nil and reports a warning. This is a breaking change, but it is currently flagged as an experimental feature and opt-in only, so will stick to a changelog item.

authz/github: first-pass repo perms syncing

6e5ed40

bobheadxi commented Aug 25, 2021

View reviewed changes

bobheadxi requested review from davejrt and ggilmore August 25, 2021 20:46

Merge branch 'main' of github.com:sourcegraph/sourcegraph into authz/…

b885429

…github-repo-perms-caching

bobheadxi requested a review from unknwon August 25, 2021 21:43

unknwon reviewed Aug 26, 2021

View reviewed changes

bobheadxi mentioned this pull request Aug 26, 2021

doc: add guidance about GitHub token permissions for caching #24332

Merged

bobheadxi changed the title ~~authz/github: repo perms syncing~~ authz/github: improved repo-centric perms sync from teams/orgs perms caches Aug 26, 2021

bobheadxi changed the title ~~authz/github: improved repo-centric perms sync from teams/orgs perms caches~~ authz/github: faster repo-centric perms sync from teams/orgs perms caches Aug 26, 2021

bobheadxi mentioned this pull request Aug 26, 2021

authz/github: user-centric perms sync from team/org perms caches #23978

Merged

4 tasks

Merge branch 'main' of github.com:sourcegraph/sourcegraph into authz/…

fea611c

…github-repo-perms-caching

bobheadxi changed the title ~~authz/github: faster repo-centric perms sync from teams/orgs perms caches~~ authz/github: faster repo-centric perms sync from team/org perms caches Aug 26, 2021

bobheadxi added 6 commits August 26, 2021 09:26

invalidateGroup to indicate expanded functionality

471682e

extract getRepoAffiliatedGroups

74b953d

remove unused isPopulated

cac7df3

test and fix affiliated collaborator listing

65f583c

update and expand integration tests

7684aba

updated tests

e4136b4

bobheadxi force-pushed the authz/github-repo-perms-caching branch from 0dc737d to e4136b4 Compare August 26, 2021 18:58

bobheadxi added 2 commits August 26, 2021 15:17

add cache-and-invalidate

7d3605c

add groups-enabled integration test

69b10f2

bobheadxi force-pushed the authz/github-repo-perms-caching branch from d9f2c29 to 69b10f2 Compare August 26, 2021 19:37

bobheadxi added 2 commits August 26, 2021 18:13

account for org admins, test membership listing

af1b34d

Merge branch 'main' of github.com:sourcegraph/sourcegraph into authz/…

b230dbc

…github-repo-perms-caching

bobheadxi marked this pull request as ready for review August 26, 2021 22:30

bobheadxi requested a review from unknwon August 26, 2021 22:30

bobheadxi changed the title ~~authz/github: faster repo-centric perms sync from team/org perms caches~~ authz/github: repo-centric perms sync from team/org perms caches Aug 26, 2021

run test with reduced-scope auth

c77f776

bobheadxi added 4 commits August 30, 2021 10:29

cache disabled

30a68ca

add comments to test expected members

1c7a245

add repoAffiliatedGroup struct

788243d

add security notes

68f4b3e

bobheadxi force-pushed the authz/github-repo-perms-caching branch from d127b34 to 68f4b3e Compare August 30, 2021 14:44

bobheadxi added 4 commits August 30, 2021 10:44

fix docstring

f8a7190

simplify partial cache update

57bbc9d

minor: improve symmetry of direct perms sync

c9505c5

generate

9104b1a

unknwon approved these changes Aug 30, 2021

View reviewed changes

bobheadxi and others added 7 commits August 30, 2021 14:44

typo

68d3680

Co-authored-by: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>

partially revert 57bbc9d, unnest partial update

c511471

add simple test cases for partial cache updates

7a90087

fixup partial cache updates

555d34f

Merge branch 'main' of github.com:sourcegraph/sourcegraph into authz/…

3f4d3ba

…github-repo-perms-caching

add changelog item for allowGroupsPermissionsSync

8ace2cd

clean up scopes checking and add test

ee2a75e

bobheadxi requested a review from a team August 30, 2021 22:00

ggilmore reviewed Aug 30, 2021

View reviewed changes

enterprise/cmd/repo-updater/internal/authz/integration_test.go Outdated Show resolved Hide resolved

doc typo

5af46f9

Co-authored-by: Geoffrey Gilmore <geoffrey@sourcegraph.com>

bobheadxi merged commit fa91928 into main Aug 31, 2021

bobheadxi deleted the authz/github-repo-perms-caching branch August 31, 2021 15:47

This was referenced Aug 31, 2021

Large-scale GitHub perms sync test environment #24470

Closed

authz/github: fixes for oauth scopes check #24471

Merged

3.31.1 patch request for permissions sync commits #24513

Closed

bobheadxi self-assigned this Sep 1, 2021

This was referenced Sep 2, 2021

authz/github: enforce allowGroupsPermissionsSync if groupsCacheTTL is enabled #24561

Merged

authz/github: do not recommend groupsCacheTTL, document allowGroupsPermissionsSync #24528

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

authz/github: repo-centric perms sync from team/org perms caches #24328

authz/github: repo-centric perms sync from team/org perms caches #24328

bobheadxi commented Aug 25, 2021 •

edited

bobheadxi Aug 25, 2021

unknwon Aug 26, 2021

bobheadxi Aug 26, 2021

unknwon Aug 26, 2021

bobheadxi Aug 26, 2021

unknwon Aug 26, 2021

bobheadxi Aug 26, 2021

unknwon Aug 26, 2021

bobheadxi Aug 26, 2021

sourcegraph-bot commented Aug 26, 2021 •

edited

unknwon Aug 30, 2021

bobheadxi Aug 30, 2021

	// If this is a partial cache, add self to group
	if len(group.Users) > 0 {
	hasUser := false
	for _, user := range group.Users {
	if user == accountID {
	hasUser = true
	break
	}
	}
	if !hasUser {
	group.Users = append(group.Users, accountID)
	p.groupsCache.setGroup(group)
	}
	}

authz/github: repo-centric perms sync from team/org perms caches #24328

authz/github: repo-centric perms sync from team/org perms caches #24328

Conversation

bobheadxi commented Aug 25, 2021 • edited

To review

Subsequent patches

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcegraph-bot commented Aug 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobheadxi commented Aug 25, 2021 •

edited

sourcegraph-bot commented Aug 26, 2021 •

edited