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

Commit 57de59c

Browse files
authored
internal/database/sub_repo_permissions: modify store to be able to insert ip based permissions (#63811)
Closes https://linear.app/sourcegraph/issue/SRC-459/ Closes This PR adds support for saving and retreiving the IP addressess associated with each path rule in the sub_repo_permissions store. It does this by: **Adding a new permissions type to the internal/authz package**: https://github.com/sourcegraph/sourcegraph/blob/1be7df6d79c96017b9a49c289038cf59cb4a8eb2/internal/authz/iface.go#L52-L96 **Adding new `*WithIPs` versions of all the setter and getter methods** The new setter methods uses the above `authz.SubRepoPermissionsWithIPs` type that write to the appropriate `ips` column in the DB. The new getter methods retrieve the ip addresses associated with each path entry. However, here there is an additional complication: It's possible for someone to call the `*WithIPs` getters when the ips column is still NULL (indicating that the perforce syncer hasn't been updated / ran in order to save the IP addresses from the protection table yet. | repo_id | user_id | version | updated_at | paths | ips | |---------|---------|---------|------------|-------|-----| | 1 | 1 | 1 | 2023-07-01 10:00:00 | {`"/depot/main/..."`, `"/depot/dev/..."`, `"-/depot/secret/..."`} | NULL | | 2 | 1 | 1 | 2023-07-01 11:00:00 | {`"/depot/public/..."`, `"-/depot/private/..."`} | NULL | In order to address this, the getters each have a `backfill` boolean that allows the caller to choose the behavior that they want. - If `backfill = true`, the paths without IP entries will be returned with a `*` (wildcard) IP indicating that any client IP address is okay. (This is effectively the behavior we have today since we don't check IPs for sub_repo_permisisons). I imagine this can be used when callers don't care about enforcing IP-based permissions (such as when IP address enforcement is disabled in site configuration). - If `backfill = false`, if the IPs column is NULL - an error is returned instead of backfilling ("The IP addresses associated with this sub-repository-permissions entry have not been synced yet."). This allows for callers that care about IP address enforcement to know _explicitly_ if the IP address information hasn't been updated yet - so we can't know whether or not the user is able to view the file (e.g when IP based enforcement is enabled). **Ensuring that the old setter methods set the IPs column to NULL**: self-explanatory, if someone uses the non `*WithIP` variants of the setters, we want to ensure that we zero out that column so that we don't leave stale / inconsistent information for those Path entries. --- Overall, the design this adds the new IP address functionality without having to immediately update all the call sites in the codebase to force them to interpret all this information (which would make for a gargantuan PR). Eventually, we should be able to simply delete the old versions of the setters/getters once the IP address functioanlity has been threaded through everywhere. ## Test plan Extensive unit tests. For each new setter and getter, I added unit tests that tested along all of the following dimenisons: - **initial store state**: empty database, database seeded with permissions with no IP information (paths column only), database seeded with permissions that have the IP information synced - **insertion method**: was the data for the test inserted **with IP information** (using the `withIP` variant of upsert, etc.), or was it inserted with the old legacy way with no ip information - **retreieval method**: was the data reterived with the legacy getters (that don't look at the IP information), with the new IP getters that either backfill (if the IP information for that paths entry hasn't been synced yet, it will return an `*` for that entry), or avoids backfilling (will return the information in the IPs column, or hard-error)? ## Changelog - The sub_repository_permissions_ database store can now save and retrieve the IP addresses associated with each path rule.
1 parent 12570e4 commit 57de59c

File tree

4 files changed

+2491
-4
lines changed

4 files changed

+2491
-4
lines changed

internal/authz/iface.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,52 @@ type SubRepoPermissions struct {
4949
Paths []string
5050
}
5151

52+
// SubRepoPermissionsWithIPs denotes access control rules within a repository's
53+
// contents.
54+
type SubRepoPermissionsWithIPs struct {
55+
Paths []PathWithIP
56+
}
57+
58+
// PathWithIP denotes a rule associated with the given file path and range of IP addresses that
59+
// this rule applies to.
60+
//
61+
// Rules are expressed as Glob syntaxes:
62+
//
63+
// pattern:
64+
// { term }
65+
//
66+
// term:
67+
// `*` matches any sequence of non-separator characters
68+
// `**` matches any sequence of characters
69+
// `?` matches any single non-separator character
70+
// `[` [ `!` ] { character-range } `]`
71+
// character class (must be non-empty)
72+
// `{` pattern-list `}`
73+
// pattern alternatives
74+
// c matches character c (c != `*`, `**`, `?`, `\`, `[`, `{`, `}`)
75+
// `\` c matches character c
76+
//
77+
// character-range:
78+
// c matches character c (c != `\\`, `-`, `]`)
79+
// `\` c matches character c
80+
// lo `-` hi matches character c for lo <= c <= hi
81+
//
82+
// pattern-list:
83+
// pattern { `,` pattern }
84+
// comma-separated (without spaces) patterns
85+
//
86+
// This Glob syntax is currently from github.com/gobwas/glob:
87+
// https://sourcegraph.com/github.com/gobwas/glob@e7a84e9525fe90abcda167b604e483cc959ad4aa/-/blob/glob.go?L39:6
88+
//
89+
// We use a third party library for double-wildcard support, which the standard
90+
// library does not provide.
91+
//
92+
// Paths are relative to the root of the repo.
93+
type PathWithIP struct {
94+
Path string
95+
IP string
96+
}
97+
5298
// ExternalUserPermissions is a collection of accessible repository/project IDs
5399
// (on the code host). It contains exact IDs, as well as prefixes to both include
54100
// and exclude IDs.

0 commit comments

Comments
 (0)