Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feat/worker/permission syncing: make sub repo permissions re-insertion fall back to original paths if ips not added yet#64086

Merged
ggilmore merged 1 commit into
mainfrom
graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet
Jul 30, 2024
Merged

feat/worker/permission syncing: make sub repo permissions re-insertion fall back to original paths if ips not added yet#64086
ggilmore merged 1 commit into
mainfrom
graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet

Conversation

@ggilmore

@ggilmore ggilmore commented Jul 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes https://linear.app/sourcegraph/issue/SRC-453/modify-the-perforce-authorization-provider-to-support-ip-aware-sub

This PR builds on https://github.com/sourcegraph/sourcegraph/pull/64010, and enhances the re-insertion logic.

Before, we'd fail the sync operation outright if we tried to re-insert existing permissions that hadn't been converted from the path only form to the (ip, path) tuple.

Now, when trying to reinsert the existing permissions:

  • if it has already been converted ((path, ip)) -> we save it back in the database using the UpsertWithUP() method
  • if it hasn't been converted (path only) -> we save it back in the databse using the existing old plain Upsert method

I accomplish this by using a small interface that encpasulates the data and the insertion logic to use:

type subRepoPermissionsUpserter interface {
	// UpsertWithSpec inserts or updates the sub-repository permissions with the data
	// stored in the upserter.
	UpsertWithSpec(ctx context.Context, store database.SubRepoPermsStore, userID int32, spec api.ExternalRepoSpec) error
}

type ipBasedPermissions struct {
	perms *authz.SubRepoPermissionsWithIPs
}

func (u *ipBasedPermissions) UpsertWithSpec(ctx context.Context, store database.SubRepoPermsStore, userID int32, spec api.ExternalRepoSpec) error {
	return store.UpsertWithSpecWithIPs(ctx, userID, spec, *u.perms)
}

type pathBasedPermissions struct {
	perms *authz.SubRepoPermissions
}

func (u *pathBasedPermissions) UpsertWithSpec(ctx context.Context, store database.SubRepoPermsStore, userID int32, spec api.ExternalRepoSpec) error {
	return store.UpsertWithSpec(ctx, userID, spec, *u.perms)
}

var (
	_ subRepoPermissionsUpserter = &ipBasedPermissions{}
	_ subRepoPermissionsUpserter = &pathBasedPermissions{}
)

Now, any code that deals with inserting sub repo permissions now can deal with a []subRepoPermissionsUpserter without having to care about the exact semantics to use. Our re-insertion logic is also now more robust.

Test plan

New unit tests

Changelog

@cla-bot cla-bot Bot added the cla-signed label Jul 25, 2024
@ggilmore Graphite App

ggilmore commented Jul 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ggilmore and the rest of your teammates on Graphite Graphite

@ggilmore ggilmore mentioned this pull request Jul 25, 2024
Merged
@github-actions github-actions Bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Jul 25, 2024
@ggilmore ggilmore force-pushed the graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet branch 2 times, most recently from 904ef64 to 221ce19 Compare July 26, 2024 09:27
@ggilmore ggilmore force-pushed the graphite-ggilmorewip_sync_p4_ip branch from e0b1725 to 2ef6fa7 Compare July 26, 2024 09:42
@ggilmore ggilmore force-pushed the graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet branch from 221ce19 to 5e186ea Compare July 26, 2024 09:42
@ggilmore ggilmore force-pushed the graphite-ggilmorewip_sync_p4_ip branch from 2ef6fa7 to 115ddad Compare July 26, 2024 09:44
@ggilmore ggilmore force-pushed the graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet branch from 5e186ea to 49c2623 Compare July 26, 2024 09:44
@ggilmore ggilmore force-pushed the graphite-ggilmorewip_sync_p4_ip branch from 115ddad to 4ddb9ff Compare July 26, 2024 09:48
@ggilmore ggilmore force-pushed the graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet branch 3 times, most recently from bbdb954 to f25e97d Compare July 28, 2024 21:04
@ggilmore ggilmore marked this pull request as ready for review July 29, 2024 08:39
@ggilmore ggilmore requested a review from a team July 29, 2024 08:39
@ggilmore ggilmore force-pushed the graphite-ggilmorewip_sync_p4_ip branch from 4ddb9ff to 9610c24 Compare July 29, 2024 17:21
@ggilmore ggilmore force-pushed the graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet branch from f25e97d to 9e26623 Compare July 29, 2024 17:21
@ggilmore ggilmore mentioned this pull request Jul 29, 2024
Merged
@ggilmore ggilmore force-pushed the graphite-ggilmorewip_sync_p4_ip branch from 9610c24 to 86e4045 Compare July 29, 2024 22:11
@ggilmore ggilmore force-pushed the graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet branch from 9e26623 to 62d9ad8 Compare July 29, 2024 22:11
@ggilmore Graphite App

ggilmore commented Jul 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Jul 30, 10:40 AM EDT: @ggilmore started a stack merge that includes this pull request via Graphite.
  • Jul 30, 10:43 AM EDT: Graphite rebased this pull request as part of a merge.
  • Jul 30, 11:07 AM EDT: @ggilmore merged this pull request with Graphite.

@ggilmore ggilmore changed the base branch from graphite-ggilmorewip_sync_p4_ip to graphite-base/64086 July 30, 2024 14:41
ggilmore referenced this pull request Jul 30, 2024
@ggilmore
feature/worker/permission syncer: perforce: sync HOST field using IP …
70b31c9 
…addresses (#64010)

The perforce permission syncer has been adapted to now read and save the HOST field from the perforce protections table, which contains the IP address(es) that the path rule that it applies to.  It uses the updated sub_repository_rules store methods introduced in https://github.com/sourcegraph/sourcegraph/pull/63811/.

### Notes

- There is some existing logic in the permissions syncer that attempts to re-insert the existing sub_repo_permissions if we encounter a temporary (timeout, etc.) error when syncing. However, there is an edge case: what do we do if the existing permissions don't have an IP address associated with them yet (they were inserted before the updated permission syncer ran)? For simplicity, in this PR I leaned towawrds correctness - I fail the operation outright (I'd rather temporarirly lock someone out rather than accidentally leak information). I implemented a more robust straetgy for this in https://github.com/sourcegraph/sourcegraph/pull/64086. 


## Test plan

- The existing unit tests have been adapted to use the new authz.SubRepoPermissionsWithIP structs (I use wildcard IP addresses). 


- The big new test to pay attention to is TestScanIPPermissions (and the associated `sample-protects-ip.txt` file). 

## Changelog

- The perforce permissions syncer has been updated to save the IP address associated with each sub_repository_permissions rule.
@ggilmore ggilmore changed the base branch from graphite-base/64086 to main July 30, 2024 14:41
@ggilmore
feat/worker/permission syncing: make sub repo permissions re-insertio…
fbdf7d2 
…n fall back to original paths if ips not added yet
@ggilmore ggilmore force-pushed the graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet branch from 62d9ad8 to fbdf7d2 Compare July 30, 2024 14:42
@ggilmore ggilmore merged commit e74a4d4 into main Jul 30, 2024
@ggilmore ggilmore deleted the graphite-ggilmorefeat_worker_permission_syncing_make_sub_repo_permissions_re-insertion_fall_back_to_original_paths_if_ips_not_added_yet branch July 30, 2024 15:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@pjlast pjlast pjlast approved these changes

@eseliger eseliger eseliger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ggilmore @pjlast @eseliger