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

feature/db/sub_repo_perms: add IPs column to sub repo perms#63771

Merged
ggilmore merged 3 commits into
mainfrom
graphite-ggilmorefeature_db_sub_repo_perms_add_ips_column_to_sub_repo_perms
Jul 18, 2024
Merged

feature/db/sub_repo_perms: add IPs column to sub repo perms#63771
ggilmore merged 3 commits into
mainfrom
graphite-ggilmorefeature_db_sub_repo_perms_add_ips_column_to_sub_repo_perms

Conversation

@ggilmore

@ggilmore ggilmore commented Jul 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR modifies the sub_repo_permissions database to store the ip addresses associated with each Perforce path rule, as part of the IP based sub repo permissions work.

The new IP column is implemetned as a []text similar to the path column. The IP address associated with paths[0] is stored in the ips column in ips[0].

For example, the follownig proections table

Protections:
    read     user     emily   *                    //depot/elm_proj/...
    write    group    devgrp  *                    //...
    write    user     *       192.168.41.0/24     -//...
    write    user     *       [2001:db8:1:2::]/64 -//...
    write    user     joe     *                   -//...
    write    user     lisag   *                   -//depot/...
    write    user     lisag   *                    //depot/doc/...
    super    user     edk     *                    //...

turns into the following rows in the sub_repo_permissions table

repo_id user_id version updated_at paths ips
1 1 1 2023-07-01 10:00:00 {"//depot/elm_proj/..."} {"*"}
1 2 1 2023-07-01 10:00:00 {"//..."} {"*"}
1 3 1 2023-07-01 10:00:00 {"-//..."} {"192.168.41.0/24"}
1 4 1 2023-07-01 10:00:00 {"-//..."} {"2001:db8:1:2::]/64"}
1 5 1 2023-07-01 10:00:00 {"-//..."} {"*"}
1 6 1 2023-07-01 10:00:00 {"-//depot/...", "//depot/doc/..."} {"*", "*"}
1 7 1 2023-07-01 10:00:00 {"//..."} {"*"}

Test plan

The unit test for the sub_repository_permissions store PR that is built on this PR: https://app.graphite.dev/github/pr/sourcegraph/sourcegraph/63811/internal-database-sub_repo_permissions-modify-store-to-be-able-to-insert-ip-based-permissions

Changelog

  • The sub_repo_permissions table now has an ips column to store the associated IP address associated with each path rule.

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

ggilmore commented Jul 10, 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

@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 10, 2024
@ggilmore ggilmore force-pushed the graphite-ggilmorefeature_db_sub_repo_perms_add_ips_column_to_sub_repo_perms branch from 4957d56 to ef0c925 Compare July 11, 2024 20:42
@ggilmore ggilmore force-pushed the graphite-ggilmorefeature_db_sub_repo_perms_add_ips_column_to_sub_repo_perms branch from ef0c925 to 930b4a7 Compare July 12, 2024 23:32
@ggilmore ggilmore mentioned this pull request Jul 12, 2024
Merged
ggilmore
ggilmore commented Jul 16, 2024
View reviewed changes
Comment thread internal/database/schema.md
"sub_repo_permissions_repo_id_user_id_version_uindex" UNIQUE, btree (repo_id, user_id, version)
"sub_repo_perms_user_id" btree (user_id)
Check constraints:
"ips_paths_length_check" CHECK (ips IS NULL OR array_length(ips, 1) = array_length(paths, 1) AND NOT (''::text = ANY (ips)))

@ggilmore ggilmore Jul 16, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This ensures

  • that we have an IP address associated with each path entry, or no ips at all (if the perforce permissions syncer hasn't been updated yet to save those)
  • that we have no blank IP address entries (this doesn't seem to be possible in perforce, so there is no reason to allow it here either)

@ggilmore ggilmore requested a review from a team July 16, 2024 00:53
@ggilmore ggilmore marked this pull request as ready for review July 16, 2024 00:53
eseliger
eseliger reviewed Jul 16, 2024
View reviewed changes

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needs a test plan, the auditor wrongly marked this as ok.

Comment thread migrations/frontend/1720651147_add_ips_to_sub_repo_perms_table/up.sql Outdated
Comment thread migrations/frontend/1720651147_add_ips_to_sub_repo_perms_table/down.sql Outdated
@ggilmore ggilmore marked this pull request as draft July 16, 2024 19:17
@ggilmore ggilmore marked this pull request as ready for review July 16, 2024 20:25
@ggilmore ggilmore requested review from a team and eseliger July 16, 2024 20:53
@ggilmore ggilmore merged commit 14137ed into main Jul 18, 2024
@ggilmore Graphite App

ggilmore commented Jul 18, 2024

Copy link
Copy Markdown
Contributor Author

Merge activity

@ggilmore ggilmore deleted the graphite-ggilmorefeature_db_sub_repo_perms_add_ips_column_to_sub_repo_perms branch July 18, 2024 19:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@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.

2 participants

@ggilmore @eseliger