From 8b5fa0db7cb946846d30801852e0fd7b55ca2f4c Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Mon, 19 Sep 2022 14:44:28 -0500 Subject: [PATCH 01/10] Add the Rolify gem MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rolify lets us add “global” and resource-specific roles to our User model. Issue #299 --- Gemfile | 3 +++ Gemfile.lock | 2 ++ 2 files changed, 5 insertions(+) diff --git a/Gemfile b/Gemfile index f65ac3ef..65c11be0 100644 --- a/Gemfile +++ b/Gemfile @@ -178,3 +178,6 @@ gem "country_select", "~> 8.0" # Used for sending email through Mailgun gem "mailgun-ruby", "~> 1.2" + +# Rolify is used for user roles +gem "rolify", "~> 6.0" diff --git a/Gemfile.lock b/Gemfile.lock index f616575f..122163ab 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -326,6 +326,7 @@ GEM mime-types (>= 1.16, < 4.0) netrc (~> 0.8) rexml (3.2.5) + rolify (6.0.0) rubocop (1.28.1) parallel (~> 1.10) parser (>= 3.1.0.0) @@ -506,6 +507,7 @@ DEPENDENCIES rails (~> 7.0.2.3) rake redis (~> 4.0) + rolify (~> 6.0) rubocop rubocop-minitest rubocop-performance From 9acb2dd78a247715a6f6301af61ec1d82de3ae6a Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Mon, 19 Sep 2022 15:27:49 -0500 Subject: [PATCH 02/10] Rolify the User model 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 --- app/models/role.rb | 13 ++++++++++++ app/models/user.rb | 11 +++++----- config/initializers/rolify.rb | 10 ++++++++++ .../20220919194501_rolify_create_roles.rb | 18 +++++++++++++++++ db/schema.rb | 20 ++++++++++++++++++- test/fixtures/roles.yml | 13 ++++++++++++ test/models/role_test.rb | 7 +++++++ 7 files changed, 85 insertions(+), 7 deletions(-) create mode 100644 app/models/role.rb create mode 100644 config/initializers/rolify.rb create mode 100644 db/migrate/20220919194501_rolify_create_roles.rb create mode 100644 test/fixtures/roles.yml create mode 100644 test/models/role_test.rb diff --git a/app/models/role.rb b/app/models/role.rb new file mode 100644 index 00000000..8ae3ff9f --- /dev/null +++ b/app/models/role.rb @@ -0,0 +1,13 @@ +class Role < ApplicationRecord + has_and_belongs_to_many :users, join_table: :users_roles + + belongs_to :resource, + polymorphic: true, + optional: true + + validates :resource_type, + inclusion: { in: Rolify.resource_types }, + allow_nil: true + + scopify +end diff --git a/app/models/user.rb b/app/models/user.rb index 87264d7c..19604aec 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,6 +1,11 @@ # typed: strict class User < ApplicationRecord + rolify + devise :database_authenticatable, :registerable, + :recoverable, :rememberable, :validatable, + :trackable, :lockable, :confirmable + has_many :api_keys, dependent: :delete_all has_many :archive_items, foreign_key: :submitter_id, dependent: :nullify @@ -9,12 +14,6 @@ class User < ApplicationRecord has_one :applicant, dependent: :destroy - # Include default devise modules. Others available are: - # :timeoutable and :omniauthable - devise :database_authenticatable, :registerable, - :recoverable, :rememberable, :validatable, - :trackable, :lockable, :confirmable - sig { returns(T::Boolean) } def super_admin? self.super_admin diff --git a/config/initializers/rolify.rb b/config/initializers/rolify.rb new file mode 100644 index 00000000..d1234097 --- /dev/null +++ b/config/initializers/rolify.rb @@ -0,0 +1,10 @@ +Rolify.configure do |config| + # By default ORM adapter is ActiveRecord. uncomment to use mongoid + # config.use_mongoid + + # Dynamic shortcuts for User class (user.is_admin? like methods). Default is: false + config.use_dynamic_shortcuts + + # Configuration to remove roles from database once the last resource is removed. Default is: true + # config.remove_role_if_empty = false +end diff --git a/db/migrate/20220919194501_rolify_create_roles.rb b/db/migrate/20220919194501_rolify_create_roles.rb new file mode 100644 index 00000000..828b44e1 --- /dev/null +++ b/db/migrate/20220919194501_rolify_create_roles.rb @@ -0,0 +1,18 @@ +class RolifyCreateRoles < ActiveRecord::Migration[7.0] + def change + create_table "roles", id: :uuid do |t| + t.string :name + t.references :resource, polymorphic: true + + t.timestamps + end + + create_table "users_roles", id: false do |t| + t.references :user, type: :uuid + t.references :role, type: :uuid + end + + add_index :roles, [ :name, :resource_type, :resource_id ] + add_index :users_roles, [ :user_id, :role_id ] + end +end diff --git a/db/schema.rb b/db/schema.rb index da71b138..7f20f52f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2022_09_12_215054) do +ActiveRecord::Schema[7.0].define(version: 2022_09_19_194501) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -197,6 +197,16 @@ t.index ["searchable_type", "searchable_id"], name: "index_pg_search_documents_on_searchable" end + create_table "roles", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.string "name" + t.string "resource_type" + t.bigint "resource_id" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["name", "resource_type", "resource_id"], name: "index_roles_on_name_and_resource_type_and_resource_id" + t.index ["resource_type", "resource_id"], name: "index_roles_on_resource" + end + create_table "scrapes", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.boolean "fulfilled", default: false, null: false t.string "url", null: false @@ -292,6 +302,14 @@ t.index ["unlock_token"], name: "index_users_on_unlock_token", unique: true end + create_table "users_roles", id: false, force: :cascade do |t| + t.uuid "user_id" + t.uuid "role_id" + t.index ["role_id"], name: "index_users_roles_on_role_id" + t.index ["user_id", "role_id"], name: "index_users_roles_on_user_id_and_role_id" + t.index ["user_id"], name: "index_users_roles_on_user_id" + end + create_table "youtube_channels", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "title", null: false t.string "youtube_id", null: false diff --git a/test/fixtures/roles.yml b/test/fixtures/roles.yml new file mode 100644 index 00000000..cdeb4ae7 --- /dev/null +++ b/test/fixtures/roles.yml @@ -0,0 +1,13 @@ +# Read about fixtures at https://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html + +new_user: + name: :new_user + +insights_user: + name: :insights_user + +media_vault_user: + name: :media_vault_user + +admin: + name: :admin diff --git a/test/models/role_test.rb b/test/models/role_test.rb new file mode 100644 index 00000000..5e1a9aeb --- /dev/null +++ b/test/models/role_test.rb @@ -0,0 +1,7 @@ +require "test_helper" + +class RoleTest < ActiveSupport::TestCase + # test "the truth" do + # assert true + # end +end From 60cec4dc0aaa00d1f2ab2cbf9a5333bedeb5b0b6 Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Mon, 19 Sep 2022 17:19:42 -0500 Subject: [PATCH 03/10] Use role instead of attribute to define admins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/controllers/application_controller.rb | 4 ++-- app/models/user.rb | 5 ----- app/views/layouts/_header.html.erb | 2 +- .../20220919214503_convert_admins_to_rolify.rb | 15 +++++++++++++++ db/schema.rb | 3 +-- db/seeds.rb | 4 ++-- docs/ARCHITECTURE.md | 2 +- test/fixtures/users.yml | 6 +++--- test/models/user_test.rb | 8 ++++++++ 9 files changed, 33 insertions(+), 16 deletions(-) create mode 100644 db/migrate/20220919214503_convert_admins_to_rolify.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 8946f86c..6bc93ffd 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -75,7 +75,7 @@ def authenticate_super_user # First we make sure they're logged in at all, this also sets the current user so we can check it return false unless authenticate_user! - current_user.super_admin? + current_user.is_admin? end sig { void } @@ -83,7 +83,7 @@ def authenticate_super_user! # First we make sure they're logged in at all, this also sets the current user so we can check it authenticate_user! - unless current_user.super_admin? + unless current_user.is_admin? redirect_back_or_to "/", allow_other_host: false, alert: "You must be a super user/admin to access this page." end end diff --git a/app/models/user.rb b/app/models/user.rb index 19604aec..4d57bf34 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -14,11 +14,6 @@ class User < ApplicationRecord has_one :applicant, dependent: :destroy - sig { returns(T::Boolean) } - def super_admin? - self.super_admin - end - # `Devise::Recoverable#set_reset_password_token` is a protected method, which prevents us from # calling it directly. Since we need to be able to do that for tests and for duck-punching other # `Devise::Recoverable` methods, we pull it into the public space here. diff --git a/app/views/layouts/_header.html.erb b/app/views/layouts/_header.html.erb index 04809b13..cf2ca736 100644 --- a/app/views/layouts/_header.html.erb +++ b/app/views/layouts/_header.html.erb @@ -18,7 +18,7 @@
  • <%= link_to "Settings", account_path, class: "block no-underline py-2 px-4 text-sm text-gray-700 hover:bg-gray-100 dark:hover:bg-gray-600 dark:text-gray-200 dark:hover:text-white" %>
    • - <% if current_user.super_admin? %> + <% if current_user.is_admin? %>
  • <%= link_to jobs_status_index_path, class: "flex flex-inline gap-1 block no-underline py-2 px-4 text-sm text-gray-700 hover:bg-gray-100 dark:hover:bg-gray-600 dark:text-gray-200 dark:hover:text-white" do %> diff --git a/db/migrate/20220919214503_convert_admins_to_rolify.rb b/db/migrate/20220919214503_convert_admins_to_rolify.rb new file mode 100644 index 00000000..df2406ab --- /dev/null +++ b/db/migrate/20220919214503_convert_admins_to_rolify.rb @@ -0,0 +1,15 @@ +class ConvertAdminsToRolify < ActiveRecord::Migration[7.0] + def up + User.where(super_admin: true).each { |admin| admin.add_role :admin } + + remove_column :users, :super_admin + end + def down + add_column :users, :super_admin, :boolean, default: false + + User.with_role(:admin).each do |admin| + admin.update super_admin: true + admin.remove_role :admin + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 7f20f52f..a65a56b4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2022_09_19_194501) do +ActiveRecord::Schema[7.0].define(version: 2022_09_19_214503) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -295,7 +295,6 @@ t.datetime "created_at", null: false t.datetime "updated_at", null: false t.boolean "restricted", default: false, null: false - t.boolean "super_admin", default: false t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["email"], name: "index_users_on_email", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true diff --git a/db/seeds.rb b/db/seeds.rb index 6f81e5bc..b5c2a478 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -11,12 +11,12 @@ easy_password = "password123" # Super-admin account; no applicant necessary. -User.create!({ +admin = User.create!({ email: "admin@example.com", password: easy_password, - super_admin: true, confirmed_at: Time.now, }) +admin.add_role :admin Applicant.create!([ # This applicant is a fresh, unconfirmed applicant. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 035042c7..8dad5524 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -21,7 +21,7 @@ Zenodotus allows its users to search its archive using image or text inputs. Sea ## User model -Zenodotus' `User` model handles authentication for the app via [Devise](https://github.com/heartcombo/devise). Internal users may be indicated as such with the `super_admin` boolean. +Zenodotus' `User` model handles authentication for the app via [Devise](https://github.com/heartcombo/devise). Roles are managed with the Rolify gem. Internal users have the `:admin` role and are recognized with the `is_admin?` helper (provided by Rolify automatically). ## MediaReview Coming soon diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 693ab191..c0d486a8 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -31,8 +31,8 @@ existing_user: confirmed_at: <%= Time.now %> sign_in_count: 1 -super_admin: - email: "super_admin@example.com" +admin: + email: "admin@example.com" encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> - super_admin: true confirmed_at: <%= Time.now %> + roles: admin diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 2f6e2069..5102af7c 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -1,6 +1,14 @@ require "test_helper" class UserTest < ActiveSupport::TestCase + test "should recognize admins" do + assert users(:admin).is_admin? + end + + test "should recognize users are not admins" do + assert_not users(:existing_user).is_admin? + end + test "can associate a user with an applicant" do user = users(:user1) applicant = applicants(:approved) From df97cee3883d033eaa209802e68bcfc3d0505110 Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Mon, 19 Sep 2022 18:58:49 -0500 Subject: [PATCH 04/10] Assign default roles to new users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/models/user.rb | 16 +++++++++++++++- test/fixtures/users.yml | 8 ++++++++ test/models/user_test.rb | 9 +++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 4d57bf34..c4acce82 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -48,7 +48,7 @@ def send_setup_instructions def self.create_from_applicant(applicant) raise ApplicantNotApprovedError unless applicant.approved? - self.create!({ + user = self.create!({ applicant: applicant, email: applicant.email, # The user will have to change their password immediately. This is just to pass validation. @@ -58,6 +58,20 @@ def self.create_from_applicant(applicant) confirmed_at: applicant.confirmed_at, confirmation_sent_at: applicant.confirmation_sent_at }) + + user.assign_default_roles + + user + end + + # All new users are implicitly Insights users. + # All new users are also "new" until they have completed their initial setup. + sig { void } + def assign_default_roles + if self.roles.blank? + self.add_role :new_user + self.add_role :insights_user + end end end diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index c0d486a8..057d46a6 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -31,6 +31,14 @@ existing_user: confirmed_at: <%= Time.now %> sign_in_count: 1 +new_user: + email: "new-user@example.com" + encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> + confirmed_at: <%= Time.now %> + roles: + - new_user + - insights_user + admin: email: "admin@example.com" encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 5102af7c..654e6786 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -1,6 +1,15 @@ require "test_helper" class UserTest < ActiveSupport::TestCase + test "should assign new users to default roles" do + approved_applicant = applicants(:approved) + + user = User.create_from_applicant(approved_applicant) + + assert user.is_new_user? + assert user.is_insights_user? + end + test "should recognize admins" do assert users(:admin).is_admin? end From 27df82fee9aac3b8cbc19eb54d9c5ee3a69d4806 Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Mon, 19 Sep 2022 18:59:39 -0500 Subject: [PATCH 05/10] Remove `new_user` role during account creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/controllers/accounts_controller.rb | 2 ++ test/controllers/accounts_controller_test.rb | 14 ++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 2b911eac..fe6e36fd 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -87,6 +87,8 @@ def create raise InvalidTokenError if @user.new_record? raise InvalidUpdatePasswordError if typed_params.password.blank? || @user.invalid? + @user.remove_role :new_user + sign_in @user redirect_to after_sign_in_path_for(@user) end diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index 6627aa7b..c5cd7512 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -108,4 +108,18 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest }) assert_response :bad_request end + + test "should remove new_user role during setup" do + user = users(:new_user) + + assert user.is_new_user? + + post create_account_path({ + reset_password_token: user.set_reset_password_token, + password: "password1", + password_confirmation: "password1" + }) + + assert_not user.is_new_user? + end end From baf895283a6bcb2222edd6c707538aab32a1e687 Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Mon, 19 Sep 2022 19:00:08 -0500 Subject: [PATCH 06/10] Add tests for Insights and MediaVault roles Issue #299 --- test/fixtures/users.yml | 15 +++++++++++++++ test/models/user_test.rb | 14 ++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 057d46a6..ac8b4982 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -39,6 +39,21 @@ new_user: - new_user - insights_user +insights_user: + email: "insights-only-user@example.com" + encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> + confirmed_at: <%= Time.now %> + roles: + - insights_user + +media_vault_user: + email: "media-vault-user@example.com" + encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> + confirmed_at: <%= Time.now %> + roles: + - insights_user + - media_vault_user + admin: email: "admin@example.com" encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 654e6786..320c5e55 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -10,6 +10,20 @@ class UserTest < ActiveSupport::TestCase assert user.is_insights_user? end + test "should recognize Insights-only users" do + insights_user = users(:insights_user) + + assert insights_user.is_insights_user? + assert_not insights_user.is_media_vault_user? + end + + test "should recognize MediaVault users" do + media_vault_user = users(:media_vault_user) + + assert media_vault_user.is_insights_user? + assert media_vault_user.is_media_vault_user? + end + test "should recognize admins" do assert users(:admin).is_admin? end From df4b8d40d576b7fdeca9b842e617d8b910533e70 Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Mon, 19 Sep 2022 19:05:51 -0500 Subject: [PATCH 07/10] Use `new_user` role for "already-setup check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/models/user.rb | 2 +- db/seeds.rb | 4 ---- test/fixtures/users.yml | 1 - test/models/user_test.rb | 2 +- 4 files changed, 2 insertions(+), 7 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index c4acce82..af259dd5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -27,7 +27,7 @@ def set_reset_password_token # Like the original method, it also creates the user's `reset_password_token`. sig { returns(String) } def send_setup_instructions - raise AlreadySetupError if sign_in_count.positive? + raise AlreadySetupError unless self.is_new_user? token = set_reset_password_token diff --git a/db/seeds.rb b/db/seeds.rb index b5c2a478..13e8cd35 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -84,8 +84,6 @@ # Override the randomized initial password. password: easy_password, password_confirmation: easy_password, - # Make sure they don't look fresh. - sign_in_count: 1, }) # Create the restricted user @@ -95,8 +93,6 @@ # Override the randomized initial password. password: easy_password, password_confirmation: easy_password, - # Make sure they don't look fresh. - sign_in_count: 1, }) Sources::Tweet.create_from_url "https://twitter.com/kairyssdal/status/1415029747826905090" diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index ac8b4982..39471b24 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -29,7 +29,6 @@ existing_user: email: "new@example.com" encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> confirmed_at: <%= Time.now %> - sign_in_count: 1 new_user: email: "new-user@example.com" diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 320c5e55..c543881a 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -103,7 +103,7 @@ class UserTest < ActiveSupport::TestCase end test "cannot send setup email to previously logged-in user" do - user = users(:existing_user) + user = users(:insights_user) assert_raises User::AlreadySetupError do user.send_setup_instructions From f9a9cfd26c5e41da71bddda7f5b4797fffee2c53 Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Mon, 19 Sep 2022 19:11:09 -0500 Subject: [PATCH 08/10] Clean up users fixtures and update tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- test/controllers/accounts_controller_test.rb | 14 ++++++------ test/controllers/archive_controller_test.rb | 2 +- .../jobs_tracker_controller_test.rb | 6 ++--- .../text_search_controller_test.rb | 2 +- test/fixtures/api_keys.yml | 2 +- test/fixtures/users.yml | 22 +++++-------------- test/helpers/accounts_helper.rb | 2 +- test/models/api_key_test.rb | 6 ++--- test/models/archive_item_test.rb | 6 ++--- test/models/image_search_test.rb | 4 ++-- test/models/user_test.rb | 10 ++++----- 11 files changed, 32 insertions(+), 44 deletions(-) diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index c5cd7512..ac77c9d1 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -4,7 +4,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest include Devise::Test::IntegrationHelpers test "can get the setup page with a valid token" do - user = users(:user1) + user = users(:user) token = user.set_reset_password_token @@ -21,7 +21,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest end test "cannot get setup page while logged in" do - user = users(:user1) + user = users(:user) # Use a valid token to make this a legitimate test. # The redirection will run before the token is even checked, but we want to make sure it's an @@ -36,7 +36,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest end test "can setup an account as a new user" do - user = users(:user1) + user = users(:user) post create_account_path({ reset_password_token: user.set_reset_password_token, @@ -57,7 +57,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest end test "cannot setup an account with a previously-used token" do - user = users(:user1) + user = users(:user) create_params = { reset_password_token: user.set_reset_password_token, @@ -76,7 +76,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest end test "cannot setup an account with blank passwords" do - user = users(:user1) + user = users(:user) post create_account_path({ reset_password_token: user.set_reset_password_token, @@ -87,7 +87,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest end test "cannot setup an account with short passwords" do - user = users(:user1) + user = users(:user) short_password = "password"[0, Devise.password_length.begin - 1] post create_account_path({ @@ -99,7 +99,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest end test "cannot setup an account with mismatching passwords" do - user = users(:user1) + user = users(:user) post create_account_path({ reset_password_token: user.set_reset_password_token, diff --git a/test/controllers/archive_controller_test.rb b/test/controllers/archive_controller_test.rb index 35e670c8..e88d08cf 100644 --- a/test/controllers/archive_controller_test.rb +++ b/test/controllers/archive_controller_test.rb @@ -24,7 +24,7 @@ def after_all end test "load index if authenticated" do - sign_in users(:user1) + sign_in users(:user) get root_url assert_response :success end diff --git a/test/controllers/jobs_tracker_controller_test.rb b/test/controllers/jobs_tracker_controller_test.rb index 292a75c5..850f469c 100644 --- a/test/controllers/jobs_tracker_controller_test.rb +++ b/test/controllers/jobs_tracker_controller_test.rb @@ -4,14 +4,14 @@ class JobsTrackerControllerTest < ActionDispatch::IntegrationTest include Devise::Test::IntegrationHelpers test "can view jobs" do - sign_in users(:user1) + sign_in users(:user) InstagramMediaSource.extract("https://www.instagram.com/p/CBcqOkyDDH8/", false) get jobs_status_index_path assert_response :success end test "can resubmit a scrape" do - sign_in users(:user1) + sign_in users(:user) scrape = Scrape.create({ fulfilled: false, url: "https://www.instagram.com/p/CBcqOkyDDH8/", scrape_type: :instagram }) assert_not_nil scrape @@ -25,7 +25,7 @@ class JobsTrackerControllerTest < ActionDispatch::IntegrationTest end test "can delete a scrape" do - sign_in users(:user1) + sign_in users(:user) scrape = Scrape.create({ fulfilled: false, url: "https://www.instagram.com/p/CBcqOkyDDH8/", scrape_type: :instagram }) assert_not_nil scrape diff --git a/test/controllers/text_search_controller_test.rb b/test/controllers/text_search_controller_test.rb index 99fa5c70..65fc22ab 100644 --- a/test/controllers/text_search_controller_test.rb +++ b/test/controllers/text_search_controller_test.rb @@ -11,7 +11,7 @@ class ArchiveControllerTest < ActionDispatch::IntegrationTest Sources::Tweet.create_from_url("https://twitter.com/ggreenwald/status/1430523746457112578") Sources::Tweet.create_from_url("https://twitter.com/bidenfoundation/status/1121446608040755200") Sources::Tweet.create_from_url("https://twitter.com/POTUS/status/1428115295756066824") - sign_in users(:user1) + sign_in users(:user) # And then search get text_search_submit_url, params: { query: "Biden" } diff --git a/test/fixtures/api_keys.yml b/test/fixtures/api_keys.yml index 6911fb87..ef6c42e2 100644 --- a/test/fixtures/api_keys.yml +++ b/test/fixtures/api_keys.yml @@ -6,4 +6,4 @@ # one: hashed_api_key: <%= ZenoEncryption.hash_string("123456789") %> - user: user1 + user: user diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 39471b24..ad1ab578 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -10,25 +10,13 @@ # two: {} # column: value -user1: - email: "test123@example.com" - encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> - confirmed_at: <%= Time.now %> - -user2: - email: "test456@example.com" - encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> - confirmed_at: <%= Time.now %> - -user3: - email: "test789@example.com" - encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> - confirmed_at: <%= Time.now %> - -existing_user: - email: "new@example.com" +user: + email: "user@example.com" encrypted_password: <%= Devise::Encryptor.digest(User, 'password') %> confirmed_at: <%= Time.now %> + roles: + - insights_user + - media_vault_user new_user: email: "new-user@example.com" diff --git a/test/helpers/accounts_helper.rb b/test/helpers/accounts_helper.rb index b5042ba8..082e6cc8 100644 --- a/test/helpers/accounts_helper.rb +++ b/test/helpers/accounts_helper.rb @@ -4,7 +4,7 @@ class AccountsHelperTest < ActionView::TestCase include Devise::Test::IntegrationHelpers test "search_history_helper" do - sign_in users(:user1) + sign_in users(:user) TextSearch.create(query: "search term", user: User.first) TextSearch.create(query: "another search term", user: User.first) TextSearch.create(query: "yet another search term", user: User.first) diff --git a/test/models/api_key_test.rb b/test/models/api_key_test.rb index 6971ba8f..25f0d22a 100644 --- a/test/models/api_key_test.rb +++ b/test/models/api_key_test.rb @@ -2,20 +2,20 @@ class ApiKeyTest < ActiveSupport::TestCase test "creating an api key returns the clear string of the key" do - api_key = ApiKey.create(user: users(:user1)) + api_key = ApiKey.create(user: users(:user)) assert_not_nil api_key.api_key assert_not_nil api_key.hashed_api_key end test "retrieving an api key does not return the clear string of the key" do - api_key = ApiKey.create(user: users(:user1)) + api_key = ApiKey.create(user: users(:user)) api_key = ApiKey.find(api_key.id) assert_nil api_key.api_key assert_not_nil api_key.hashed_api_key end test "deleting a user deletes all of its api keys" do - user = users(:user3) + user = users(:user) api_key = ApiKey.create(user: user) assert_not_nil api_key user.destroy! diff --git a/test/models/archive_item_test.rb b/test/models/archive_item_test.rb index 14ea287b..42a2f0b2 100644 --- a/test/models/archive_item_test.rb +++ b/test/models/archive_item_test.rb @@ -13,10 +13,10 @@ def around end test "destroying a user resets the submitter_id of ArchiveItems it created" do - sign_in users(:user3) - Sources::Tweet.create_from_url "https://twitter.com/unsung_son/status/1470963204855578626", users(:user3) + sign_in users(:user) + Sources::Tweet.create_from_url "https://twitter.com/unsung_son/status/1470963204855578626", users(:user) assert_not_nil ArchiveItem.first.submitter - User.destroy(users(:user3).id) + User.destroy(users(:user).id) assert_nil ArchiveItem.first.submitter end diff --git a/test/models/image_search_test.rb b/test/models/image_search_test.rb index fcc2fa82..105ddee0 100644 --- a/test/models/image_search_test.rb +++ b/test/models/image_search_test.rb @@ -7,8 +7,8 @@ class ImageSearchTest < ActiveSupport::TestCase def setup image_file = File.open("test/mocks/media/instagram_media_12765281-136d-4bfa-b7ad-e89f107b5769.jpg", binmode: true) video_file = File.open("test/mocks/media/youtube_media_23b12624-2ef2-4dcb-97d2-966aa9fcba80.mp4", binmode: true) - @image_search = ImageSearch.create!(image: image_file, user: users(:user1)) - @video_search = ImageSearch.create!(video: video_file, user: users(:user1)) + @image_search = ImageSearch.create!(image: image_file, user: users(:user)) + @video_search = ImageSearch.create!(video: video_file, user: users(:user)) Zelkova.graph.reset end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index c543881a..fb8005ea 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -29,11 +29,11 @@ class UserTest < ActiveSupport::TestCase end test "should recognize users are not admins" do - assert_not users(:existing_user).is_admin? + assert_not users(:user).is_admin? end test "can associate a user with an applicant" do - user = users(:user1) + user = users(:user) applicant = applicants(:approved) user.update(applicant: applicant) @@ -42,7 +42,7 @@ class UserTest < ActiveSupport::TestCase end test "destroying user destroys applicant too" do - user = users(:user1) + user = users(:user) applicant = applicants(:approved) user.update(applicant: applicant) @@ -82,14 +82,14 @@ class UserTest < ActiveSupport::TestCase end test "can create a reset password token" do - user = users(:user1) + user = users(:user) token = user.set_reset_password_token assert token end test "can look up a user with the reset token" do - user = users(:user1) + user = users(:user) token = user.set_reset_password_token assert_equal user, User.with_reset_password_token(token) From d3a4f3ef2595cf827990f58eb32ebc0ac5479b00 Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Tue, 20 Sep 2022 08:00:46 -0500 Subject: [PATCH 09/10] Add Rubocop exclusions for Rolify stuff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .rubocop.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.rubocop.yml b/.rubocop.yml index ea97a82e..6fc917f8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -19,7 +19,7 @@ AllCops: # Join tables don't really need timestamps Rails/CreateTableWithTimestamps: Exclude: - # - 'db/migrate/20180128231930_create_organizations_and_events.rb' + - db/migrate/20220919194501_rolify_create_roles.rb # Rails generates this file Style/BlockComments: @@ -64,6 +64,10 @@ Rails/HasManyOrHasOneDependent: 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 Style/NumericPredicate: Enabled: true From d27e0a8ec20cea205c413b99691855b5c01c7158 Mon Sep 17 00:00:00 2001 From: Justin Reese Date: Tue, 20 Sep 2022 10:49:22 -0500 Subject: [PATCH 10/10] Seed database with Rolify roles and tweak config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- config/initializers/rolify.rb | 8 +++++++- db/seeds.rb | 7 +++++++ docs/ARCHITECTURE.md | 9 ++++++++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/config/initializers/rolify.rb b/config/initializers/rolify.rb index d1234097..9caabbdc 100644 --- a/config/initializers/rolify.rb +++ b/config/initializers/rolify.rb @@ -3,8 +3,14 @@ # config.use_mongoid # Dynamic shortcuts for User class (user.is_admin? like methods). Default is: false + # + # Enabled because these are convenient methods, and according to the Rolify documentation they + # are generated at boot time (and when `add_role` is run), so shouldn't hurt performance. config.use_dynamic_shortcuts # Configuration to remove roles from database once the last resource is removed. Default is: true - # config.remove_role_if_empty = false + # + # Toggled to false because we have well-defined user roles that we don't want removed, even if + # the last user using them is deleted. + config.remove_role_if_empty = false end diff --git a/db/seeds.rb b/db/seeds.rb index 13e8cd35..473effd8 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -8,6 +8,13 @@ # movies = Movie.create([{ name: 'Star Wars' }, { name: 'Lord of the Rings' }]) # Character.create(name: 'Luke', movie: movies.first) +Role.create!([ + { name: "new_user" }, + { name: "insights_user" }, + { name: "media_vault_user" }, + { name: "admin" }, +]) + easy_password = "password123" # Super-admin account; no applicant necessary. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 8dad5524..19977e53 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -21,7 +21,14 @@ Zenodotus allows its users to search its archive using image or text inputs. Sea ## User model -Zenodotus' `User` model handles authentication for the app via [Devise](https://github.com/heartcombo/devise). Roles are managed with the Rolify gem. Internal users have the `:admin` role and are recognized with the `is_admin?` helper (provided by Rolify automatically). +Zenodotus' `User` model handles authentication for the app via [Devise](https://github.com/heartcombo/devise). Roles are managed with the Rolify gem, which also generates role-specific helpers (e.g., `is_admin?`) at boot time. + +Roles: + +- `new_user`: Indicates a user was newly-created and has not yet gone through the setup process (clicked the link in their welcome email and chosen their own password). Applied to every new user created from an applicant, and removed when they have completed their own setup process. +- `insights_user`: Indicates a user has access to Fact-Check Insights. Applied to every new user created from an applicant. +- `media_vault_user`: Indicates a user has access to MediaVault. Currently only applied manually. +- `admin`: Indicates a user is authorized to administrate the site. (I.e., an internal user.) ## MediaReview Coming soon