Add user roles #349

reefdog · 2022-09-20T00:22:23Z

This PR adds the concept of user roles to the site, and converts our determination of "what is an admin?" to use it.

It adds the Rolify gem (as determined in #282), which lets us add roles ad-hoc both "globally" (applies to the user everywhere) and per-resource (e.g. "user can moderate this particular archive item"), although we're only using the global roles. Roles are non-exclusive; a user can have multiple roles at once.

We currently have four roles:

new_user is assigned during User creation, and it semantically means "this user has not yet completed their setup process". This role is removed when the user follows the link in their email and chooses their password.
insights_user is also assigned during User creation, and it semantically means "this user is approved to use the Insights site". This is a little bit unnecessary since all users can use the site, but I think it's good to be explicit and look to a future where this might not be the case (e.g., Vault-only users).
media_vault_user are users who have the right to use Media Vault
admin are users who are administrators of the site

The only role that is currently used for any gatekeeping is admin, which replaces the super_admin attribute on the User model. Rolify provides dynamic methods (defined during app boot and during new role creation, not through "method missing") for each role, e.g. is_admin? or is_new_user?, and we make use of those.

⚠️ Note that this PR adds no new functionality; it is just a clean swap of the old system with roles. Future changes will actually use the roles for more gatekeeping (e.g., #300).

Testing:

🚨 bundle install
🚨 rails db:reset ← you really should do a full reset if possible, to get all the new seeds
✅ rails t of course
Run the app and login with the standard [email protected] and [email protected]
✅ Nothing should be different from before! Login should work, only admins should get the Jobs page in their user menu, etc.

Closes #299

Rolify lets us add “global” and resource-specific roles to our User model. Issue #299

This commit begins the process of adding roles to the User model by: - Generating the role boilerplate and migrations - Configuring Rolify (specifically, enabling dynamic shortcuts so we can do queries like `user.is_admin?` or `user.is_insights_user?`) - Adding Role fixtures It also moves the Devise stuff in the User model up next to Rolify, with the idea that this sort of model-inheritance stuff should come before the model-definition stuff. Issue #299

This is a consequential commit. Previously, we were defining admins by the presence of a `super_admin` attribute on the `User` model. Now, we’re using Rolify with an `admin` role. Consequently, this commit: - Adds a (reversible) migration which finds all users with the `super_admin` attribute and adds the `:admin` role to them - Updates the seeds file to create the admin user - Removes the `User#super_admin?` method and converts all use of it to the `User#is_admin?` dynamic method created by Rolify - Updates the `User` fixtures and tests - Updates the `ARCHITECTURE.md` user model documentation Issue #299

This commit adds the `new_user` and `insights_user` roles to users that we create from applicants. It doesn’t do this to all new users using an `after_create` callback on the `User` model, as suggested by the Rolify docs, because we want more control over when we assign these roles. Issue #299

The `new_user` role distinguishes users who haven’t been through setup yet from those who have. This commit removes that role during account creation. Issue #299

Issue #299

Prior to the existence of our roles and the `new_user` role, we were using `user.sign_in_count` as a proxy for whether a user was already setup, and thus whether we could send them setup instructions. Now, we use the existence of the `new_user` role instead, as it’s more explicitly intended for this. Issue #299

reefdog · 2022-09-20T04:17:01Z

This PR is tripping up Rubocop with two offenses:

Offenses:

app/models/role.rb:2:3: C: Rails/HasAndBelongsToMany: Prefer has_many :through to has_and_belongs_to_many.
  has_and_belongs_to_many :users, join_table: :users_roles
  ^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20220919194501_rolify_create_roles.rb:10:5: C: Rails/CreateTableWithTimestamps: Add timestamps when creating a new table.
    create_table "users_roles", id: false do |t| ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I'd like to create the following two exceptions:

Rails/HasAndBelongsToMany:
  Enabled: true
  Exclude:
    # Rolify uses HABTM. Despite a decade of the community attempting to implement
    # `has_many: through`, it still struggles mightily with it. Let's make an exception.
    - app/models/role.rb

Rolify uses HABTM and, judging by the literal decade of issues and PRs attempting to implement has_many :through, the community hasn't been able to make it work correctly. (See: PR#181, PR#571, PR#583, #318, #411…) I think we're going to really twist ourselves in knots trying to get around it.

We should either ditch Rolify over this, or make an exception for this case.

# Join tables don't really need timestamps
Rails/CreateTableWithTimestamps:
  Exclude:
    - db/migrate/20220919194501_rolify_create_roles.rb

This one's pretty straightforward. We could create timestamps, but as that preexisting comment says, a true dumb join table doesn't really need them.

cguess · 2022-09-20T12:05:52Z

Running tests gives me two errors (below):


Error:
ApiKeyTest#test_deleting_a_user_deletes_all_of_its_api_keys:
StandardError: No fixture named 'user3' found for fixture set 'users'
    test/models/api_key_test.rb:18:in `block in <class:ApiKeyTest>'


rails test test/models/api_key_test.rb:17

..........E

Error:
ArchiveItemTest#test_destroying_a_user_resets_the_submitter_id_of_ArchiveItems_it_created:
StandardError: No fixture named 'user3' found for fixture set 'users'
    test/models/archive_item_test.rb:16:in `block in <class:ArchiveItemTest>'
    test/models/archive_item_test.rb:11:in `block in around'
    test/models/archive_item_test.rb:10:in `around'


rails test test/models/archive_item_test.rb:15

cguess · 2022-09-20T12:06:36Z

This PR is tripping up Rubocop with two offenses:
Offenses:

app/models/role.rb:2:3: C: Rails/HasAndBelongsToMany: Prefer has_many :through to has_and_belongs_to_many.
  has_and_belongs_to_many :users, join_table: :users_roles
  ^^^^^^^^^^^^^^^^^^^^^^^
db/migrate/20220919194501_rolify_create_roles.rb:10:5: C: Rails/CreateTableWithTimestamps: Add timestamps when creating a new table.
    create_table "users_roles", id: false do |t| ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I'd like to create the following two exceptions:
Rails/HasAndBelongsToMany:
  Enabled: true
  Exclude:
    # Rolify uses HABTM. Despite a decade of the community attempting to implement
    # `has_many: through`, it still struggles mightily with it. Let's make an exception.
    - app/models/role.rb
Rolify uses HABTM and, judging by the literal decade of issues and PRs attempting to implement has_many :through, the community hasn't been able to make it work correctly. (See: PR#181, PR#571, PR#583, #318, #411…) I think we're going to really twist ourselves in knots trying to get around it.

We should either ditch Rolify over this, or make an exception for this case.
# Join tables don't really need timestamps
Rails/CreateTableWithTimestamps:
  Exclude:
    - db/migrate/20220919194501_rolify_create_roles.rb
This one's pretty straightforward. We could create timestamps, but as that preexisting comment says, a true dumb join table doesn't really need them.

Do it, sounds good to me!

Now that we have roles, we can target users more specifically for specific tests. We also don’t need some of the duplicate test fixtures. Issue #299

Since adding Rolify, we are triggering two significant Rubocop offenses: - `has_and_belongs_to_many` is frowned upon in favor of `has_many :through`, but Rolify still uses the former. There have been many attempts to resolve this (see the Rolify issues and PRs) over the past decade, but none have fully landed. It’s beyond us to try and wrestle this into existence, so we’re going to make an exception and move on. - The HABTM join table has no timestamps, which is also frowned upon. We genuinely don’t care, however, and so are making an exception. Issue #299

reefdog · 2022-09-20T13:01:53Z

Running tests gives me two errors (below):

😱 You're right! I'm not sure how I missed this, apologies. Didn't fully complete the references to the new users fixtures. Fixed and force-pushed!

[Suggested Rubocop changes]

Do it, sounds good to me!

Done! This checks clean now.

Could I get a re-review @cguess?

oneroyalace

LGTM

cguess

One little question, but otherwise you're good to go.

config/initializers/rolify.rb

@cguess

This commit addresses some excellent @cguess feedback in PR #349: - Disable the Rolify `config.remove_role_if_empty`, which would purge unused roles automatically once the last resource used them. We don’t want this until we know that we do. - Seeds the database with the actual known/defined roles. - Updates the documentation about our architecture with the roles. Issue #299

reefdog added 3 commits September 19, 2022 14:44

Add the Rolify gem

8b5fa0d

Rolify lets us add “global” and resource-specific roles to our User model. Issue #299

reefdog added Requires Migration Requires a `rails db:migrate` Requires Bundler Install Requires a `bundle install` labels Sep 20, 2022

reefdog requested review from cguess and oneroyalace September 20, 2022 00:22

reefdog self-assigned this Sep 20, 2022

reefdog added 4 commits September 19, 2022 19:33

Remove new_user role during account creation

27df82f

The `new_user` role distinguishes users who haven’t been through setup yet from those who have. This commit removes that role during account creation. Issue #299

Add tests for Insights and MediaVault roles

baf8952

Issue #299

reefdog force-pushed the 299-add-user-roles branch from 333f161 to ae85c53 Compare September 20, 2022 00:33

reefdog added 2 commits September 20, 2022 07:57

Clean up users fixtures and update tests

f9a9cfd

Now that we have roles, we can target users more specifically for specific tests. We also don’t need some of the duplicate test fixtures. Issue #299

reefdog force-pushed the 299-add-user-roles branch from ae85c53 to d3a4f3e Compare September 20, 2022 13:00

reefdog mentioned this pull request Sep 20, 2022

Remove user.restricted attribute #353

Closed

oneroyalace approved these changes Sep 20, 2022

View reviewed changes

cguess approved these changes Sep 20, 2022

View reviewed changes

config/initializers/rolify.rb Outdated Show resolved Hide resolved

cguess approved these changes Sep 20, 2022

View reviewed changes

reefdog merged commit af57aed into master Sep 20, 2022

reefdog deleted the 299-add-user-roles branch September 20, 2022 15:51

reefdog mentioned this pull request Sep 20, 2022

Add additional MediaVault-specific permissions to users if needed #300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user roles #349

Add user roles #349

reefdog commented Sep 20, 2022 •

edited

Loading

reefdog commented Sep 20, 2022 •

edited

Loading

cguess commented Sep 20, 2022

cguess commented Sep 20, 2022

reefdog commented Sep 20, 2022

oneroyalace left a comment

cguess left a comment

Add user roles #349

Add user roles #349

Conversation

reefdog commented Sep 20, 2022 • edited Loading

reefdog commented Sep 20, 2022 • edited Loading

cguess commented Sep 20, 2022

cguess commented Sep 20, 2022

reefdog commented Sep 20, 2022

oneroyalace left a comment

Choose a reason for hiding this comment

cguess left a comment

Choose a reason for hiding this comment

reefdog commented Sep 20, 2022 •

edited

Loading

reefdog commented Sep 20, 2022 •

edited

Loading