rubyforgood / homeward-tails

Homeward Tails is an application making it easy to link adopters/fosters with pets. We work with grassroots pet rescue organizations to understand how we can make the most impact.
MIT License
68 stars 112 forks source link

Validate Roles #603

Open mononoken opened 7 months ago

mononoken commented 7 months ago

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:

mononoken commented 6 months ago

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 commented 6 months ago

These validations can wait until after #679 is complete.

jmilljr24 commented 5 months ago

@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.