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

fix(examples): fix access control model for Users. #10102

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rikrose
Copy link

@rikrose rikrose commented Dec 20, 2024

In the multi-tenant example, the access control model is broken - a tenant-admin could create, edit and delete super-admins, and make other users and themselves in to super-admins. This contribution fixes it so that:

  • Only super-admins can create/edit/delete super-admins.
  • Restrict tenant-admins from activities outside tenants they are admins for. Have not verified that this stops tenant-admins from editing users outside of their own tenants.
  • Removes the button to Create/Delete in the admin console if the user is not a super-admin or tenant-admin.

Link to discussion on Discord: https://discord.com/channels/967097582721572934/1319199782996283443

…can create/edit/delete super-admins. Restrict tenant-admins from activities outside tenants they are admins for. Have not verified that this stops tenant-admins from editing users outside of their own tenants.
@rikrose
Copy link
Author

rikrose commented Dec 20, 2024

Basically the same logic in both changed files, which are only for access control to the Users collection in the multi-tenant example.

@rikrose rikrose marked this pull request as ready for review December 20, 2024 16:15
@rikrose
Copy link
Author

rikrose commented Dec 20, 2024

Fixing formatting, per suggestion on Discord. Also took the opportunity to the same logic phrased the same way in both files.

Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It still might be improved more. It would be nice if we can still return queries, but I'm not sure if we can cover the different situations that way.


export const updateAndDeleteAccess: Access = (args) => {
const { req } = args
const { req, data }: { req: any; data: User } = args
Copy link
Contributor

Choose a reason for hiding this comment

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

This should already be properly typed from args as PayloadRequest.

Copy link
Author

Choose a reason for hiding this comment

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

I will make this change.

Copy link
Author

Choose a reason for hiding this comment

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

Change made with most recent update.

}

// Can delete a user, only if you're an admin in *all* of their tenants
return data.tenants?.every((tenant) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

What if I'm a tenant admin and I need to remove admin access for another user who is also an admin of another?

Copy link
Author

Choose a reason for hiding this comment

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

This is a use-case not covered, yet, and talking about it has made me remember that this change would allow a tentant-admin to delete a user provided that they aren't also a super-user and also not a tentant-admin of any shared tenants. This does not cover the case of being an admin of another tenant.

It's possible that some query magic might be able to cover the case, but I haven't managed that yet.

I will work on this some more.

Copy link
Author

Choose a reason for hiding this comment

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

I have separated the delete and update access check functions. The delete function checks for super-admin, and otherwise returns false. The update check function checks that the only affected tenants are ones that the requesting user is a tenant-admin of.

…te. tenant-admins can update tenancy lists for tenants that they're admins of.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants