Refactor authorization #3529
Refactor authorization #3529
Conversation
jannyHou
force-pushed
the
cleanup/authorization
branch
from
c8d8042 to
973f6cd
Compare
|
Tests pass on my local. The CI error is same as #3520. Fixing it... |
| @@ -26,7 +27,7 @@ describe('Authorization', () => { | |||
| let events: string[]; | |||
|
|
|||
| before(givenApplicationAndAuthorizer); | |||
| beforeEach(givenRequestContext); | |||
| beforeEach(() => givenRequestContext()); | |||
There was a problem hiding this comment.
Why is this change needed?
There was a problem hiding this comment.
@raymondfeng I am seeing this weird error...
There was a problem hiding this comment.
Oh, I think that's because givenRequestContext takes an optional user argument.
| @@ -161,6 +165,7 @@ describe('Authorization', () => { | |||
| __dirname, | |||
| '../../../fixtures/casbin/rbac_policy.csv', | |||
| ); | |||
| return await casbin.newEnforcer(conf, policy); | |||
| const result = await casbin.newEnforcer(conf, policy); | |||
| return result; | |||
There was a problem hiding this comment.
return casbin.newEnforcer(conf, policy); should be good enough.
| }); | ||
|
|
||
| // retrieve it from authentication module | ||
| const user = await invocationCtx.get<{name: string}>( |
There was a problem hiding this comment.
Use UserProfile instead of {name: string}?
| debug('Security context for %s', description, authorizationCtx); | ||
| let authorizers = await loadAuthorizers( | ||
| invocationCtx, | ||
| metadata.voters || [], | ||
| ); | ||
|
|
||
| // pass `currentUser` to authorizers | ||
| Object.assign(metadata, {currentUser: user}); |
There was a problem hiding this comment.
currentUser should be set to authorizationContext if necessary instead of metadata.
There was a problem hiding this comment.
#3529 is also updated accordingly.
| } | ||
| } | ||
| return await next(); | ||
| const result = await next(); |
There was a problem hiding this comment.
return next(); should be good.
| @@ -27,6 +27,7 @@ | |||
| "devDependencies": { | |||
| "@loopback/build": "^2.0.3", | |||
| "@loopback/testlab": "^1.6.3", | |||
| "@loopback/authentication": "^2.1.8", | |||
There was a problem hiding this comment.
Please add some comments about this temporary workaround.
There was a problem hiding this comment.
@raymondfeng I added an entry in the README.md file to explain the user injection.
There was a problem hiding this comment.
BTW, let's change the initial version to be 0.0.1 so that the first release will be 0.1.0.
jannyHou
force-pushed
the
authorization
branch
from
997eb86 to
0ee7df4
Compare
jannyHou
force-pushed
the
cleanup/authorization
branch
3 times, most recently
from
5269dd3 to
968ef91
Compare
|
loopbackio/loopback4-example-shopping#231 is also updated. |
| email: user.email, | ||
| type: 'USER' | ||
| } | ||
| } |
There was a problem hiding this comment.
Please move this inline function to the module scope.
jannyHou
force-pushed
the
cleanup/authorization
branch
from
0b09eee to
ba1e319
Compare
Some refactor for #1205
It contains subsequent changes to make the e2e tests in PR loopbackio/loopback4-example-shopping#231 work.
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈