From fa50dacd1e8410db62af14ddcd44e2b7e4d0f456 Mon Sep 17 00:00:00 2001 From: Brock Wilcox Date: Sun, 1 Nov 2026 11:57:45 -0500 Subject: [PATCH] Scope organization, partner user, and attachment lookups to the current organization Bugfixes: - current_organization only uses the organization_id param for super admins; other roles without an organization get nil. - invite_user invites to current_organization instead of a form param (hidden field renamed to organization_id so super admins can still choose the organization). - resend_user_invitation only finds users of the current organization. - PartnerUsersController finds the partner within the current organization, and users within that partner, for every action (including reset_password). - AttachmentsController only deletes documents of the current organization's partners. Specs that relied on the old lookups now set up records in the signed-in user's organization. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UCzWYsxjKeDD9EVVp6L2eS --- app/controllers/application_controller.rb | 4 +- app/controllers/attachments_controller.rb | 9 ++++- app/controllers/organizations_controller.rb | 4 +- app/controllers/partner_users_controller.rb | 10 ++--- app/views/users/_add_user_modal.erb | 2 +- .../application_controller_spec.rb | 14 +++++++ spec/requests/attachments_requests_spec.rb | 22 +++++++++- spec/requests/dashboard_requests_spec.rb | 11 +++++ spec/requests/organization_requests_spec.rb | 30 ++++++++++++++ spec/requests/partner_users_requests_spec.rb | 40 ++++++++++++++++++- spec/requests/users_requests_spec.rb | 6 +-- 11 files changed, 136 insertions(+), 16 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 62cd29a199..221f14afe0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -29,7 +29,9 @@ def current_organization nil elsif current_role.resource.is_a?(Organization) current_role.resource - else + elsif current_role.name.to_sym == Role::SUPER_ADMIN + # Super admins have no organization on their role, so they may choose + # which organization to act on. No other role may do this. Organization.find_by(id: params[:organization_id]) end end diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index e156714706..56ef502faf 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -1,6 +1,13 @@ class AttachmentsController < ApplicationController + # Only partner documents are deleted through this controller, and only for + # partners belonging to the current organization. def destroy - ActiveStorage::Attachment.find(params[:id])&.purge + ActiveStorage::Attachment.find_by!( + id: params[:id], + name: "documents", + record_type: "Partner", + record_id: current_organization.partners.select(:id) + ).purge redirect_back_or_to(partners_path) end diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index 8bc30a4b3d..3b8dd689e0 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -30,14 +30,14 @@ def invite_user UserInviteService.invite(email: params[:email], name: params[:name], roles: [Role::ORG_USER], - resource: Organization.find(params[:org])) + resource: current_organization) redirect_to organization_path, notice: "User invited to organization!" rescue => e redirect_to organization_path, alert: e.message end def resend_user_invitation - user = User.find(params[:user_id]) + user = current_organization.users.find(params[:user_id]) user.invite! redirect_to organization_path, notice: "User re-invited to organization!" end diff --git a/app/controllers/partner_users_controller.rb b/app/controllers/partner_users_controller.rb index c0df52cfe5..c560f5842e 100644 --- a/app/controllers/partner_users_controller.rb +++ b/app/controllers/partner_users_controller.rb @@ -2,7 +2,7 @@ class PartnerUsersController < ApplicationController before_action :authorize_admin - before_action :set_partner, only: %i[index create destroy resend_invitation] + before_action :set_partner def index @users = @partner.users @@ -29,7 +29,7 @@ def create end def destroy - user = User.find(params[:id]) + user = @partner.users.find(params[:id]) if user.remove_role(Role::PARTNER, @partner) redirect_back_or_to("/", notice: "Access to #{@partner.name} has been revoked for #{user.display_name}.") @@ -39,7 +39,7 @@ def destroy end def resend_invitation - user = User.find(params[:id]) + user = @partner.users.find(params[:id]) if user.invitation_accepted_at.nil? user.invite! @@ -55,7 +55,7 @@ def resend_invitation end def reset_password - user = User.find(params[:id]) + user = @partner.users.find(params[:id]) user.send_reset_password_instructions redirect_back_or_to(root_path, notice: "Password e-mail sent!") @@ -64,7 +64,7 @@ def reset_password private def set_partner - @partner = Partner.find(params[:partner_id]) + @partner = current_organization.partners.find(params[:partner_id]) end def user_params diff --git a/app/views/users/_add_user_modal.erb b/app/views/users/_add_user_modal.erb index b6f787e544..a40c07b913 100644 --- a/app/views/users/_add_user_modal.erb +++ b/app/views/users/_add_user_modal.erb @@ -17,7 +17,7 @@ <%= fa_icon "envelope" %> - <%= hidden_field_tag :org , current_organization.id %>
+ <%= hidden_field_tag :organization_id, current_organization.id %>
<%= submit_button({ text: "Invite User", icon: "envelope" }) %> <% end # form %> diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 2ecdc3c995..bf0f5527de 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -12,6 +12,20 @@ it "should return nil" do expect(controller.current_organization).to eq(nil) end + + it "should return nil even when an organization_id param is given" do + controller.params[:organization_id] = organization.id + expect(controller.current_organization).to eq(nil) + end + end + + context "As a super admin" do + let(:user) { create(:super_admin, organization: nil) } + + it "should return the organization from the organization_id param" do + controller.params[:organization_id] = organization.id + expect(controller.current_organization).to eq(organization) + end end context "As an org user" do diff --git a/spec/requests/attachments_requests_spec.rb b/spec/requests/attachments_requests_spec.rb index bc4dd8b75b..69a0da9fd2 100644 --- a/spec/requests/attachments_requests_spec.rb +++ b/spec/requests/attachments_requests_spec.rb @@ -1,14 +1,32 @@ RSpec.describe "Attachments", type: :request do + let(:organization) { create(:organization) } + before do - sign_in(create(:user)) + sign_in(create(:user, organization: organization)) end describe "DELETE #destroy" do - let(:partner) { create(:partner, documents: [Rack::Test::UploadedFile.new(Rails.root.join("spec/fixtures/files/dbase.pdf"), "application/pdf")]) } + let(:partner) { create(:partner, organization: organization, documents: [Rack::Test::UploadedFile.new(Rails.root.join("spec/fixtures/files/dbase.pdf"), "application/pdf")]) } it "redirects to referrer" do delete attachment_path(partner.documents.first) expect(response).to redirect_to(partners_path) end + + it "does not delete another organization's partner documents" do + other_partner = create(:partner, organization: create(:organization), documents: [Rack::Test::UploadedFile.new(Rails.root.join("spec/fixtures/files/dbase.pdf"), "application/pdf")]) + attachment = other_partner.documents.first + delete attachment_path(attachment) + expect(response).to have_http_status(:not_found) + expect(ActiveStorage::Attachment.exists?(attachment.id)).to be(true) + end + + it "does not delete attachments that are not partner documents" do + organization.logo.attach(io: Rails.root.join("spec/fixtures/files/logo.jpg").open, filename: "logo.jpg") + attachment = organization.logo.attachment + delete attachment_path(attachment) + expect(response).to have_http_status(:not_found) + expect(ActiveStorage::Attachment.exists?(attachment.id)).to be(true) + end end end diff --git a/spec/requests/dashboard_requests_spec.rb b/spec/requests/dashboard_requests_spec.rb index c568fc200f..e6f0eb58ee 100644 --- a/spec/requests/dashboard_requests_spec.rb +++ b/spec/requests/dashboard_requests_spec.rb @@ -59,6 +59,17 @@ end end + context "While signed in as a partner user" do + before do + sign_in(create(:partner_user, partner: create(:partner, organization: organization))) + end + + it "does not allow choosing an organization via organization_id" do + get dashboard_path(organization_id: create(:organization).id) + expect(response).to redirect_to(partners_dashboard_path) + end + end + context "While not signed in" do it "redirects for authentication" do get dashboard_path diff --git a/spec/requests/organization_requests_spec.rb b/spec/requests/organization_requests_spec.rb index 0ce747b79e..2f18cdf6b6 100644 --- a/spec/requests/organization_requests_spec.rb +++ b/spec/requests/organization_requests_spec.rb @@ -490,12 +490,34 @@ end end + describe "POST #invite_user" do + it "invites the user to the admin's organization" do + post invite_user_organization_path(email: "new_user@example.com", name: "New User") + expect(User.find_by(email: "new_user@example.com").has_role?(Role::ORG_USER, organization)).to be(true) + end + end + context "when attempting to access a different organization" do let(:other_organization) { create(:organization) } let(:other_organization_params) do { organization_id: other_organization.id } end + describe "POST #invite_user" do + it "does not invite to the other organization" do + post invite_user_organization_path(org: other_organization.id, email: organization_admin.email) + expect(organization_admin.reload.has_role?(Role::ORG_USER, other_organization)).to be(false) + end + end + + describe "POST #resend_user_invitation" do + it "does not re-invite the other organization's user" do + other_user = create(:user, organization: other_organization) + post resend_user_invitation_organization_path(user_id: other_user.id) + expect(response).to have_http_status(:not_found) + end + end + describe "GET #show" do before { get organization_path(other_organization_params) } @@ -532,6 +554,14 @@ sign_in(super_admin_org_admin) end + describe "POST #invite_user" do + it "invites the user to the organization given by organization_id" do + other_organization = create(:organization) + post invite_user_organization_path(organization_id: other_organization.id, email: "new_user@example.com") + expect(User.find_by(email: "new_user@example.com").has_role?(Role::ORG_USER, other_organization)).to be(true) + end + end + describe "POST #promote_to_org_admin" do context "promoting a user" do include_examples "promote to admin check", :user, :super_admin diff --git a/spec/requests/partner_users_requests_spec.rb b/spec/requests/partner_users_requests_spec.rb index 8492284b71..a88fd4f470 100644 --- a/spec/requests/partner_users_requests_spec.rb +++ b/spec/requests/partner_users_requests_spec.rb @@ -1,8 +1,8 @@ # spec/requests/partner_users_controller_spec.rb RSpec.describe PartnerUsersController, type: :request do - let!(:partner) { create(:partner) } # Assuming you have a factory for creating partners let(:organization) { create(:organization) } + let!(:partner) { create(:partner, organization: organization) } let(:user) { create(:user, organization: organization) } let(:org_admin) { create(:organization_admin, organization: organization) } let(:default_params) do @@ -236,4 +236,42 @@ end end end + + context "with a partner from another organization" do + let(:other_partner) { create(:partner, organization: create(:organization)) } + let!(:other_partner_user) { create(:partner_user, partner: other_partner) } + + before do + sign_in(org_admin) + end + + it "does not list its users" do + get partner_users_path(partner_id: other_partner) + expect(response).to have_http_status(:not_found) + end + + it "does not invite users to it" do + post partner_users_path(partner_id: other_partner), params: {user: {email: org_admin.email, name: "Admin"}} + expect(response).to have_http_status(:not_found) + expect(org_admin.reload.has_role?(Role::PARTNER, other_partner)).to be(false) + end + + it "does not remove its users" do + delete partner_user_path(partner_id: other_partner, id: other_partner_user) + expect(response).to have_http_status(:not_found) + expect(other_partner_user.reload.has_role?(Role::PARTNER, other_partner)).to be(true) + end + + it "does not resend invitations to its users" do + post resend_invitation_partner_user_path(partner_id: other_partner, id: other_partner_user) + expect(response).to have_http_status(:not_found) + end + + it "does not reset its users' passwords via one of your own partners" do + expect { + post reset_password_partner_user_path(partner_id: partner, id: other_partner_user) + }.not_to change { ActionMailer::Base.deliveries.count } + expect(response).to have_http_status(:not_found) + end + end end diff --git a/spec/requests/users_requests_spec.rb b/spec/requests/users_requests_spec.rb index e083112f14..12cb3d53a8 100644 --- a/spec/requests/users_requests_spec.rb +++ b/spec/requests/users_requests_spec.rb @@ -29,7 +29,7 @@ describe "POST #partner_user_reset_password" do let(:partner) { create(:partner, organization: organization) } - let!(:user) { create(:partner_user, partner: partner, email: "me@partner.com") } + let!(:partner_user) { create(:partner_user, partner: partner, email: "me@partner.com") } let(:params) { { organization_id: organization.id, partner_id: partner.id, email: "me@partner.com" } } it "should send a password" do @@ -46,13 +46,13 @@ end it "should return an error if it cannot find the user" do - user.update!(email: "some-other-email@mail.com") + partner_user.update!(email: "some-other-email@mail.com") post partner_user_reset_password_users_path(params) expect(ActionMailer::Base.deliveries.size).to eq(0) end it "should return send a password even if case-insensitive spelling of email" do - user.update!(email: "Me@partner.com") + partner_user.update!(email: "Me@partner.com") post partner_user_reset_password_users_path(params) expect(ActionMailer::Base.deliveries.size).to eq(1) end