From e817a2ae7cc4d99a67a0eee9a8c47864f8ce8114 Mon Sep 17 00:00:00 2001 From: elasticspoon Date: Sun, 23 Jun 2024 17:25:00 -0400 Subject: [PATCH 1/3] feat: replace org_user role deactivate with removal The issue was that banks could "deactivate" a user (which is essentially a soft delete of the whole User record). However, if that user then went to a Partner they would not be able to invite that user because that user would exist (new User cannot be created) but be soft deleted (existing User is treated like they didn't have permissions). We changed the logic around deactivation to address the role not the user. I attempted to implement logic around deactivating roles themselves in #4392, #4382. But the logic became very complex. We decided (#3587 (comment)) to go for a more architerurally simple solution of removing the deactivation functionality and just deleting the roles. To migrate the existing data I decided to reactivate any discarded user with the org user role and remove the org user role from them. (My guess is that those are the org users that actually got deactivated) --- app/controllers/organizations_controller.rb | 21 ++++---- app/controllers/partners/users_controller.rb | 1 + app/views/users/_organization_user.html.erb | 36 ++++++------- config/initializers/devise.rb | 7 +++ config/routes.rb | 3 +- ...ctivate_users_and_remove_org_user_roles.rb | 15 ++++++ db/schema.rb | 2 +- spec/factories/users.rb | 4 ++ spec/factories/users_roles.rb | 14 ++++++ spec/requests/organization_requests_spec.rb | 42 +++++++++------- spec/requests/partners/user_requests_spec.rb | 50 +++++++++++++++++++ spec/system/log_in_system_spec.rb | 16 +++++- spec/system/organization_system_spec.rb | 24 +++------ 13 files changed, 162 insertions(+), 73 deletions(-) create mode 100644 db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb create mode 100644 spec/factories/users_roles.rb diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index 4ab62d3ac4..2a0fbf99d3 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -65,18 +65,17 @@ def demote_to_user end end - def deactivate_user - user = User.with_discarded.find_by!(id: params[:user_id]) - raise ActiveRecord::RecordNotFound unless user.has_role?(Role::ORG_USER, current_organization) - user.discard! - redirect_to user_update_redirect_path, notice: "User has been deactivated." - end - - def reactivate_user - user = User.with_discarded.find_by!(id: params[:user_id]) + def remove_user + user = User.find(params[:user_id]) raise ActiveRecord::RecordNotFound unless user.has_role?(Role::ORG_USER, current_organization) - user.undiscard! - redirect_to user_update_redirect_path, notice: "User has been reactivated." + begin + RemoveRoleService.call(user_id: params[:user_id], + resource_type: Role::ORG_USER, + resource_id: current_organization.id) + redirect_to user_update_redirect_path, notice: "User has been removed!" + rescue => e + redirect_back(fallback_location: organization_path, alert: e.message) + end end private diff --git a/app/controllers/partners/users_controller.rb b/app/controllers/partners/users_controller.rb index bb4a4eb5bf..57693186b6 100644 --- a/app/controllers/partners/users_controller.rb +++ b/app/controllers/partners/users_controller.rb @@ -23,6 +23,7 @@ def update end end + # partner user creation def create user = UserInviteService.invite(name: user_params[:name], email: user_params[:email], diff --git a/app/views/users/_organization_user.html.erb b/app/views/users/_organization_user.html.erb index c4711e50cd..9e882b4e4b 100644 --- a/app/views/users/_organization_user.html.erb +++ b/app/views/users/_organization_user.html.erb @@ -1,9 +1,6 @@ <%= user.display_name %> - <% if user.discarded? %> - Deactivated - <% end %> <%= user.email %> <%= user.kind %> @@ -18,30 +15,25 @@ <% end %> <% if current_user.has_role?(Role::ORG_ADMIN, current_organization) && user.has_role?(Role::ORG_ADMIN, current_organization) %> <%= edit_button_to demote_to_user_organization_path(user_id: user.id), - {text: 'Make User'}, + {text: 'Demote to User'}, {method: :post, rel: "nofollow", data: {confirm: 'This will demote the admin to user status. Are you sure that you want to submit this?', size: 'xs'}} unless user.id == current_user.id %> <% end %> diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index b17625ca9c..3951cd3f08 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -326,3 +326,10 @@ # so you need to do it manually. For the users scope, it would be: # config.omniauth_path_prefix = '/my_engine/users/auth' end + +Warden::Manager.after_set_user do |user, auth, opts| + if user.roles.empty? + auth.logout + throw(:warden) + end +end diff --git a/config/routes.rb b/config/routes.rb index b44654f925..81fafcf144 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -89,8 +89,7 @@ def set_up_flipper resource :organization, path: :manage, only: %i(edit update) do collection do post :invite_user - put :deactivate_user - put :reactivate_user + post :remove_user post :resend_user_invitation post :promote_to_org_admin post :demote_to_user diff --git a/db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb b/db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb new file mode 100644 index 0000000000..50cfa5265c --- /dev/null +++ b/db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb @@ -0,0 +1,15 @@ +class ReactivateUsersAndRemoveOrgUserRoles < ActiveRecord::Migration[7.1] + def up + users = User.unscoped.joins(:roles).where.not(discarded_at: nil).where("roles.name": "org_user") + users.each do |user| + user.transaction do + user.update!(discarded_at: nil) + user.roles.where(name: "org_user").delete_all + end + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index 3ac916c5e4..bcbb2930f3 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.1].define(version: 2024_06_01_155348) do +ActiveRecord::Schema[7.1].define(version: 2024_06_24_185108) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 3f2c869181..5da60b25ce 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -77,6 +77,10 @@ end end + trait :no_roles do + organization { nil } + end + trait :deactivated do discarded_at { Time.zone.now } end diff --git a/spec/factories/users_roles.rb b/spec/factories/users_roles.rb new file mode 100644 index 0000000000..4a507b56c2 --- /dev/null +++ b/spec/factories/users_roles.rb @@ -0,0 +1,14 @@ +# == Schema Information +# +# Table name: users_roles +# +# id :bigint not null, primary key +# role_id :bigint +# user_id :bigint +# +FactoryBot.define do + factory :users_role do + user + role + end +end diff --git a/spec/requests/organization_requests_spec.rb b/spec/requests/organization_requests_spec.rb index 60df080987..851e2609e2 100644 --- a/spec/requests/organization_requests_spec.rb +++ b/spec/requests/organization_requests_spec.rb @@ -109,28 +109,34 @@ end end - describe "PUT #deactivate_user" do - subject { put deactivate_user_organization_path(user_id: user.id) } + describe "POST #remove_user" do + subject { post remove_user_organization_path(user_id: user.id) } - it "redirect after update" do - subject - expect(response).to redirect_to(organization_path) - end - it "deactivates the user" do - expect { subject }.to change { user.reload.discarded_at }.to be_present - end - end + context "when user is org user" do + it "redirect after update" do + subject + expect(response).to redirect_to(organization_path) + end - describe "PUT #reactivate_user" do - subject { put reactivate_user_organization_path(user_id: user.id) } - before { user.discard! } + it "removes the org user role" do + role = user.roles.find_by(resource: organization, name: Role::ORG_USER) + user_role = UsersRole.find_by(user: user, role: role) + expect(user_role).to_not be_nil - it "redirect after update" do - subject - expect(response).to redirect_to(organization_path) + subject + + expect(UsersRole.find_by(user: user, role: role)).to be_nil + end end - it "reactivates the user" do - expect { subject }.to change { user.reload.discarded_at }.to be_nil + + context "when user is not an org user" do + let(:user) { create(:user, organization: create(:organization)) } + + it 'raises an error' do + subject + + expect(response).to be_not_found + end end end diff --git a/spec/requests/partners/user_requests_spec.rb b/spec/requests/partners/user_requests_spec.rb index 60447e3821..34828a4517 100644 --- a/spec/requests/partners/user_requests_spec.rb +++ b/spec/requests/partners/user_requests_spec.rb @@ -7,6 +7,7 @@ before do sign_in(partner_user) end + describe "GET #edit" do it "successfully loads the page" do get edit_partners_user_path(partner_user) @@ -28,4 +29,53 @@ expect(partner_user.name).to eq "New name" end end + + describe "POST #create" do + let(:params) do + {user: {name: "New User", email: "new_partner_email@example.com"}} + end + + it "creates a new user" do + post partners_users_path, params: params + + aggregate_failures do + expect(response.request.flash[:success]).to eq "You have invited New User to join your organization!" + expect(response).to redirect_to(partners_users_path) + end + end + + context "when the user already exists" do + let(:user) { create(:user, name: "Existing User") } + + let(:params) do + {user: {name: user.name, email: user.email}} + end + + it "creates a new user" do + post partners_users_path, params: params + + aggregate_failures do + expect(response.request.flash[:success]).to eq "You have invited Existing User to join your organization!" + expect(response).to redirect_to(partners_users_path) + expect(user.roles.count).to eq(2) + end + end + + context "when org user had role removed" do + before do + post remove_user_organization_path(user_id: user.id) + end + + it "adds role to user" do + post partners_users_path, params: params + + aggregate_failures do + expect(response.request.flash[:success]).to eq "You have invited Existing User to join your organization!" + expect(response).to redirect_to(partners_users_path) + expect(user.roles.count).to eq(2) + end + end + end + end + end end diff --git a/spec/system/log_in_system_spec.rb b/spec/system/log_in_system_spec.rb index f15c5041be..5a666cd82d 100644 --- a/spec/system/log_in_system_spec.rb +++ b/spec/system/log_in_system_spec.rb @@ -10,9 +10,23 @@ end end + describe "User with no roles" do + before do + create(:user, :no_roles, email: "no_role_user@example.com") + end + + it "should not allow the user to log in" do + visit "/users/sign_in" + fill_in "user_email", with: "no_role_user@example.com" + fill_in "user_password", with: DEFAULT_USER_PASSWORD + find('input[name="commit"]').click + expect(page).to have_content("You need to sign in before continuing.") + end + end + describe "Deactivated user" do before do - create(:user, :deactivated, email: "deactivated@exmaple.com") + create(:user, :deactivated, email: "deactivated@example.com") end it "should not allow the user to log in" do diff --git a/spec/system/organization_system_spec.rb b/spec/system/organization_system_spec.rb index 6099954340..76b10ee645 100644 --- a/spec/system/organization_system_spec.rb +++ b/spec/system/organization_system_spec.rb @@ -158,31 +158,19 @@ it "can see 'Make user' button for admins" do create(:organization_admin) visit organization_path - expect(page.find(".table.border")).to have_content "Make User" + expect(page.find(".table.border")).to have_content "Demote to User" end - it "can deactivate a user in the organization" do - user = create(:user, name: "User to be deactivated") + it "can remove a user from the organization" do + user = create(:user, name: "User to be deactivated", organization: organization) visit organization_path accept_confirm do click_button dom_id(user, "dropdownMenu") - click_link dom_id(user) + click_link "Remove User" end - expect(page).to have_content("User has been deactivated") - expect(user.reload.discarded_at).to be_present - end - - it "can re-activate a user in the organization" do - user = create(:user, :deactivated) - visit organization_path - accept_confirm do - click_button dom_id(user, "dropdownMenu") - click_link dom_id(user) - end - - expect(page).to have_content("User has been reactivated") - expect(user.reload.discarded_at).to be_nil + expect(page).to have_content("User has been removed!") + expect(user.has_role?(Role::ORG_USER)).to be false end end end From d77b19c9b2d569732dff636aeeb2df16154e38c9 Mon Sep 17 00:00:00 2001 From: elasticspoon Date: Wed, 26 Jun 2024 16:29:20 -0400 Subject: [PATCH 2/3] fix: review comments also simplification/correction of some test logic --- spec/requests/organization_requests_spec.rb | 10 ++-------- spec/requests/partners/user_requests_spec.rb | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/spec/requests/organization_requests_spec.rb b/spec/requests/organization_requests_spec.rb index 851e2609e2..17e2aff777 100644 --- a/spec/requests/organization_requests_spec.rb +++ b/spec/requests/organization_requests_spec.rb @@ -113,19 +113,13 @@ subject { post remove_user_organization_path(user_id: user.id) } context "when user is org user" do - it "redirect after update" do + it "redirects after update" do subject expect(response).to redirect_to(organization_path) end it "removes the org user role" do - role = user.roles.find_by(resource: organization, name: Role::ORG_USER) - user_role = UsersRole.find_by(user: user, role: role) - expect(user_role).to_not be_nil - - subject - - expect(UsersRole.find_by(user: user, role: role)).to be_nil + expect { subject }.to change { user.has_role?(Role::ORG_USER, organization) }.from(true).to(false) end end diff --git a/spec/requests/partners/user_requests_spec.rb b/spec/requests/partners/user_requests_spec.rb index 34828a4517..1e752733e5 100644 --- a/spec/requests/partners/user_requests_spec.rb +++ b/spec/requests/partners/user_requests_spec.rb @@ -45,34 +45,40 @@ end context "when the user already exists" do - let(:user) { create(:user, name: "Existing User") } + let(:organization) { create(:organization) } + let(:org_user) { create(:user, name: "Existing User", organization: organization) } let(:params) do - {user: {name: user.name, email: user.email}} + {user: {name: org_user.name, email: org_user.email}} end - it "creates a new user" do + it "succeeds and adds additional role to user" do post partners_users_path, params: params aggregate_failures do expect(response.request.flash[:success]).to eq "You have invited Existing User to join your organization!" expect(response).to redirect_to(partners_users_path) - expect(user.roles.count).to eq(2) + expect(org_user.has_role?(Role::PARTNER, partner)).to be true + expect(org_user.has_role?(Role::ORG_USER, organization)).to be true end end context "when org user had role removed" do before do - post remove_user_organization_path(user_id: user.id) + sign_in create(:organization_admin, organization: organization) + post remove_user_organization_path(user_id: org_user.id) + sign_in(partner_user) end it "adds role to user" do + expect(org_user.has_role?(Role::ORG_USER, organization)).to be false + post partners_users_path, params: params aggregate_failures do expect(response.request.flash[:success]).to eq "You have invited Existing User to join your organization!" expect(response).to redirect_to(partners_users_path) - expect(user.roles.count).to eq(2) + expect(org_user.has_role?(Role::PARTNER, partner)).to be true end end end From 63f2a9cbad9c74dfecaba6136da9cd091c06265a Mon Sep 17 00:00:00 2001 From: elasticspoon Date: Wed, 26 Jun 2024 16:58:40 -0400 Subject: [PATCH 3/3] fix: simplify migration All discarded users are org_user meaning they have been deactivated. To simplify everything we will just remove all thier roles. --- ...240624185108_reactivate_users_and_remove_org_user_roles.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb b/db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb index 50cfa5265c..cea79ade07 100644 --- a/db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb +++ b/db/migrate/20240624185108_reactivate_users_and_remove_org_user_roles.rb @@ -1,10 +1,10 @@ class ReactivateUsersAndRemoveOrgUserRoles < ActiveRecord::Migration[7.1] def up - users = User.unscoped.joins(:roles).where.not(discarded_at: nil).where("roles.name": "org_user") + users = User.unscoped.where.not(discarded_at: nil) users.each do |user| user.transaction do user.update!(discarded_at: nil) - user.roles.where(name: "org_user").delete_all + user.roles.delete_all end end end