-
Notifications
You must be signed in to change notification settings - Fork 9
Add AffectedScope-decorator #4931
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.
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
base: main
Are you sure you want to change the base?
Conversation
77b603c to
35091e6
Compare
packages/api/cms-api/src/user-permissions/decorators/affected-scope.decorator.ts
Show resolved
Hide resolved
docs/docs/2-core-concepts/1-content-scope/3-evaluate-content-scopes.md
Outdated
Show resolved
Hide resolved
packages/api/cms-api/src/user-permissions/decorators/affected-scope.decorator.ts
Show resolved
Hide resolved
| "@comet/cms-api": minor | ||
| --- | ||
|
|
||
| Skip undefined values in content scope check |
There was a problem hiding this comment.
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?
- It's more correct than Treat
nullandundefinedscope dimensions the same inAccessControlService#isAllowed#2643 - It makes handling optional values easier
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
Use this decorator in following cases:
@AffectedEntityExample:
Further Information