[fga] added create_snapshot permission #18559
Conversation
svenefftinge
force-pushed
the
se/create-snapshot
branch
from
7292b9b
to
c5f9575
Compare
| @@ -45,6 +45,9 @@ schema: |- | |||
| relation member: user | |||
| // Some users in an organization may additionally have the `owner` role | |||
| relation owner: user | |||
| // users who can snapshot workspaces | |||
| relation snapshoter: user | organization#member | |||
There was a problem hiding this comment.
I'm not sure I understand the intend behind this relation, especially how it is intended to be expanded upon in the future.
The permission create_snapshot itself is clear: Owners can take snapshots, if the org allows them to.
The I would expect the relation to either look like:
- for making it an explicit role on the org which has to be added :
relation snapshoter: organization#memberthis would need to be added to every member, thoughah, TIL that this can reference the membership of another group 💡 Then this is probably what we want. 🙃
- for making it a toggle:
relation snapshoter: user:*- in this case only the
owner &ensure's that only the owner can do it. I'd rename it to sth likeenable_snapshotting
- in this case only the
There was a problem hiding this comment.
This allows to enable/disable snapshotting on org level, by setting the organization#membership relation every member can snapshot their workspaces.
There was a problem hiding this comment.
it is not used indeed but since we allow assigning roles to users everywhere I thought I'd do it here as well for consistency reasons. I can remove it if you like that better also because YAGNI
There was a problem hiding this comment.
I can remove it if you like that better also because YAGNI
👍
| if (!workspace || workspace.ownerId !== user.id) { | ||
| // we use the workspacService which checks if the requesting user has access to the workspace. If that is the case they have access to snapshots as well. | ||
| // below is the old permission check which would also check if the user has access to the snapshot itself. This is not the case anymore. | ||
| const workspace = await this.workspaceService.getWorkspace(user.id, workspaceId); |
There was a problem hiding this comment.
Would be awesome to have a test for this 🧡
There was a problem hiding this comment.
What should I test here exactly?
There was a problem hiding this comment.
Create a workspace from a snapshot context, to validate the fact that all users (incl member + strangers) can create workspace from a Snapshot.
There was a problem hiding this comment.
Since you know how painful it would be to write a test against gitpod-server-iml. Do you suggest to extract this logic into a service and write a test?
There was a problem hiding this comment.
LGTM, tested and works ✔️
a nit on clarity: https://github.com/gitpod-io/gitpod/pull/18559/files#r1302563299
svenefftinge
force-pushed
the
se/create-snapshot
branch
from
c5f9575
to
5805b65
Compare
|
/unhold |
Description
This pull request adds a new feature to enable users to create snapshots of workspaces in an organization. It defines new relations and permissions in the
definitions.tsandschema.yamlfiles, and implements the authorization logic in theauthorizer.tsandgitpod-server-impl.tsfiles.Related Issue(s)
Fixes EXP-348
How to test
Documentation
Preview status
Gitpod was successfully deployed to your preview environment.
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh. If enabled,with-previewandwith-large-vmwill be enabled./hold