Skip to content
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 users with roles and finish role-based page authorization #3002

Closed
wants to merge 77 commits into from

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Dec 15, 2023

Still under a feature flag, as runtime RPC protection is needed next and can be done in a separate PR.

List of features:

  • Assign roles to users individually by email (I considered several options and this seemed like the best one, eventually we could allow for importing CSVs and such. It does not seem possible in @auth/core to map roles from a service and I'm not sure if that would even be desirable or straightforward for users).
  • Disable users from accessing pages if they don't have the correct roles (client-side only, as our routing for these pages is client-side? Should we also try to block by HTTP somehow or it's not worth it?).
  • Hide inaccessible pages in sidebar + show message that page is inaccessible if user somehow navigates to them.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 27, 2023
@apedroferreira apedroferreira changed the title [WIP] Assign roles to users for authorization Assign roles to users for authorization Dec 27, 2023
@apedroferreira apedroferreira marked this pull request as ready for review December 27, 2023 17:42
@apedroferreira apedroferreira changed the title Assign roles to users for authorization Add users with roles and finish page-level role-based authorization Dec 27, 2023
@apedroferreira apedroferreira changed the title Add users with roles and finish page-level role-based authorization Add users with roles and finish pages role-based authorization Dec 27, 2023
@apedroferreira apedroferreira changed the title Add users with roles and finish pages role-based authorization Add users with roles and finish role-based page authorization Dec 27, 2023
@@ -47,6 +75,31 @@
]
},
"description": "Available roles for this application. These can be assigned to users."
},
"users": {
Copy link
Member

@Janpot Janpot Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can start with authentication only for this PR.

I don't think it makes sense at the moment for the toolpad configuration to act as a user pool. This would require developers to have to hardcode each user in the configuration. And likely be a privacy problem for public repos.

We will create a role mapping once we have an authentication provider that supports passing roles. So not for GitHub/Google. We don't support RBAC for our free version anyway.

edit: just realized that the authentication part of this PR is stacked from another PR. The comment still stands though. We won't support rbac for the free version. We should probably add azure AD provider first before creating a role mapping.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can keep most of this PR on hold if that's the best course and see what can be merged for now.
We can also have a more in-depth discussion about this and see if there's any more solutions we can agree on.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 2, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 2, 2024
Comment on lines +52 to +55
id: profile.email ?? String(profile.id),
name: profile.name,
email: profile.email,
image: profile.avatar_url,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id: profile.id.toString(),
name: profile.name ?? profile.login,
email: profile.email,
image: profile.avatar_url,

const roles = await getProfileRoles(profile.email, project);

return {
id: profile.email,
Copy link
Member Author

@apedroferreira apedroferreira Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
id: profile.email,
id: profile.sub,

?

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 7, 2024
@apedroferreira
Copy link
Member Author

Replaced by #3077, can revisit later if we decide to add our own role assignments.

@apedroferreira apedroferreira deleted the auth-roles branch January 16, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants