[server] Perform authorization checks for Orgs against spicedb #16207

easyCZ · 2023-02-03T16:49:23Z

Description

Behind an experiment, performs authorization checks for Organizaitons against SpiceDB, and reports the outcome. Whether the outcome matches, or not, we always return the result of the existing authorization check.

Effectively, this change allows us to being checking perms against spicedb, without affecting the system, only collecting data.

We do not yet create any relationships against spicedb. As a result, all of these checks will return "NO_PERMISSION" initially. That's desired. We want to have the checks in place, and incrementally start adding these relationships, either from a batch job, or when resources are actually created. This will be done in subsequent PR(s).

Related Issue(s)

Part of Perform permission check in parallel with existing system, report stats #16103
Depends on [server] Add spicedb client #16197

How to test

Preview
Sign in, create organization, interact with organization in any way
There is no change in behaviour
There are logs indicating the permission checks are happening
Metrics are reported

Release Notes

NONE

Documentation

Build Options:

/werft with-github-actions
Experimental feature to run the build with GitHub Actions (and not in Werft).
leeway-no-cache
leeway-target=components:all
/werft no-test
Run Leeway with --dont-test
/werft publish-to-npm

Preview Environment Options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-gce-vm
If enabled this will create the environment on GCE infra
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

werft-gitpod-dev-com · 2023-02-05T21:12:29Z

started the job as gitpod-build-mp-spicedb-server-checks.6 because the annotations in the pull request description changed
(with .werft/ from main)

easyCZ · 2023-02-05T21:14:39Z

/hold for dependency on #16197

easyCZ · 2023-02-05T21:15:32Z

components/server/src/authorization/checks.ts

+export const ReadOrganizationMetadata = check("user", "organization_metadata_read", "organization");
+export const WriteOrganizationMetadata = check("user", "organization_metadata_write", "organization");
+
+export const ReadOrganizationMembers = check("user", "organization_members_read", "organization");
+export const WriteOrganizationMembers = check("user", "organization_members_write", "organization");


These are declarative permission checks. The idea is to be able to collate all of these in a single place, and be able to trace their usage.

easyCZ · 2023-02-05T21:16:36Z

components/server/src/workspace/gitpod-server-impl.ts

+    protected async guardOrganizationOperationWithCentralizedPerms(
+        orgId: string,
+        op: OrganizationOperation,
+    ): Promise<CheckResult> {
+        const user = this.checkUser();

-        const members = await this.teamDB.findMembersByTeam(team.id);
-        await this.guardAccess({ kind: "team", subject: team, members }, op);
-        return { team, members };
+        switch (op) {
+            case "org_metadata_read":
+                return await this.authorizer.check(ReadOrganizationMetadata(user.id, orgId));
+            case "org_metadata_write":
+                return await this.authorizer.check(WriteOrganizationMetadata(user.id, orgId));
+
+            case "org_members_read":
+                return await this.authorizer.check(ReadOrganizationMembers(user.id, orgId));
+            case "org_members_write":
+                return await this.authorizer.check(WriteOrganizationMembers(user.id, orgId));
+
+            default:
+                return NotPermitted;
+        }


This acts as an adapter between the "fine grained ops" defined as strings, and the actual checks. Once we have fully migrated, this adapter won't exist, but it makes the existing code a bit easier to evolve, and reason about.

Are we going to pass the check constants instead fo the strings? Would be great to move this into the authorization folder as well.

Eventually will go away, once we migrate but for now, they are useful as a way of adapting the behaviour.

easyCZ · 2023-02-05T21:17:59Z

components/server/src/workspace/gitpod-server-impl.ts

@@ -2037,7 +2046,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
    protected async guardTeamOperation(
        teamId: string,
        op: ResourceAccessOp,
-        fineGrainedOps: OrganizationOperation[],
+        fineGrainedOp: OrganizationOperation,


Originally I thought we'd have a list of these, but I've since changed my mind. We should only reference a single operation, which in turn may require multiple different perms (like Inviting team members will require organizaiton_metadata_read and organization_members_write)

svenefftinge

Really like where this is going!

svenefftinge · 2023-02-06T07:58:24Z

components/server/src/workspace/gitpod-server-impl.ts

@@ -2037,7 +2052,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
    protected async guardTeamOperation(


Would be great to move this logic out of this class into it's own one.

Yep, eventually. For now, I'm trying to incorporate it to limit the amount of changes. The nice part of the PermissionChecker interface is that we could effectively implement the current behaviour (with db) such that it conforms to the PermissionChecker interface, and then split on the two different implementations behind a feature flag.

svenefftinge · 2023-02-06T08:00:28Z

components/server/src/workspace/gitpod-server-impl.ts

+            const team = await this.teamDB.findTeamById(teamId);
+            if (!team) {
+                // We return Permission Denied because we don't want to leak the existence, or not of the Organization.
+                throw new ResponseError(ErrorCodes.PERMISSION_DENIED, `No access to Organization ID: ${teamId}`);


Doesn't seem to be a change from this PR, but I think it is more common to return a not-found error code in such cases (including no permissions but resource exists).

See https://gitpod.slack.com/archives/C01MLB7J24T/p1675414231569989

Yeah, it's not a change from this PR (here, it's just thunked).

thanks commented there

svenefftinge · 2023-02-06T08:18:01Z

components/server/src/authorization/perms.ts

+
+export const PermissionChecker = Symbol("PermissionChecker");
+
+export interface PermissionChecker {


What's the purpose of introducing an interface for this? Also the naming pattern is a little unusal, as we'd uzse the same name for bot interface and implmentations but just add a word about the implementation to it. E.g. PermissionCheckerSpiceDBImpl but then again I'm not sure we need an interface at all, especially if the signature contains types from authzed.

So the guard layer of APIs should only use a PermissionChecker. Once that check passes, and we create a resource, we'll want to use a PermissionGranter. They can both be implemented by the same class (Authorizer), but the type signatures should be constrained. The goal is to use composition, rather than exposing everything. This makes it easier to actually write stub implementations for tests (if we had any..).

The type does in fact rely on the spicedb type. Not ideal, but at this stage I wanted to reduce re-definition of types. If we needed to abstract, it would be relatively easy to swap the argument type for a custom one.

Thank you, makes sense 👍

svenefftinge · 2023-02-06T08:23:03Z

components/server/src/workspace/gitpod-server-impl.ts

+    protected async guardOrganizationOperationWithCentralizedPerms(
+        orgId: string,
+        op: OrganizationOperation,
+    ): Promise<CheckResult> {
+        const user = this.checkUser();

-        const members = await this.teamDB.findMembersByTeam(team.id);
-        await this.guardAccess({ kind: "team", subject: team, members }, op);
-        return { team, members };
+        switch (op) {
+            case "org_metadata_read":
+                return await this.authorizer.check(ReadOrganizationMetadata(user.id, orgId));
+            case "org_metadata_write":
+                return await this.authorizer.check(WriteOrganizationMetadata(user.id, orgId));
+
+            case "org_members_read":
+                return await this.authorizer.check(ReadOrganizationMembers(user.id, orgId));
+            case "org_members_write":
+                return await this.authorizer.check(WriteOrganizationMembers(user.id, orgId));
+
+            default:
+                return NotPermitted;
+        }


Are we going to pass the check constants instead fo the strings? Would be great to move this into the authorization folder as well.

easyCZ · 2023-02-06T08:39:16Z

/werft run

👍 started the job as gitpod-build-mp-spicedb-server-checks.8
(with .werft/ from main)

easyCZ · 2023-02-06T09:42:20Z

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-mp-spicedb-server-checks.10
(with .werft/ from main)

easyCZ · 2023-02-06T09:59:26Z

/unhold

roboquat added release-note-none do-not-merge/work-in-progress size/L size/XL and removed size/L labels Feb 3, 2023

easyCZ changed the base branch from main to mp/spicedb-server-client February 5, 2023 20:54

easyCZ force-pushed the mp/spicedb-server-checks branch from dc7717e to ff08aae Compare February 5, 2023 21:08

easyCZ changed the title ~~[server] Spicedb server checks~~ [server] Perform authorization checks for Orgs against spicedb Feb 5, 2023

roboquat added the do-not-merge/hold label Feb 5, 2023

easyCZ marked this pull request as ready for review February 5, 2023 21:14

easyCZ requested a review from a team February 5, 2023 21:14

roboquat removed the do-not-merge/work-in-progress label Feb 5, 2023

github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 5, 2023

easyCZ commented Feb 5, 2023

View reviewed changes

easyCZ force-pushed the mp/spicedb-server-checks branch from ff08aae to c0fe7d6 Compare February 6, 2023 07:24

svenefftinge approved these changes Feb 6, 2023

View reviewed changes

Base automatically changed from mp/spicedb-server-client to main February 6, 2023 09:14

[server] Perform authorization checks for Orgs against spicedb

ceb0c9b

easyCZ force-pushed the mp/spicedb-server-checks branch from c0fe7d6 to ceb0c9b Compare February 6, 2023 09:18

roboquat removed the do-not-merge/hold label Feb 6, 2023

roboquat merged commit 8233dc7 into main Feb 6, 2023

roboquat deleted the mp/spicedb-server-checks branch February 6, 2023 09:59

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Feb 6, 2023

		@@ -2037,7 +2052,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
		protected async guardTeamOperation(


		export const PermissionChecker = Symbol("PermissionChecker");

		export interface PermissionChecker {

[server] Perform authorization checks for Orgs against spicedb #16207

[server] Perform authorization checks for Orgs against spicedb #16207

Uh oh!

Conversation

easyCZ commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

How to test

Release Notes

Documentation

Build Options:

Preview Environment Options:

Uh oh!

werft-gitpod-dev-com bot commented Feb 5, 2023

Uh oh!

easyCZ commented Feb 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svenefftinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

easyCZ commented Feb 6, 2023 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

easyCZ commented Feb 6, 2023 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

easyCZ commented Feb 6, 2023

Uh oh!

Uh oh!

easyCZ commented Feb 3, 2023 •

edited

Loading

easyCZ commented Feb 6, 2023 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ commented Feb 6, 2023 •

edited by werft-gitpod-dev-com bot

Loading