Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
7 changes: 7 additions & 0 deletions .changeset/eleven-zoos-rule.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@comet/cms-api": minor
---

Add `@AffectedScope` decorator

See https://docs.comet-dxp.com/docs/core-concepts/content-scope/evaluate-content-scopes#operations-that-require-a-content-scope-independently-of-a-specific-entity
5 changes: 5 additions & 0 deletions .changeset/upset-wolves-take.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@comet/cms-api": minor
---

Skip undefined values in content scope check
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 352cca4

Why this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't changing this a breaking change? Now null isn't treated as "not existing" any longer, but rather "no specific value set". I can imagine that this would break access control in some of our applications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main difference is that it's a bit more loose.

Before:
{foo: undefined} forced the user having {foo: null} or {foo: undefined}
{foo: null} forced the user having {foo: null} or {foo: undefined}

Now:
{foo: undefined} doesn't require to have a content scope
{foo: null} forces the user having {foo: null}

It's breaking indeed if the user's content-scope consists of undefined values:
Breaking:
{foo: null} isn't allowed anymore if the user has {foo: undefined}
Can we neglect this? If not, re-adding these lines should be make it compatible again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also could revert this breaking change. It's not really necessary, but it makes handling optional values easier in argsToScope easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we neglect this? If not, re-adding these lines should be make it compatible again.

@Ben-Ho @kaufmo what do you think about this? To my best knowledge you're the only ones that have projects with optional scope dimensions.

Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ async product(@Args("id", { type: () => ID }) id: string): Promise<Product> {
```

```ts
@Query([Product])
@Query(() => [Product])
@AffectedEntity(Dealer, { idArg: "dealerId" })
async products(@Args("dealerId", { type: () => ID }) dealerId: string): Promise<Product[]> {
// Note: you can trust "dealerId" being in a valid scope, but you need to make sure that your business code restricts this query to the given dealer
Expand Down Expand Up @@ -75,3 +75,32 @@ It's also possible to pass a function which returns the content scope to the `@S
@ScopedEntity(PageTreeNodeDocumentEntityScopeService)
export class PredefinedPage extends BaseEntity implements DocumentInterface {
```

### Operations that require a content scope independently of a specific entity

**@AffectedScope**

Use this decorator in following cases:

- You don't have an entity to check with `@AffectedEntity`
- You want to check for a combination of content scope values.

Example:

```ts
@Query(() => [Product])
@AffectedScope((args) => ({ country: args.country, department: args.department }))
async products(@Args("country", { type: () => string }) id: string, @Args("department", { type: () => string }): Promise<Product[]> {
// Note: you can trust "country" and "department" being in a valid scope in the sense that the user must have a content scope which is a combination of these dimensions:
// [{ country: "AT", department: "main" }]; // ok
// [{ country: "AT" }, { department: "main" }]; // not sufficient
}
```

:::info
Type annotation can either be achieved via generics (`@AffectedScope<ArgsType>((args) => args)`) or by type assignment (`@AffectedScope((args: ArgsType) => args)`)
:::

:::info
If a scope value is `undefined` the dimension will be removed before checking.
:::
1 change: 1 addition & 0 deletions packages/api/cms-api/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ export { SentryModule } from "./sentry/sentry.module";
export { AzureAiTranslatorModule } from "./translation/azure-ai-translator.module";
export { AbstractAccessControlService } from "./user-permissions/access-control.service";
export { AffectedEntity, AffectedEntityMeta, AffectedEntityOptions } from "./user-permissions/decorators/affected-entity.decorator";
export { AffectedScope } from "./user-permissions/decorators/affected-scope.decorator";
export { DisablePermissionCheck, RequiredPermission } from "./user-permissions/decorators/required-permission.decorator";
export { SCOPED_ENTITY_METADATA_KEY, ScopedEntity, ScopedEntityMeta } from "./user-permissions/decorators/scoped-entity.decorator";
export { CurrentUser } from "./user-permissions/dto/current-user";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,12 @@ import { AccessControlServiceInterface, Permission } from "./user-permissions.ty
export abstract class AbstractAccessControlService implements AccessControlServiceInterface {
private checkContentScope(userContentScopes: ContentScope[], targetContentScope: ContentScope): boolean {
return userContentScopes.some((userContentScope) =>
Object.entries(targetContentScope).every(([dimension, targetContentScopeValue]) => {
const userContentScopeValue = (userContentScope as Record<string, unknown>)[dimension];

// Treat null and undefined the same
if (userContentScopeValue == null && targetContentScopeValue == null) {
return true;
}

return userContentScopeValue === targetContentScopeValue;
}),
Object.entries(targetContentScope)
.filter(([_dimension, targetContentScopeValue]) => targetContentScopeValue !== undefined)
.every(([dimension, targetContentScopeValue]) => {
const userContentScopeValue = (userContentScope as Record<string, unknown>)[dimension];
return userContentScopeValue === targetContentScopeValue;
}),
);
}
isAllowed(user: CurrentUser, permission: Permission, contentScope?: ContentScope): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@
import { AbstractAccessControlService } from "../access-control.service";
import { ContentScopeService } from "../content-scope.service";
import { AFFECTED_ENTITY_METADATA_KEY, AffectedEntityMeta } from "../decorators/affected-entity.decorator";
import { AFFECTED_SCOPE_METADATA_KEY, AffectedScopeMeta } from "../decorators/affected-scope.decorator";
import { REQUIRED_PERMISSION_METADATA_KEY, RequiredPermissionMetadata } from "../decorators/required-permission.decorator";
import { SCOPED_ENTITY_METADATA_KEY, ScopedEntityMeta } from "../decorators/scoped-entity.decorator";
import { CurrentUser } from "../dto/current-user";
import { Permission } from "../user-permissions.types";

Check failure on line 14 in packages/api/cms-api/src/user-permissions/auth/user-permissions.guard.spec.ts

View workflow job for this annotation

GitHub Actions / Tests

UserPermissionsGuard ► UserPermissionsGuard denies by wrong multidimensional AffectedScope ► UserPermissionsGuard denies by wrong multidimensional AffectedScope

Failed test found in: api/api-generator/node_modules/@comet/cms-api/junit.xml api/cms-api/junit.xml Error: Error: expect(received).toBe(expected) // Object.is equality
Raw output
Error: expect(received).toBe(expected) // Object.is equality

Expected: false
Received: true
    at /home/runner/work/comet/comet/packages/api/cms-api/src/user-permissions/auth/user-permissions.guard.spec.ts:649:11
    at Generator.next (<anonymous>)
    at fulfilled (/home/runner/work/comet/comet/packages/api/cms-api/src/user-permissions/auth/user-permissions.guard.spec.ts:14:58)
    at processTicksAndRejections (node:internal/process/task_queues:103:5)
import { UserPermissionsGuard } from "./user-permissions.guard";

const permissions = {
Expand Down Expand Up @@ -41,12 +42,14 @@
affectedEntities?: AffectedEntityMeta[];
scopedEntity?: ScopedEntityMeta<TestEntity>;
disableCometGuards?: boolean;
affectedScope?: AffectedScopeMeta;
}) => {
reflector.getAllAndOverride = jest.fn().mockImplementation((decorator: string) => {
if (decorator === REQUIRED_PERMISSION_METADATA_KEY) return annotations.requiredPermission;
if (decorator === AFFECTED_ENTITY_METADATA_KEY) return annotations.affectedEntities;
if (decorator === SCOPED_ENTITY_METADATA_KEY) return annotations.scopedEntity;
if (decorator === DISABLE_COMET_GUARDS_METADATA_KEY) return annotations.disableCometGuards;
if (decorator === AFFECTED_SCOPE_METADATA_KEY) return annotations.affectedScope;
return false;
});
};
Expand Down Expand Up @@ -527,4 +530,148 @@
),
).rejects.toThrowError("Could not get content scope");
});

it("allows user by AffectedScope", async () => {
mockAnnotations({
requiredPermission: {
requiredPermission: [permissions.p1],
options: undefined,
},
affectedScope: { argsToScope: (args) => ({ a: args.a }) },
});
expect(
await guard.canActivate(
mockContext({
userPermissions: [
{
permission: permissions.p1,
contentScopes: [{ a: 1 }],
},
],
args: { a: 1 },
}),
),
).toBe(true);
expect(
await guard.canActivate(
mockContext({
userPermissions: [
{
permission: permissions.p1,
contentScopes: [{ a: 1 }],
},
],
args: { a: 1, b: 2 },
}),
),
).toBe(true);
});

it("allows user by multidimensional AffectedScope", async () => {
mockAnnotations({
requiredPermission: {
requiredPermission: [permissions.p1],
options: undefined,
},
affectedScope: { argsToScope: (args) => ({ a: args.a, b: args.submittedB }) },
});
expect(
await guard.canActivate(
mockContext({
userPermissions: [
{
permission: permissions.p1,
contentScopes: [{ a: 1, b: "2" }],
},
],
args: { a: 1, submittedB: "2" },
}),
),
).toBe(true);
});

it("denies by wrong AffectedScope", async () => {
mockAnnotations({
requiredPermission: {
requiredPermission: [permissions.p1],
options: undefined,
},
affectedScope: { argsToScope: (args) => ({ a: args.a }) },
});
expect(
await guard.canActivate(
mockContext({
userPermissions: [
{
permission: permissions.p1,
contentScopes: [{ a: 1 }],
},
],
args: { a: 2 },
}),
),
).toBe(false);
expect(
await guard.canActivate(
mockContext({
userPermissions: [
{
permission: permissions.p1,
contentScopes: [{ a: 1 }],
},
],
args: { a: "1" },
}),
),
).toBe(false);
});

it("denies by wrong multidimensional AffectedScope", async () => {
mockAnnotations({
requiredPermission: {
requiredPermission: [permissions.p1],
options: undefined,
},
affectedScope: { argsToScope: (args) => ({ a: args.a, b: args.b }) },
});
expect(
await guard.canActivate(
mockContext({
userPermissions: [
{
permission: permissions.p1,
contentScopes: [{ a: 1, b: "2" }],
},
],
args: { a: 1 },
}),
),
).toBe(false);
expect(
await guard.canActivate(
mockContext({
userPermissions: [
{
permission: permissions.p1,
contentScopes: [{ a: 1 }, { b: "2" }], // User must have combination of a and b
},
],
args: { a: 1, b: "2" },
}),
),
).toBe(false);
expect(
await guard.canActivate(
mockContext({
userPermissions: [
{
permission: permissions.p1,
contentScopes: [{ a: 1, b: "2" }],
},
],
args: { a: 1, b: 2 },
}),
),
).toBe(false);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export class UserPermissionsGuard implements CanActivate {
} else {
if (requiredContentScopes.length === 0)
throw new Error(
`Could not get content scope. Either pass a scope-argument or add an @AffectedEntity()-decorator or enable skipScopeCheck in the @RequiredPermission()-decorator of ${location}`,
`Could not get content scope. Either pass a scope-argument or add an @AffectedEntity()/@AffectedScope()-decorator or enable skipScopeCheck in the @RequiredPermission()-decorator of ${location}`,
);

// requiredContentScopes is an two level array of scopes
Expand Down
11 changes: 11 additions & 0 deletions packages/api/cms-api/src/user-permissions/content-scope.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { PageTreeService } from "../page-tree/page-tree.service";
import { SCOPED_ENTITY_METADATA_KEY, ScopedEntityMeta } from "../user-permissions/decorators/scoped-entity.decorator";
import { ContentScope } from "../user-permissions/interfaces/content-scope.interface";
import { AFFECTED_ENTITY_METADATA_KEY, AffectedEntityMeta } from "./decorators/affected-entity.decorator";
import { AFFECTED_SCOPE_METADATA_KEY, AffectedScopeMeta } from "./decorators/affected-scope.decorator";

// TODO Remove service and move into UserPermissionsGuard once ChangesCheckerInterceptor is removed
@Injectable()
Expand All @@ -33,13 +34,23 @@ export class ContentScopeService {
const args = await this.getArgs(context);
const location = `${context.getClass().name}::${context.getHandler().name}()`;

// AffectedEntities
const affectedEntities = this.reflector.getAllAndOverride<AffectedEntityMeta[]>(AFFECTED_ENTITY_METADATA_KEY, [context.getHandler()]) || [];
for (const affectedEntity of affectedEntities) {
contentScopes.push(...(await this.getContentScopesFromEntity(affectedEntity, args, location)));
}

// AffectedScope
const affectedScope = this.reflector.getAllAndOverride<AffectedScopeMeta>(AFFECTED_SCOPE_METADATA_KEY, [context.getHandler()]) || undefined;
if (affectedScope) {
contentScopes.push([affectedScope.argsToScope(args) as ContentScope]);
}

// Scope arg
if (args.scope) {
contentScopes.push([args.scope as ContentScope]);
}

return contentScopes;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { SetMetadata } from "@nestjs/common";

import { type ContentScope } from "../../user-permissions/interfaces/content-scope.interface";

export type AffectedScopeMeta = {
argsToScope: (args: Record<string, unknown>) => ContentScope;
};

export const AFFECTED_SCOPE_METADATA_KEY = "affectedScope";

export const AffectedScope = <T>(argsToScope: (args: T) => ContentScope): MethodDecorator => {
return SetMetadata<string, AffectedScopeMeta>(AFFECTED_SCOPE_METADATA_KEY, { argsToScope: argsToScope as AffectedScopeMeta["argsToScope"] });
};
Loading