Add docs for user permissions #2128

fraxachun · 2024-06-02T06:17:33Z

https://vivid-planet.atlassian.net/browse/COM-705

https://deploy-preview-2128--comet-dxp-docs.netlify.app/

COM-667

nsams · 2024-06-03T07:24:12Z

User-permissions have basically the same topic as the content-scope docs. If you want to move parts into user-permissions you should at least add a link.

Or - maybe the whole content-scope topic should be integrated into user-permissions

fraxachun · 2024-06-04T05:58:54Z

User-permissions have basically the same topic as the content-scope docs. If you want to move parts into user-permissions you should at least add a link.

Actually, there is one: https://github.com/vivid-planet/comet/pull/2128/files#diff-6de07fe72c2ca99e1f939c8c7fb83a75b8cc2d3461488b02eddfaacc471de65cR22

Or - maybe the whole content-scope topic should be integrated into user-permissions

I thought about this but decided to leave the general explanation as is and move the permission specific parts.

nsams · 2024-06-04T06:11:39Z

not the other way where you removed @ScopedEntity and @AffectedEntity (https://github.com/vivid-planet/comet/pull/2128/files#diff-05e86dd9dbc3fa6137f21d6a53defee69158651691820542b313fd9f925a2a3fL59)

fraxachun · 2024-06-04T10:55:16Z

not the other way where you removed @ScopedEntity and @AffectedEntity (https://github.com/vivid-planet/comet/pull/2128/files#diff-05e86dd9dbc3fa6137f21d6a53defee69158651691820542b313fd9f925a2a3fL59)

91c6d1e

docs/docs/content-scope/content-website.md

docs/docs/content-scope/data-driven-application.md

docs/docs/logging/index.md

docs/docs/user-permissions/index.md

docs/docs/user-permissions/access-control.md

johnnyomair · 2024-06-11T13:45:17Z

docs/docs/user-permissions/access-control.md

+Use this option only when you are sure that checking the scope is not necessary (e.g. the current entity does not have a scope). Do not add it just because it seems cumbersome at the moment to add the correct `AffectedEntity`/`ScopedEntity` decorators.
+:::
+
+:::note


This note doesn't seem right here. Was this intentional?

Yes, because in many cases skipScopeCheck is disabled only for this reason. But we could think about a more prominent location for this advice since it covers a fairly extensive design guideline.

Okay, maybe add a sentence that skipScopeCheck shouldn't be used in combination with @GetCurrentUser()?

But we could think about a more prominent location for this advice since it covers a fairly extensive design guideline.

This could be something for a follow-up PR regarding general API design.

docs/docs/user-permissions/admin.md

Co-authored-by: Johannes Obermair <48853629+johnnyomair@users.noreply.github.com>

johnnyomair · 2024-06-21T06:47:19Z

@fraxachun this PR still has some open concerns (to be fair, they are hidden by GitHub's UI)

docs/docs/user-permissions/access-control.md

docs/docs/user-permissions/index.md

docs/docs/user-permissions/access-control.md

thomasdax98 · 2024-06-25T08:51:23Z

docs/docs/user-permissions/access-control.md

+## Scope check
+
+:::caution
+COMET DXP validates the data relevant for the operation, but cannot check if the validated data is finally used. You are responsible for applying the validated data in your operations.


This sounds confusing to me. I guess what it should tell me is "Be careful which data you use and how you use it in your operation to not accidentally bypass the scope check".

What do you think about:

Suggested change

COMET DXP validates the data relevant for the operation, but cannot check if the validated data is finally used. You are responsible for applying the validated data in your operations.

COMET DXP validates the scope of the data relevant for the operation, but it cannot check how you use the data / what you do with the data within the operation. It's your job to use the data in a responsible way and without bypassing the scope check in your code. This means, for example:

- configuring the scope check for all data used in your operation (not just for one entity)

- not loading additional data from the DB without considering the scope

I don't understand these examples.

docs/docs/user-permissions/access-control.md

thomasdax98 · 2024-06-25T08:56:10Z

docs/docs/user-permissions/access-control.md

+    // 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
+}
+```
+


I'd also mention that you can have multiple AffectedEntity decorators:

Suggested change

It's possible to add multiple `@AffectedEntity` decorators to one operation if multiple entities are affected:

\```ts

@Query(Product)

@AffectedEntity(Product)

@AffectedEntity(Dealer, { idArg: "dealerId" })

async product(@Args("id", { type: () => ID }) id: string, @Args("dealerId", { type: () => ID }) dealerId: string): Promise<Product> {

//...

}

\```

thomasdax98 · 2024-06-25T09:18:33Z

docs/docs/user-permissions/access-control.md

+:::info
+You might have to load multiple relations for nested data.
+:::
+


Suggested change

For documents that get their scope from a `PageTreeNode`, you can pass the helper service `PageTreeNodeDocumentEntityScopeService` provided by the library:

\```ts

@ScopedEntity(PageTreeNodeDocumentEntityScopeService)

export class PredefinedPage extends BaseEntity<PredefinedPage, "id"> implements DocumentInterface {

\```

thomasdax98 · 2024-06-25T09:21:06Z

docs/docs/user-permissions/access-control.md

+ @AffectedEntity(User)
+- async myProducts(@GetCurrentUser() currentUser: CurrentUser): Promise<Product[]> {
+ async productsForUser(@Args("userId", { type: () => ID }) userId: string): Promise<Product[]> {


This won't work, I suppose. We either need

Suggested change

+ @AffectedEntity(User)

- async myProducts(@GetCurrentUser() currentUser: CurrentUser): Promise<Product[]> {

+ async productsForUser(@Args("userId", { type: () => ID }) userId: string): Promise<Product[]> {

+ @AffectedEntity(User)

- async myProducts(@GetCurrentUser() currentUser: CurrentUser): Promise<Product[]> {

+ async productsForUser(@Args("id", { type: () => ID }) id: string): Promise<Product[]> {

or

Suggested change

+ @AffectedEntity(User)

- async myProducts(@GetCurrentUser() currentUser: CurrentUser): Promise<Product[]> {

+ async productsForUser(@Args("userId", { type: () => ID }) userId: string): Promise<Product[]> {

+ @AffectedEntity(User, { idArg: "userId" })

- async myProducts(@GetCurrentUser() currentUser: CurrentUser): Promise<Product[]> {

+ async productsForUser(@Args("userId", { type: () => ID }) userId: string): Promise<Product[]> {

docs/docs/user-permissions/access-control.md

johnnyomair · 2024-09-19T06:32:15Z

@fraxachun what's the status here? IMO it would be good to have docs for user permissions asap.

fraxachun · 2024-10-18T11:52:58Z

@johnnyomair @thomasdax98 I updated this PR to merge in v7 main and tried to resolve all concerns (except #2128 (comment)).

Add docs for user permissions

a197ecd

fraxachun requested review from nsams, thomasdax98 and johnnyomair June 2, 2024 06:17

auto-assign bot assigned fraxachun Jun 2, 2024

Link from content scope chapters

91c6d1e

Merge remote-tracking branch 'origin/main' into user-permission-docs

7d883a3

nsams removed their request for review June 5, 2024 05:53

johnnyomair reviewed Jun 11, 2024

View reviewed changes

fraxachun and others added 9 commits June 13, 2024 12:08

Update docs/docs/content-scope/content-website.md

7dc2ba7

Co-authored-by: Johannes Obermair <48853629+johnnyomair@users.noreply.github.com>

Update docs/docs/content-scope/data-driven-application.md

7108bf8

Co-authored-by: Johannes Obermair <48853629+johnnyomair@users.noreply.github.com>

Remove wrongly copied text

c2784dc

Update docs/docs/user-permissions/index.md

ebab3b4

Co-authored-by: Johannes Obermair <48853629+johnnyomair@users.noreply.github.com>

Update docs/docs/user-permissions/index.md

9154cfc

Co-authored-by: Johannes Obermair <48853629+johnnyomair@users.noreply.github.com>

Fix wrong guards

28df049

Update docs/docs/user-permissions/admin.md

4b70c01

Co-authored-by: Johannes Obermair <48853629+johnnyomair@users.noreply.github.com>

Add demo code

4361ac6

Merge remote-tracking branch 'origin/main' into user-permission-docs

d737402

johnnyomair added 2 commits June 24, 2024 08:04

Apply suggestions from code review

36ae826

Apply suggestions from code review

3139432

thomasdax98 reviewed Jun 25, 2024

View reviewed changes

Multiple small improvements

7764240

fraxachun mentioned this pull request Oct 11, 2024

Make UserPermissions Admin panel more modular and export components #2626

Open

1 task

Merge remote-tracking branch 'origin/main' into user-permission-docs

b80c68e

fraxachun added 8 commits October 18, 2024 11:26

Fix merge errors

10a517d

Add sentence

b66e736

Update to current api

0e85f2d

Use admin panel

68766b7

Move section

07cbbe9

Add part

12ee25e

Add text

73dbd31

fix

773e977

fraxachun requested review from thomasdax98 and johnnyomair October 18, 2024 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docs for user permissions #2128

Add docs for user permissions #2128

fraxachun commented Jun 2, 2024 •

edited by thomasdax98

Loading

nsams commented Jun 3, 2024

fraxachun commented Jun 4, 2024

nsams commented Jun 4, 2024

fraxachun commented Jun 4, 2024

johnnyomair Jun 11, 2024

fraxachun Jun 13, 2024

johnnyomair Jun 21, 2024

fraxachun Oct 18, 2024

johnnyomair commented Jun 21, 2024

thomasdax98 Jun 25, 2024

fraxachun Oct 18, 2024

thomasdax98 Jun 25, 2024

fraxachun Oct 18, 2024

thomasdax98 Jun 25, 2024

fraxachun Oct 18, 2024

thomasdax98 Jun 25, 2024

fraxachun Oct 18, 2024

johnnyomair commented Sep 19, 2024

fraxachun commented Oct 18, 2024

-COMET DXP validates the data relevant for the operation, but cannot check if the validated data is finally used. You are responsible for applying the validated data in your operations.
+COMET DXP validates the scope of the data relevant for the operation, but it cannot check how you use the data / what you do with the data within the operation. It's your job to use the data in a responsible way and without bypassing the scope check in your code. This means, for example:
+- configuring the scope check for all data used in your operation (not just for one entity)
+- not loading additional data from the DB without considering the scope

+It's possible to add multiple `@AffectedEntity` decorators to one operation if multiple entities are affected:
+\```ts
+@Query(Product)
+@AffectedEntity(Product)
+@AffectedEntity(Dealer, { idArg: "dealerId" })
+async product(@Args("id", { type: () => ID }) id: string, @Args("dealerId", { type: () => ID }) dealerId: string): Promise<Product> {
+    //...
+}
+\```

+For documents that get their scope from a `PageTreeNode`, you can pass the helper service `PageTreeNodeDocumentEntityScopeService` provided by the library:
+\```ts
+@ScopedEntity(PageTreeNodeDocumentEntityScopeService)
+export class PredefinedPage extends BaseEntity<PredefinedPage, "id"> implements DocumentInterface {
+\```

Add docs for user permissions #2128

Are you sure you want to change the base?

Add docs for user permissions #2128

Conversation

fraxachun commented Jun 2, 2024 • edited by thomasdax98 Loading

nsams commented Jun 3, 2024

fraxachun commented Jun 4, 2024

nsams commented Jun 4, 2024

fraxachun commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyomair commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyomair commented Sep 19, 2024

fraxachun commented Oct 18, 2024

fraxachun commented Jun 2, 2024 •

edited by thomasdax98

Loading