Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions components/server/ee/src/workspace/gitpod-server-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1596,7 +1596,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {
// Team Subscriptions 2
async getTeamSubscription(ctx: TraceContext, teamId: string): Promise<TeamSubscription2 | undefined> {
this.checkUser("getTeamSubscription");
await this.guardTeamOperation(teamId, "get", ["not_implemented"]);
await this.guardTeamOperation(teamId, "get", "not_implemented");
return this.teamSubscription2DB.findForTeam(teamId, new Date().toISOString());
}

Expand Down Expand Up @@ -2098,7 +2098,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {

try {
if (attrId.kind == "team") {
await this.guardTeamOperation(attrId.teamId, "get", ["not_implemented"]);
await this.guardTeamOperation(attrId.teamId, "get", "not_implemented");
}
const subscriptionId = await this.stripeService.findUncancelledSubscriptionByAttributionId(attributionId);
return subscriptionId;
Expand All @@ -2119,7 +2119,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {
}
let team: Team | undefined;
if (attrId.kind === "team") {
team = (await this.guardTeamOperation(attrId.teamId, "update", ["not_implemented"])).team;
team = (await this.guardTeamOperation(attrId.teamId, "update", "not_implemented")).team;
await this.ensureStripeApiIsAllowed({ team });
} else {
if (attrId.userId !== user.id) {
Expand All @@ -2141,7 +2141,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {
}
let team: Team | undefined;
if (attrId.kind === "team") {
team = (await this.guardTeamOperation(attrId.teamId, "update", ["not_implemented"])).team;
team = (await this.guardTeamOperation(attrId.teamId, "update", "not_implemented")).team;
await this.ensureStripeApiIsAllowed({ team });
} else {
if (attrId.userId !== user.id) {
Expand Down Expand Up @@ -2211,7 +2211,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {
let team: Team | undefined;
try {
if (attrId.kind === "team") {
team = (await this.guardTeamOperation(attrId.teamId, "update", ["not_implemented"])).team;
team = (await this.guardTeamOperation(attrId.teamId, "update", "not_implemented")).team;
await this.ensureStripeApiIsAllowed({ team });
} else {
await this.ensureStripeApiIsAllowed({ user });
Expand Down Expand Up @@ -2260,7 +2260,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {
await this.ensureStripeApiIsAllowed({ user });
returnUrl = this.config.hostUrl.with(() => ({ pathname: `/user/billing`, search: `org=0` })).toString();
} else if (attrId.kind === "team") {
const team = (await this.guardTeamOperation(attrId.teamId, "update", ["not_implemented"])).team;
const team = (await this.guardTeamOperation(attrId.teamId, "update", "not_implemented")).team;
await this.ensureStripeApiIsAllowed({ team });
}
let url: string;
Expand Down Expand Up @@ -2493,7 +2493,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {
traceAPIParams(ctx, { teamId });

this.checkAndBlockUser("getBillingModeForTeam");
const { team } = await this.guardTeamOperation(teamId, "get", ["not_implemented"]);
const { team } = await this.guardTeamOperation(teamId, "get", "not_implemented");

return this.billingModes.getBillingModeForTeam(team, new Date());
}
Expand Down
41 changes: 41 additions & 0 deletions components/server/src/authorization/checks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Copyright (c) 2023 Gitpod GmbH. All rights reserved.
* Licensed under the GNU Affero General Public License (AGPL).
* See License.AGPL.txt in the project root for license information.
*/

import { v1 } from "@authzed/authzed-node";
import { ResourceType, SubjectType } from "./perms";

const FULLY_CONSISTENT = v1.Consistency.create({
requirement: {
oneofKind: "fullyConsistent",
fullyConsistent: true,
},
});

type SubjectResourceCheckFn = (subjectID: string, resourceID: string) => v1.CheckPermissionRequest;

function check(subjectT: SubjectType, op: string, resourceT: ResourceType): SubjectResourceCheckFn {
return (subjectID, resourceID) =>
v1.CheckPermissionRequest.create({
subject: v1.SubjectReference.create({
object: v1.ObjectReference.create({
objectId: subjectID,
objectType: subjectT,
}),
}),
permission: op,
resource: v1.ObjectReference.create({
objectId: resourceID,
objectType: resourceT,
}),
consistency: FULLY_CONSISTENT,
});
}

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");
Comment on lines +37 to +41
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

74 changes: 74 additions & 0 deletions components/server/src/authorization/perms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,17 @@
* See License.AGPL.txt in the project root for license information.
*/

import { v1 } from "@authzed/authzed-node";
import { ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
import { inject, injectable } from "inversify";
import { ResponseError } from "vscode-ws-jsonrpc";
import {
observespicedbClientLatency as observeSpicedbClientLatency,
spicedbClientLatency,
} from "../prometheus-metrics";
import { SpiceDBClient } from "./spicedb";

export type OrganizationOperation =
// A not yet implemented operation at this time. This exists such that we can be explicit about what
// we have not yet migrated to fine-grained-permissions.
Expand Down Expand Up @@ -31,3 +42,66 @@ export type OrganizationOperation =
| "org_authprovider_write"
// Ability to read Organization Auth Providers.
| "org_authprovider_read";

export type ResourceType = "organization";

export type SubjectType = "user";

export type CheckResult = {
permitted: boolean;
err?: Error;
response?: v1.CheckPermissionResponse;
};

export const NotPermitted = { permitted: false };

export const PermissionChecker = Symbol("PermissionChecker");

export interface PermissionChecker {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, makes sense 👍

check(req: v1.CheckPermissionRequest): Promise<CheckResult>;
}

@injectable()
export class Authorizer implements PermissionChecker {
@inject(SpiceDBClient)
private client: SpiceDBClient;

async check(req: v1.CheckPermissionRequest): Promise<CheckResult> {
if (!this.client) {
return {
permitted: false,
err: new Error("Authorization client not available."),
response: v1.CheckPermissionResponse.create({}),
};
}

const timer = spicedbClientLatency.startTimer();
try {
const response = await this.client.checkPermission(req);
const permitted = response.permissionship === v1.CheckPermissionResponse_Permissionship.HAS_PERMISSION;
const err = !permitted ? newUnathorizedError(req.resource!, req.permission, req.subject!) : undefined;

observeSpicedbClientLatency("check", req.permission, undefined, timer());

return { permitted, response, err };
} catch (err) {
log.error("[spicedb] Failed to perform authorization check.", err, { req });
observeSpicedbClientLatency("check", req.permission, err, timer());

throw err;
}
}
}

function newUnathorizedError(resource: v1.ObjectReference, relation: string, subject: v1.SubjectReference) {
return new ResponseError(
ErrorCodes.PERMISSION_DENIED,
`Subject (${objString(subject.object)}) is not permitted to perform ${relation} on resource ${objString(
resource,
)}.`,
);
}

function objString(obj?: v1.ObjectReference): string {
return `${obj?.objectType}:${obj?.objectId}`;
}
2 changes: 2 additions & 0 deletions components/server/src/container-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ import { UbpResetOnCancel } from "@gitpod/gitpod-payment-endpoint/lib/chargebee/
import { retryMiddleware } from "nice-grpc-client-middleware-retry";
import { IamSessionApp } from "./iam/iam-session-app";
import { spicedbClientFromEnv, SpiceDBClient } from "./authorization/spicedb";
import { Authorizer, PermissionChecker } from "./authorization/perms";

export const productionContainerModule = new ContainerModule((bind, unbind, isBound, rebind) => {
bind(Config).toConstantValue(ConfigFile.fromFile());
Expand Down Expand Up @@ -313,4 +314,5 @@ export const productionContainerModule = new ContainerModule((bind, unbind, isBo
bind(SpiceDBClient)
.toDynamicValue(() => spicedbClientFromEnv())
.inSingletonScope();
bind(PermissionChecker).to(Authorizer).inSingletonScope();
});
30 changes: 30 additions & 0 deletions components/server/src/prometheus-metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ export function registerServerMetrics(registry: prometheusClient.Registry) {
registry.registerMetric(stripeClientRequestsCompletedDurationSeconds);
registry.registerMetric(imageBuildsStartedTotal);
registry.registerMetric(imageBuildsCompletedTotal);
registry.registerMetric(centralizedPermissionsValidationsTotal);
registry.registerMetric(spicedbClientLatency);
}

const loginCounter = new prometheusClient.Counter({
Expand Down Expand Up @@ -195,3 +197,31 @@ export const imageBuildsCompletedTotal = new prometheusClient.Counter({
export function increaseImageBuildsCompletedTotal(outcome: "succeeded" | "failed") {
imageBuildsCompletedTotal.inc({ outcome });
}

const centralizedPermissionsValidationsTotal = new prometheusClient.Counter({
name: "gitpod_perms_centralized_validations_total",
help: "counter of centralized permission checks validations against existing system",
labelNames: ["operation", "matches_expectation"],
});

export function reportCentralizedPermsValidation(operation: string, matches: boolean) {
centralizedPermissionsValidationsTotal.inc({ operation, matches_expectation: String(matches) });
}

export const spicedbClientLatency = new prometheusClient.Histogram({
name: "gitpod_spicedb_client_requests_completed_seconds",
help: "Histogram of completed spicedb client requests",
labelNames: ["operation", "permission", "outcome"],
});

export function observespicedbClientLatency(
operation: string,
permission: string,
outcome: Error | undefined,
durationInSeconds: number,
) {
spicedbClientLatency.observe(
{ operation, permission, outcome: outcome === undefined ? "success" : "error" },
durationInSeconds,
);
}
Loading