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**:
1be7df6d79/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.
57de59cb3c