-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: add access control migration app #4571
Conversation
@jannyHou , perhaps suggest necessary reading before reviewing this PR. e.g.
|
export async function getCasbinEnforcerByName(name: string): Promise<casbin.Enforcer | undefined> { | ||
if (name == 'admin') return createAdminEnforcer(); | ||
if (name == 'owner') return createOwnerEnforcer(); | ||
if (name == 'team') return createTeamEnforcer(); |
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.
As the implementation for createAdminEnforcer()
, createOwnerEnforcer()
and createTeamEnforcer()
is highly the same, maybe you want to pass the configuration file as an parameter to an createEnforcer()
function?
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.
refactored in commit 4ed9b73
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.
I am not that familiar with the area. I think I might need more information/explanations about what casbin does to AUTH in this example. Looking forward to having a meeting for explaining the design and concepts 💪
for (const role of allowedRoles) { | ||
const enforcer = await this.enforcerFactory(role); | ||
|
||
const allowed_by_role = await enforcer.enforce( |
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.
IIUC, is this where the authorizer calls casbin with enforcers to make decision?
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.
@agnes512 yep
About instance level authorization context: Look at the current It describes the fields for class level information(e.g. resource: 'project'), but lacks instance level information(e.g. resource: 'project', instanceId: '1') This PR needs to generate instance level resource name like
Conclusion: I think we should implement something to allow authorization context contribute instance level info. UPDATE The voters could contain some pre-process and post-process logic, so the current approach for providing instance level info is ok. But need to double check which one is invoked first, voter or authorizer |
2nd Phase implementation: Casbin persistency and synchronize:
|
@emonddr 👍 thank you I am going to summarize your suggestions in #4571 (comment) and upload the meeting slides. |
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.
Good start. some questions. :)
|
||
// Generate the user name according to the naming convention | ||
// in casbin policy | ||
// A use's name would be `u${id}` |
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.
A use's name
A user's name
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.
@jannyHou . Still there ^
TokenServiceConstants.TOKEN_SECRET_VALUE, | ||
); | ||
this.bind(TokenServiceBindings.TOKEN_EXPIRES_IN).to( | ||
TokenServiceConstants.TOKEN_EXPIRES_IN_VALUE, |
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.
set this to 21600 instead of 600. So when people try it out
or debug it...the token doesn't expire in 10 minutes.
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.
Still suggesting this...
@@ -0,0 +1,25 @@ | |||
// Copyright IBM Corp. 2018. All Rights Reserved. |
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.
why do we need a migrate.ts file for a sample?
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.
copied from todo example :p, removed.
// Instance level authorizer | ||
// This is a WORKAROUND to modify the authorization context | ||
// It is not used for making a decision, so just returns ABSTAIN | ||
export async function assignProjectInstanceId( |
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'll need a function to compute the resourceId value in authorization context, but in the end, it shouldn't be a voter
function. Those should return a decision. (but I understand why you put it here in the meantime...it sets up the data that is used by casbinproviderauthorizer.)
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.
@emonddr Yeah I also had that concern, but after the meeting I think we agree that it's ok to contain per-process or post-process logic in the voters, see my comment in #4571 (comment), especially the update at the bottom.
// the current AuthrozationCtx is designed for class level object, | ||
// like projects | ||
const projectId = authorizationCtx.invocationContext.args[0]; | ||
const resourceName = getResourceName(metadata.resource ?? authorizationCtx.resource, projectId); |
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.
at the moment authorizationCtx.resource
is not really used . wondering if we should remove it.
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.
in case metadata doesn't have resource
, I think we should still keep it.
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.
Verified again, it has to be there, because according to the interface AuthorizationContext and AuthorizationMetadata, metadata has resource
as optional while context has it as required, which means the authorization context guarantees the resource
is provided.
If I remove it, getResourceName
has to make the 1st parameter optional:
function getResourceName(resource: string, id?: number): string {
// instance level name
if (id) return `${resource}${id}`;
// class level name
return `${resource}*`;
}
having multiple optional parameters is annoying, and also doesn't reflect the truth, the resource name should be guaranteed to exist when call the function.
metadata: AuthorizationMetadata, | ||
): Promise<AuthorizationDecision> { | ||
const subject = this.getUserName(authorizationCtx.principals[0].id); | ||
const object = authorizationCtx.resourceId ?? metadata.resource ?? authorizationCtx.resource; |
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.
authorizationCtx.resource
not really used at all.
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 #4571 (comment) ^
const request: AuthorizationRequest = { | ||
subject, | ||
object, | ||
action: (metadata.scopes && metadata.scopes[0]) || 'execute', |
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.
is execute
defined in the casbin policy or csv files?
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.
execute
is the default scope. I define scopes
for my endpoints, but we cannot make assumption that all endpoints will be decorated with that metadata.
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.
Perhaps create a constant named DEFAULT_SCOPE-'execute', and use this constant instead.
// TBD: REFACTOR to a casbin util file | ||
// Generate the resource name according to the naming convention | ||
// in casbin policy | ||
function getResourceName(resource: string, id?: number): string { |
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.
if we plan on having u1
in the casbin files, then we should have a table that maps u1
to a unique email
or userid
instead of a database index
. [securityId] and id in principal are usually the same and are database uuid or indexes. I think for casbin, this will cause problems .
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.
not sure I understand:
[securityId] and id in principal are usually the same and are database uuid or indexes.
That's why I create the user field in casbin with conversion u${id}
, to ensure it's unique.
So what is the problem here?
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.
What I am saying is that right now, the database id for a user is : 1,2,3, etc. But if I store stuff in Mongodb it will be a long uuid or guid, so you would need u0209b175-b39c-4dd4-a158-4dc81547e77e
in your casbin file. And if ever users are deleted and added to a database again, these guid values will be different, and so you will need to change your casbin files accordingly.
So having users in memory database:
"User": {
"1": "{\"id\":1,\"username\":\"John\",\"email\":\"[email protected]\"}",
"2": "{\"id\":2,\"username\":\"Jane\",\"email\":\"[email protected]\"}",
"3": "{\"id\":3,\"username\":\"Bob\",\"email\":\"[email protected]\"}"
},
and casbin file using u1
, u2
etc works for this example, but perhaps a more real world example would place
unique user names in the casbin file like: jannyhou88
and dremond71
, and in real database these users would have ID values like a guid or long int.
So what I am saying is that we should have a function that takes the securityId and queries the database for the unique userid, and this userid is used by cabin policy decisions.
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.
@emonddr Hmm, this is a design choice that I don't quite agree:
the database id for a user is : 1,2,3, etc. But if I store stuff in Mongodb it will be a long uuid or guid, so you would need u0209b175-b39c-4dd4-a158-4dc81547e77e in your casbin file.
First, my app as a prototype demo for RBAC, wants to avoid advise on those noisy implementation details not related to the core authorization process.
The design principle I aim to emphasize is what to do: what authorization system does, what 3rd-party lib does, NOT "how to do"(cuz it really depends on each project's need). The diagram explaining that concepts is added in the doc PR.
So, things like "how principal(user) id maps to the 3rd-party lib's record"(what we are discussing here), "how to define your casbin policy models", "how to optimize the model relations", etc...really depends on each project's business requirements. IMO an example app shouldn't be over engineered to advise on such decisions.
Back to your question, "But if I store stuff in Mongodb it will be a long uuid or guid...", since my app is created with memory db, I don't have this concern and I don't see any value of designing an example in a way that it is able to support all popular databases. Like I use opensesame
as the password for everyone to avoid dealing with designing a robust security system.
And in a real project, I don't know is it a good idea to have the back-end data coupled with the 3rd-party library. For example, what if I decide to not use casbin but use oauth0 instead? What if I change the casbin model and the subject uses a combination of 2 fields in the user model? This is also a reason why I'd like to make the back-end data agnostic of the 3rd-party lib.
WDYT?
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.
IMO, it's ok to keep the first version of this example app simple and assume that only memory datasource is used. We can improve the design to be more flexible (e.g. to support MongoDB) later - can you perhaps open a follow-up issue?
Having said that, I think it's also good to capture the concerns raised in this thread, for example as a code comment for getResourceName
and/or in the documentation we have for this example (README file, doc pages, etc.). Maybe we can create a new section "Known limitations" where we can list all issues we are aware of?
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.
Ok. We can leave it as-is for the first version. :)
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.
thanks there will be a few follow-up stories after this PR, will definitely include our discussion here. 🤝
7e96a48
to
01d6f81
Compare
@emonddr Thanks, I think I need to read up. Very helpful.
Not sure I like casbin personally. What were the other options when it was chosen? |
To save reviewer's time, I already summarized the user scenario or the original app at the very beginning, see the #App-description part in #4571 (comment) And I created another diagram with precreated data in
The app has very basic usage of Casbin:
The following diagram shows how this app defines casbin model and policy, different colors maps to different concepts. And one more diagram for the design of authorization: |
.vscode/launch.json
Outdated
@@ -13,7 +13,7 @@ | |||
"args": [ | |||
"--config", | |||
"${workspaceRoot}/packages/build/config/.mocharc.json", | |||
"packages/*/dist/__tests__/**/*.js", | |||
"examples/*/dist/__tests__/**/*.js", |
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.
Please revert this or include both packages
and examples
.
@@ -0,0 +1,25 @@ | |||
Copyright (c) IBM Corp. 2018,2019. All Rights Reserved. | |||
Node module: @loopback/example-todo |
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.
Please fix module names.
@@ -0,0 +1,76 @@ | |||
# @loopback/example-todo |
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.
Wrong module name.
|
||
async function givenRunningApplication() { | ||
process.env.SEED_DATA = '1'; | ||
app = new AccessControlApplication(); |
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.
Please use:
app = new AccessControlApplication({
rest: givenHttpServerConfig(),
});
"eslint:fix": "npm run eslint -- --fix", | ||
"pretest": "npm run build", | ||
"test": "lb-mocha \"dist/__tests__/**/*.js\"", | ||
"test:dev": "lb-mocha --allow-console-logs dist/__tests__/**/*.js && npm run posttest", |
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.
it looks like posttest is not defined
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.
@fidgi good catch, removed.
66137af
to
4fde3f3
Compare
|
||
// Generate the user name according to the naming convention | ||
// in casbin policy | ||
// A use's name would be `u${id}` |
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.
@jannyHou . Still there ^
TokenServiceConstants.TOKEN_SECRET_VALUE, | ||
); | ||
this.bind(TokenServiceBindings.TOKEN_EXPIRES_IN).to( | ||
TokenServiceConstants.TOKEN_EXPIRES_IN_VALUE, |
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.
Still suggesting this...
// TBD: REFACTOR to a casbin util file | ||
// Generate the resource name according to the naming convention | ||
// in casbin policy | ||
function getResourceName(resource: string, id?: number): string { |
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.
What I am saying is that right now, the database id for a user is : 1,2,3, etc. But if I store stuff in Mongodb it will be a long uuid or guid, so you would need u0209b175-b39c-4dd4-a158-4dc81547e77e
in your casbin file. And if ever users are deleted and added to a database again, these guid values will be different, and so you will need to change your casbin files accordingly.
So having users in memory database:
"User": {
"1": "{\"id\":1,\"username\":\"John\",\"email\":\"[email protected]\"}",
"2": "{\"id\":2,\"username\":\"Jane\",\"email\":\"[email protected]\"}",
"3": "{\"id\":3,\"username\":\"Bob\",\"email\":\"[email protected]\"}"
},
and casbin file using u1
, u2
etc works for this example, but perhaps a more real world example would place
unique user names in the casbin file like: jannyhou88
and dremond71
, and in real database these users would have ID values like a guid or long int.
So what I am saying is that we should have a function that takes the securityId and queries the database for the unique userid, and this userid is used by cabin policy decisions.
const request: AuthorizationRequest = { | ||
subject, | ||
object, | ||
action: (metadata.scopes && metadata.scopes[0]) || 'execute', |
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.
Perhaps create a constant named DEFAULT_SCOPE-'execute', and use this constant instead.
a7eb839
to
4de8c3e
Compare
@@ -0,0 +1,80 @@ | |||
{ | |||
"name": "@loopback/example-access-control-migration", | |||
"version": "1.9.5", |
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.
1.0.0?
"casbin": "^3.1.0", | ||
"jsonwebtoken": "^8.5.1", | ||
"loopback-connector-rest": "^3.6.0", | ||
"path": "^0.12.7" |
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.
Why path
? We should use the one from node core.
<html lang="en"> | ||
|
||
<head> | ||
<title>@loopback/example-todo</title> |
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.
Pls fix the title.
|
||
<body> | ||
<div class="info"> | ||
<h1>@loopback/example-todo</h1> |
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.
Same here.
1cffe6d
to
7ed7254
Compare
@@ -0,0 +1,25 @@ | |||
Copyright (c) IBM Corp. 2018,2019. All Rights Reserved. |
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.
Pls fix the header to be 2020
.
|
||
## Overview | ||
|
||
This tutorial demonstrates how to implement a RBAC(role based access control) |
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.
RBAC (Role Based Access Control)
?
## Overview | ||
|
||
This tutorial demonstrates how to implement a RBAC(role based access control) | ||
system and provides 5 endpoints to test different role's permissions. The |
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.
different roles' permissions
?
|
||
This tutorial demonstrates how to implement a RBAC(role based access control) | ||
system and provides 5 endpoints to test different role's permissions. The | ||
tutorial of building it from a dummy application is documented in |
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.
is documented
-> 'documented`
|
||
First, you'll need to install a supported version of Node: | ||
|
||
- [Node.js](https://nodejs.org/en/) at v10.19.0 or greater |
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.
v10
Feel free to look around in the application's code to get a feel for how it | ||
works. If you're interested in learning how to build it step-by-step, then | ||
continue with this tutorial! | ||
Then try each role's permission by following the |
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.
try it out
|
||
// authentication | ||
registerAuthenticationStrategy(this, JWTAuthenticationStrategy); | ||
} |
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.
Maybe we should move setupBindings
to a local component inside this app. This will show the possibility to have ACLs from an extension package in the future.
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.
@raymondfeng yeah moving the authentication setup to a component/extension is tracked in discussion #4571 (comment), I will create a follow-up story to extract the component.
export const USER_SERVICE = BindingKey.create<UserService<User, Credentials>>( | ||
'services.user.service', | ||
); | ||
} |
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.
A good candidate to be moved into the local component too.
projectId, | ||
); | ||
// resourceId will override the resource name from metadata | ||
authorizationCtx.resourceId = resourceName; |
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.
Nitpick: would it be better to bind to the invocationContext instead of a property?
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.
@raymondfeng Hmm not sure what's the difference between "to the invocationContext" and "to a property", the property resourceId
is a key(property) of invocationContext
already..
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.
It’s a property on authorization context. I was referring to bind to invocation context.
FYI - Looking at the coverage complain, all of them are for services. Since I will extract the authentication related services into a component, will add unit tests for those services there. |
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.
I tried it out on my end along with the tutorial #4706. It makes sense to me even I am not familiar with the implementation of auth systems. Great job 👏
7ed7254
to
6cbf702
Compare
metadata.resource ?? authorizationCtx.resource, | ||
projectId, | ||
); | ||
// resourceId will override the resource name from metadata | ||
authorizationCtx.resourceId = resourceName; | ||
authorizationCtx.invocationContext.bind('resourceId').to(resourceId); |
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.
Nitpick: do you want to define a constant for the key?
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.
Great work!
bd5ee68
to
76bb1e6
Compare
Implements #4520
This example migrates the lb3 acl example to lb4 https://github.com/strongloop/loopback-example-access-control
App Description
This app is a simple management system with RBAC for authorization.
3 models: User, Team, Project
Here is the rules for the 5 corresponding endpoints:
Where:
class level
means : /projectsinstance level
means: /project{id}Integrate with Casbin
The current design uses casbin system to implement the ROLE MAPPING.
It collects subject, object, operation from request, then the authorizer calls casbin enforcers to make decision.
For example: Operation "show-balance" only allows a project's
owner
orteam
to click. When a request comes in, the authorizer will examine the owner policy(stored in rbac_policy.admin.csv) and team policy(stored in rbac_policy.owner.csv) to make decision: if user X is theowner
ORteam
member of project Y, then it can see the balance.DESIGN QUESTION:
Storing instance level policies in casbin means we need EXTRA effort to update the policies when creating data. For example, when a user(id=1) creates a new project(id=3):
Is this the expected design?
cc: I would like to hear more feedback from @strongloop/loopback-maintainers
For Reviewers
There are amount of app setup codes(models, repos, authentication setup) which are not related to core logic of authorization and access control. The following files are what reviewers should focus on:
controllers/project.controller.ts
: especially the ACL rules defined at the beginningservices/casbin.authorizer.ts
: the core logic of authorizerservices/casbin.enforcers.ts
: the core logic of casbin enforcersservices/assign-project-instance-id.voter.ts
:fixture/casbin
__tests__/acceptance/project.acceptance.ts
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈