From 5d1422d37b3a32484ee957fe7d0e1cdcfe28fe62 Mon Sep 17 00:00:00 2001 From: Wabo Date: Sun, 17 May 2026 05:37:10 -0400 Subject: [PATCH] Enable Editor and Viewer user roles Splits the previously single-role Ability class along role boundaries. Admin keeps full account management. Editor gets CRUD on templates, folders, submissions, submitters, and template sharings, but cannot touch users, account settings, encrypted configs, webhooks, or MCP. Viewer gets read-only access to the same content surface. Every role keeps self-service on their own User / UserConfig / AccessToken. UsersController#index gains a one-line admin guard so non-admins cannot reach the user list via the self-manage rule's class-level CanCan check. Co-Authored-By: Claude Opus 4.7 --- app/controllers/users_controller.rb | 2 + app/models/user.rb | 8 +- app/views/users/_role_select.html.erb | 6 +- lib/ability.rb | 72 ++++++++-- spec/models/ability_spec.rb | 162 +++++++++++++++++++++++ spec/requests/role_authorization_spec.rb | 101 ++++++++++++++ 6 files changed, 333 insertions(+), 18 deletions(-) create mode 100644 spec/models/ability_spec.rb create mode 100644 spec/requests/role_authorization_spec.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 69b20db5..3e02de85 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -6,6 +6,8 @@ class UsersController < ApplicationController before_action :build_user, only: %i[new create] authorize_resource :user, only: %i[new create] + before_action(only: :index) { authorize!(:manage, current_account) } + def index @users = if params[:status] == 'archived' diff --git a/app/models/user.rb b/app/models/user.rb index 31039bc3..01c944eb 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -48,7 +48,9 @@ # class User < ApplicationRecord ROLES = [ - ADMIN_ROLE = 'admin' + ADMIN_ROLE = 'admin', + EDITOR_ROLE = 'editor', + VIEWER_ROLE = 'viewer' ].freeze EMAIL_REGEXP = /[^@;,<>\s]+@[^@;,<>\s]+/ @@ -129,6 +131,10 @@ class User < ApplicationRecord end end + def admin? = role == ADMIN_ROLE + def editor? = role == EDITOR_ROLE + def viewer? = role == VIEWER_ROLE + def signed_in_via_sso? provider == 'google_oauth2' && uid.present? end diff --git a/app/views/users/_role_select.html.erb b/app/views/users/_role_select.html.erb index c10a57ad..7a4b62fa 100644 --- a/app/views/users/_role_select.html.erb +++ b/app/views/users/_role_select.html.erb @@ -2,12 +2,12 @@ <%= f.label :role, class: 'label' %> <%= f.select :role, nil, {}, class: 'base-select' do %> - - + + <% end %> diff --git a/lib/ability.rb b/lib/ability.rb index da472f58..b9b2639b 100644 --- a/lib/ability.rb +++ b/lib/ability.rb @@ -4,25 +4,69 @@ class Ability include CanCan::Ability def initialize(user) + return if user.blank? + + grant_personal_abilities(user) + + case user.role + when User::ADMIN_ROLE then grant_admin_abilities(user) + when User::EDITOR_ROLE then grant_editor_abilities(user) + when User::VIEWER_ROLE then grant_viewer_abilities(user) + end + end + + private + + def grant_personal_abilities(user) + can :read, Account, id: user.account_id + can :manage, User, id: user.id + can :manage, UserConfig, user_id: user.id + can :manage, EncryptedUserConfig, user_id: user.id + can :manage, AccessToken, user_id: user.id + end + + def grant_admin_abilities(user) can %i[read create update], Template, Abilities::TemplateConditions.collection(user) do |template| Abilities::TemplateConditions.entity(template, user:, ability: 'manage') end - can :destroy, Template, account_id: user.account_id - can :manage, TemplateFolder, account_id: user.account_id - can :manage, TemplateSharing, template: { account_id: user.account_id } - can :manage, Submission, account_id: user.account_id - can :manage, Submitter, account_id: user.account_id - can :manage, User, account_id: user.account_id - can :manage, EncryptedConfig, account_id: user.account_id - can :manage, EncryptedUserConfig, user_id: user.id - can :manage, AccountConfig, account_id: user.account_id - can :manage, UserConfig, user_id: user.id - can :manage, Account, id: user.account_id - can :manage, AccessToken, user_id: user.id - can :manage, McpToken, user_id: user.id - can :manage, WebhookUrl, account_id: user.account_id + can :destroy, Template, account_id: user.account_id + can :manage, TemplateFolder, account_id: user.account_id + can :manage, TemplateSharing, template: { account_id: user.account_id } + can :manage, Submission, account_id: user.account_id + can :manage, Submitter, account_id: user.account_id + can :manage, User, account_id: user.account_id + can :manage, EncryptedConfig, account_id: user.account_id + can :manage, AccountConfig, account_id: user.account_id + can :manage, Account, id: user.account_id + can :manage, McpToken, user_id: user.id + can :manage, WebhookUrl, account_id: user.account_id can :manage, :mcp end + + def grant_editor_abilities(user) + can %i[read create update], Template, Abilities::TemplateConditions.collection(user) do |template| + Abilities::TemplateConditions.entity(template, user:, ability: 'manage') + end + + can :destroy, Template, account_id: user.account_id + can :manage, TemplateFolder, account_id: user.account_id + can :manage, TemplateSharing, template: { account_id: user.account_id } + can :manage, Submission, account_id: user.account_id + can :manage, Submitter, account_id: user.account_id + can :read, AccountConfig, account_id: user.account_id + end + + def grant_viewer_abilities(user) + can :read, Template, Abilities::TemplateConditions.collection(user, ability: :read) do |template| + Abilities::TemplateConditions.entity(template, user:, ability: 'read') + end + + can :read, TemplateFolder, account_id: user.account_id + can :read, TemplateSharing, template: { account_id: user.account_id } + can :read, Submission, account_id: user.account_id + can :read, Submitter, account_id: user.account_id + can :read, AccountConfig, account_id: user.account_id + end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb new file mode 100644 index 00000000..b673ec07 --- /dev/null +++ b/spec/models/ability_spec.rb @@ -0,0 +1,162 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'cancan/matchers' + +RSpec.describe Ability do + let(:account) { create(:account) } + let(:other_account) { create(:account) } + + def template_for(account) + Template.new(account_id: account.id) + end + + def template_folder_for(account) + TemplateFolder.new(account_id: account.id) + end + + def submission_for(account) + Submission.new(account_id: account.id) + end + + def submitter_for(account) + Submitter.new(account_id: account.id) + end + + shared_examples 'personal-resource grants' do + it 'manages own User, UserConfig, EncryptedUserConfig, AccessToken' do + expect(ability).to be_able_to(:manage, user) + expect(ability).to be_able_to(:manage, UserConfig.new(user_id: user.id)) + expect(ability).to be_able_to(:manage, EncryptedUserConfig.new(user_id: user.id)) + expect(ability).to be_able_to(:manage, AccessToken.new(user_id: user.id)) + expect(ability).to be_able_to(:read, Account.new.tap { |a| a.id = account.id }) + end + + it "cannot touch another user's User / UserConfig / AccessToken (unless admin)" do + other_user = create(:user, account: account) + expect(ability).not_to be_able_to(:manage, UserConfig.new(user_id: other_user.id)) + expect(ability).not_to be_able_to(:manage, AccessToken.new(user_id: other_user.id)) + + # Admin's full :manage User rule covers same-account users; editor/viewer don't. + if user.role == User::ADMIN_ROLE + expect(ability).to be_able_to(:manage, other_user) + else + expect(ability).not_to be_able_to(:manage, other_user) + end + end + end + + describe 'admin role' do + let(:user) { create(:user, account: account, role: User::ADMIN_ROLE) } + let(:ability) { described_class.new(user) } + + include_examples 'personal-resource grants' + + it 'manages templates, folders, sharings, submissions, submitters in own account' do + expect(ability).to be_able_to(:read, template_for(account)) + expect(ability).to be_able_to(:create, template_for(account)) + expect(ability).to be_able_to(:update, template_for(account)) + expect(ability).to be_able_to(:destroy, template_for(account)) + expect(ability).to be_able_to(:manage, template_folder_for(account)) + expect(ability).to be_able_to(:manage, submission_for(account)) + expect(ability).to be_able_to(:manage, submitter_for(account)) + end + + it 'manages account-wide settings and users' do + expect(ability).to be_able_to(:manage, User.new(account_id: account.id)) + expect(ability).to be_able_to(:manage, Account.new.tap { |a| a.id = account.id }) + expect(ability).to be_able_to(:manage, AccountConfig.new(account_id: account.id)) + expect(ability).to be_able_to(:manage, EncryptedConfig.new(account_id: account.id)) + expect(ability).to be_able_to(:manage, WebhookUrl.new(account_id: account.id)) + expect(ability).to be_able_to(:manage, McpToken.new(user_id: user.id)) + expect(ability).to be_able_to(:manage, :mcp) + end + + it 'cannot touch resources scoped to another account' do + expect(ability).not_to be_able_to(:read, template_for(other_account)) + expect(ability).not_to be_able_to(:destroy, template_for(other_account)) + expect(ability).not_to be_able_to(:manage, submission_for(other_account)) + expect(ability).not_to be_able_to(:manage, User.new(account_id: other_account.id)) + expect(ability).not_to be_able_to(:manage, Account.new.tap { |a| a.id = other_account.id }) + end + end + + describe 'editor role' do + let(:user) { create(:user, account: account, role: User::EDITOR_ROLE) } + let(:ability) { described_class.new(user) } + + include_examples 'personal-resource grants' + + it 'manages templates, folders, sharings, submissions, submitters in own account' do + expect(ability).to be_able_to(:read, template_for(account)) + expect(ability).to be_able_to(:create, template_for(account)) + expect(ability).to be_able_to(:update, template_for(account)) + expect(ability).to be_able_to(:destroy, template_for(account)) + expect(ability).to be_able_to(:manage, template_folder_for(account)) + expect(ability).to be_able_to(:manage, submission_for(account)) + expect(ability).to be_able_to(:manage, submitter_for(account)) + end + + it 'can read AccountConfig but cannot manage account-wide settings' do + expect(ability).to be_able_to(:read, AccountConfig.new(account_id: account.id)) + expect(ability).not_to be_able_to(:manage, AccountConfig.new(account_id: account.id)) + expect(ability).not_to be_able_to(:manage, EncryptedConfig.new(account_id: account.id)) + expect(ability).not_to be_able_to(:manage, User.new(account_id: account.id)) + expect(ability).not_to be_able_to(:manage, Account.new.tap { |a| a.id = account.id }) + expect(ability).not_to be_able_to(:manage, WebhookUrl.new(account_id: account.id)) + expect(ability).not_to be_able_to(:manage, McpToken.new(user_id: user.id)) + expect(ability).not_to be_able_to(:manage, :mcp) + end + + it 'cannot touch resources scoped to another account' do + expect(ability).not_to be_able_to(:read, template_for(other_account)) + expect(ability).not_to be_able_to(:manage, submission_for(other_account)) + end + end + + describe 'viewer role' do + let(:user) { create(:user, account: account, role: User::VIEWER_ROLE) } + let(:ability) { described_class.new(user) } + + include_examples 'personal-resource grants' + + it 'reads templates, folders, sharings, submissions, submitters in own account' do + expect(ability).to be_able_to(:read, template_for(account)) + expect(ability).to be_able_to(:read, template_folder_for(account)) + expect(ability).to be_able_to(:read, submission_for(account)) + expect(ability).to be_able_to(:read, submitter_for(account)) + expect(ability).to be_able_to(:read, AccountConfig.new(account_id: account.id)) + end + + it 'cannot mutate anything in own account beyond personal resources' do + expect(ability).not_to be_able_to(:create, template_for(account)) + expect(ability).not_to be_able_to(:update, template_for(account)) + expect(ability).not_to be_able_to(:destroy, template_for(account)) + expect(ability).not_to be_able_to(:manage, template_folder_for(account)) + expect(ability).not_to be_able_to(:manage, submission_for(account)) + expect(ability).not_to be_able_to(:manage, submitter_for(account)) + expect(ability).not_to be_able_to(:manage, User.new(account_id: account.id)) + expect(ability).not_to be_able_to(:manage, AccountConfig.new(account_id: account.id)) + expect(ability).not_to be_able_to(:manage, EncryptedConfig.new(account_id: account.id)) + expect(ability).not_to be_able_to(:manage, WebhookUrl.new(account_id: account.id)) + expect(ability).not_to be_able_to(:manage, Account.new.tap { |a| a.id = account.id }) + expect(ability).not_to be_able_to(:manage, :mcp) + end + + it 'cannot read resources scoped to another account' do + expect(ability).not_to be_able_to(:read, template_for(other_account)) + expect(ability).not_to be_able_to(:read, submission_for(other_account)) + end + end + + describe 'unknown role' do + let(:user) { create(:user, account: account, role: User::ADMIN_ROLE).tap { |u| u.update_column(:role, 'mystery') } } + let(:ability) { described_class.new(user) } + + it 'still grants personal resources but no role-specific abilities' do + expect(ability).to be_able_to(:manage, UserConfig.new(user_id: user.id)) + expect(ability).not_to be_able_to(:read, template_for(account)) + expect(ability).not_to be_able_to(:manage, submission_for(account)) + end + end +end diff --git a/spec/requests/role_authorization_spec.rb b/spec/requests/role_authorization_spec.rb new file mode 100644 index 00000000..50b52523 --- /dev/null +++ b/spec/requests/role_authorization_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Role-based authorization', type: :request do + let!(:account) { create(:account) } + let!(:admin) { create(:user, account: account, role: User::ADMIN_ROLE, email: 'admin@wabo.cc') } + let!(:editor) { create(:user, account: account, role: User::EDITOR_ROLE, email: 'editor@wabo.cc') } + let!(:viewer) { create(:user, account: account, role: User::VIEWER_ROLE, email: 'viewer@wabo.cc') } + + # `ApplicationController` rescues `CanCan::AccessDenied` only in production/test + # and redirects to `root_path` (see app/controllers/application_controller.rb). + def expect_denied + expect(response).to have_http_status(:found) + expect(response).to redirect_to(root_path) + end + + shared_examples 'an admin-only settings route' do |path_helper| + it "is denied for editors (#{path_helper})" do + sign_in editor + get send(path_helper) + expect_denied + end + + it "is denied for viewers (#{path_helper})" do + sign_in viewer + get send(path_helper) + expect_denied + end + + it "is reachable for admins (#{path_helper})" do + sign_in admin + get send(path_helper) + expect(response).to have_http_status(:ok).or have_http_status(:found) + expect(response).not_to redirect_to(root_path) if response.status == 302 + end + end + + describe 'admin-only settings' do + include_examples 'an admin-only settings route', :settings_users_path + include_examples 'an admin-only settings route', :settings_sso_index_path + include_examples 'an admin-only settings route', :settings_webhooks_path + include_examples 'an admin-only settings route', :settings_esign_path + + # Personalization's GET reads `AccountConfig`, which Editor/Viewer can do + # (so UI chrome renders correctly). Writes are gated by :create AccountConfig, + # which only admins hold. + it 'lets editors view personalization but blocks the POST' do + sign_in editor + get settings_personalization_path + expect(response).to have_http_status(:ok) + + post settings_personalization_path, params: { + account_config: { key: AccountConfig::FORM_COMPLETED_BUTTON_KEY, value: { title: 'Done', url: '' } } + } + expect(response).to redirect_to(root_path) + end + + it 'lets viewers view personalization but blocks the POST' do + sign_in viewer + get settings_personalization_path + expect(response).to have_http_status(:ok) + + post settings_personalization_path, params: { + account_config: { key: AccountConfig::FORM_COMPLETED_BUTTON_KEY, value: { title: 'Done', url: '' } } + } + expect(response).to redirect_to(root_path) + end + end + + describe 'templates and submissions list pages' do + it 'are reachable for editors' do + sign_in editor + get templates_path + expect(response).to have_http_status(:ok) + get submissions_path + expect(response).to have_http_status(:ok) + end + + it 'are reachable for viewers' do + sign_in viewer + get templates_path + expect(response).to have_http_status(:ok) + get submissions_path + expect(response).to have_http_status(:ok) + end + end + + describe 'self-service profile' do + it 'is reachable for editors and viewers' do + sign_in editor + get settings_profile_index_path + expect(response).to have_http_status(:ok) + + sign_out editor + sign_in viewer + get settings_profile_index_path + expect(response).to have_http_status(:ok) + end + end +end