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

Validate Roles #603

Open
3 of 7 tasks
mononoken opened this issue Mar 28, 2024 · 3 comments
Open
3 of 7 tasks

Validate Roles #603

mononoken opened this issue Mar 28, 2024 · 3 comments
Labels
on hold Further investigation/decision-making is required

Comments

@mononoken
Copy link
Collaborator

mononoken commented Mar 28, 2024

If you can think of additional validations, please list them. We can release this for work once we have a good consensus on the validations we want.

Description

Roles have minimal validations, and some of them may not even be desirable. This is bad because we are currently vulnerable to developer mistakes or even malicious form manipulation to assign non-existent roles or other undesirable behavior. Authz are currently set to prevent such role problems from being an actual problem in regards to our data, but the roles themselves are vulnerable.

For example, if you try this in bin/rails c --sandbox:

> User.last.add_role(:wizard)
> User.last.roles
[#<Role:0x00000001112558c0
  id: 5,
  name: "staff",
  resource_type: "Organization",
  resource_id: 1,
  created_at: Thu, 28 Mar 2024 04:34:05.742340000 UTC +00:00,
  updated_at: Thu, 28 Mar 2024 04:34:05.742340000 UTC +00:00>,
 #<Role:0x00000001112557f8
  id: 6,
  name: "wizard",
  resource_type: nil,
  resource_id: nil,
  created_at: Thu, 28 Mar 2024 06:48:51.940040000 UTC +00:00,
  updated_at: Thu, 28 Mar 2024 06:48:51.940040000 UTC +00:00>]

We see two problems above. One, wizards don't exist in this app. Two, the role validations currently permit resources to be nil (which means any checks that forget to check for resource scoping would pass).

Note: The acceptance criteria includes some validations that may already exist, but we should confirm that rolify is enforcing them as we expect or else implement ourselves.

Acceptance criteria

Add validations for these to models/role:

  • Duplicate roles with the same name and resource cannot be created (I think this is implemented by rolify's default, but we need to confirm so)
  • Limit roles to the following: [:adopter, :fosterer, :staff, :admin] (inclusion validation will be useful)
  • Resource (organization) must match User's organization
  • Name cannot be nil (this may be covered by above inclusion validation)
  • Resource type cannot be nil
  • Resource cannot be nil

Others?

  • User cannot have duplicate roles (think rolify also covers this but confirm)
@mononoken mononoken added question Further information is requested on hold Further investigation/decision-making is required labels Mar 28, 2024
@mononoken mononoken self-assigned this Apr 14, 2024
@mononoken mononoken removed on hold Further investigation/decision-making is required question Further information is requested labels Apr 14, 2024
@mononoken mononoken added the on hold Further investigation/decision-making is required label Apr 29, 2024
@mononoken mononoken removed their assignment Apr 29, 2024
@mononoken
Copy link
Collaborator Author

Putting this on hold for now. I did start this and have three of the acceptance criteria completed in this branch. We might be able to start from it when this issue is necessary. https://github.com/mononoken/pet-rescue/tree/603_validate_roles

@mononoken
Copy link
Collaborator Author

These validations can wait until after #679 is complete.

@jmilljr24
Copy link
Collaborator

jmilljr24 commented May 12, 2024

@mononoken Is there any use case where a User would have both staff and admin role? I don't see it being applicable in the current implementation, however in factories>users.rb it builds a user with both staff and admin roles. I am currently implementing role changes #615 and wanted to confirm I wasn't missing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold Further investigation/decision-making is required
Projects
None yet
Development

No branches or pull requests

2 participants