-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
Add RBAC with Azure AD authentication provider #3077
Add RBAC with Azure AD authentication provider #3077
Conversation
package.json
Outdated
@@ -98,7 +98,7 @@ | |||
"engines": { | |||
"npm": "please-use-yarn", | |||
"node": ">=18", | |||
"pnpm": "8.7.0" | |||
"pnpm": ">=8.7.0" |
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 this fine or should I use this specific version of pnpm
? I had a more recent one installed.
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 was hoping we could pin the pnpm version, just like we used to do with yarn
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.
So I should use 8.7.0? Why are we not using a more recent version?
Anyway I can switch, no problem, I guess it's safer against weird issues if we all use the same 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.
Why are we not using a more recent version?
Because I haven't yet fully figured out yet how to enforce this in circleci, netlify, ánd codesandbox ci simultaneously.
|
|
||
const SKIP_VERIFICATION_PROVIDERS: AuthProvider[] = [ | ||
// Azure AD should be fine to skip as the user has to belong to the organization to sign in | ||
'azure-ad', |
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.
Also they don't return any data that allows us to determine if emails are verified, from what I searched.
docs/schemas/v1/definitions.json
Outdated
@@ -82,6 +83,26 @@ | |||
] | |||
}, | |||
"description": "Available roles for this application. These can be assigned to users." | |||
}, | |||
"roleMappings": { |
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.
Intuitively this feels like a provider specific property. e.g. how would this work under Google/GitHub?
I'd sort of have expected something like:
providers:
- provider: 'azure-ad'
roles:
- source:
- my-azure-role
- my-other-azure-role
target: my-toolpad-role
Also, UI doesn't really have proirity right now. I'm fine to release this with config only
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.
You're right, I'll make that change, seems better. The UI is already done so should be easy to keep now.
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've changed the schema here: 2075577
The schema definition is much better like this too.
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.
👍 Looking good
application.yml
.Will add to documentation in the authentication/authorization documentation PR: #3067
Also to cover in authentication/authorization tests in: #3056
Video
Screen.Recording.2024-01-16.at.18.30.53.mov
It's possible to assign multiple provider roles to a Toolpad role by separating the roles with commas such as
admin, reader,writer
.