refactor: rename simple-authorization to legacy-authorization, change function footprint
#5772
Conversation
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
…uery Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
…fer-multiplePermissions Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Ross Hadden <[email protected]>
…fer-multiplePermissions
Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
| if (!context.isInternalCall) { | ||
| await context.validatePermissions(`reaction:accounts:${account._id}`, "add:emails", { | ||
| shopId: account.shopId, | ||
| owner: account._id, |
There was a problem hiding this comment.
@rosshadden @kieckhafer All validatePermissions calls should be using account.userId rather than account._id everywhere. We currently set them to the same string, but it's not a guarantee if people import them or something.
There was a problem hiding this comment.
Referring to owner value. In this case reaction:accounts:${account._id} probably is correct because that's the account you are accessing.
There was a problem hiding this comment.
What about context.accountId? We have a lot of properties that are (currently) the same thing.
All of these account references have database lookups to the Accounts collection. It looks like we could refactor a lot of places to use the account data on the context and save many DB calls.
…y the same This future-proofs the code a bit. Signed-off-by: Ross Hadden <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
…ctioncommerce/reaction into feat-kieckhafer-multiplePermissions
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
kieckhafer
changed the title
simple-authorization to legacy-authorization, and add keto-authorization capabilitessimple-authorization to legacy-authorization, change function footprint
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
…fer-multiplePermissions
Signed-off-by: Erik Kieckhafer <[email protected]>
|
@rosshadden I believe this is ready to go on my end. Have you updated the values @aldeed mentioned? |
Signed-off-by: Erik Kieckhafer <[email protected]>
|
Failing tests ticket #5861 |
There was a problem hiding this comment.
Approved. This ticket is too big and too crucial to future tests to leave sitting for any longer. I'm going to approve and merge this now and work on fixing the tests immediately. Any other comments or changes to this PR by other reviewers should be made into tickets.
Impact: minor
Type: feature|refactor
As an interim step in our transition from our current authorization system to our new Keto based system, we have changed the function signature for our permission checks.
The new signiture looks like this, with
legacyRolesbeing the array of roles previously sent into the legacy auth check:validatePermissions("resource", "action", { shopId, legacyRoles: [], ...}Testing
You should see no changes in how the app functions with this PR. The legacy auth service
hasPermissioncheck has been updated to use the new function signature.Breaking Changes
Nothing for core code / Reaction 3.0, but this will break and third party / plugin auth checks.