From 3ddaed4831bf77b2cfa912ac1181da05a3743b40 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Wed, 17 Jul 2024 21:02:31 +1000 Subject: [PATCH 01/28] Add Access Level field to Ownership table and update RubyGem policies to check for the level of access --- app/avo/resources/ownership_resource.rb | 3 ++ app/controllers/api/v1/owners_controller.rb | 15 ++++++ app/controllers/owners_controller.rb | 15 ++++++ app/models/api_key.rb | 4 +- app/models/rubygem.rb | 5 ++ app/policies/api/rubygem_policy.rb | 7 +++ app/policies/application_policy.rb | 4 +- app/policies/ownership_policy.rb | 4 ++ app/policies/rubygem_policy.rb | 26 ++++++---- config/initializers/requires.rb | 1 + config/locales/en.yml | 6 +++ config/routes.rb | 4 +- ...717081704_add_access_level_to_ownership.rb | 5 ++ db/schema.rb | 3 +- lib/access.rb | 30 +++++++++++ test/factories/ownership.rb | 5 ++ test/factories/rubygem.rb | 7 ++- .../api/v1/owners_controller_test.rb | 27 ++++++++++ test/functional/owners_controller_test.rb | 17 +++++++ test/lib/access_test.rb | 35 +++++++++++++ test/policies/ownership_policy_test.rb | 20 +++++++- test/policies/rubygem_policy_test.rb | 50 +++++++++++++++---- 22 files changed, 263 insertions(+), 30 deletions(-) create mode 100644 db/migrate/20240717081704_add_access_level_to_ownership.rb create mode 100644 lib/access.rb create mode 100644 test/lib/access_test.rb diff --git a/app/avo/resources/ownership_resource.rb b/app/avo/resources/ownership_resource.rb index cd51054bbd0..c9d24d89a09 100644 --- a/app/avo/resources/ownership_resource.rb +++ b/app/avo/resources/ownership_resource.rb @@ -26,4 +26,7 @@ class ConfirmedFilter < ScopeBooleanFilter; end field :authorizer, as: :belongs_to field :confirmed_at, as: :date_time + field :access_level, as: :text do |model, _resource, _view| + Access.label_for_role_flag(model.access_level) + end end diff --git a/app/controllers/api/v1/owners_controller.rb b/app/controllers/api/v1/owners_controller.rb index 57bec65a620..2b502a5447b 100644 --- a/app/controllers/api/v1/owners_controller.rb +++ b/app/controllers/api/v1/owners_controller.rb @@ -12,6 +12,17 @@ def show end end + def update + authorize @rubygem, :update_owner? + ownership = @rubygem.ownerships.find_by!(user: User.find_by_name!(email_param)) + if ownership.present? + ownership.update!(ownership_update_params) + render plain: response_with_mfa_warning("Owner updated successfully.") + else + render plain: response_with_mfa_warning("Unable to update owner."), status: :forbidden + end + end + def create authorize @rubygem, :add_owner? owner = User.find_by_name!(email_param) @@ -60,4 +71,8 @@ def render_not_found def email_param params.permit(:email).require(:email) end + + def ownership_update_params + params.permit(:access_level) + end end diff --git a/app/controllers/owners_controller.rb b/app/controllers/owners_controller.rb index 6134249f78a..31a8ab68ff0 100644 --- a/app/controllers/owners_controller.rb +++ b/app/controllers/owners_controller.rb @@ -44,6 +44,17 @@ def create end end + def update + authorize @rubygem, :update_owner? + owner = User.find_by_name(handle_params) + ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params) + if ownership.update(update_params) + redirect_to rubygem_owners_path(ownership.rubygem.slug), notice: t(".success_notice", handle: ownership.user.name) + else + index_with_error @ownership.errors.full_messages.to_sentence, :unprocessable_entity + end + end + def destroy authorize @rubygem, :remove_owner? @ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params) @@ -65,6 +76,10 @@ def token_params params.permit(:token).require(:token) end + def update_params + params.permit(:access_level) + end + def handle_params params.permit(:handle).require(:handle) end diff --git a/app/models/api_key.rb b/app/models/api_key.rb index 98b6bbacacf..9a41d74fa29 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -1,9 +1,9 @@ class ApiKey < ApplicationRecord class ScopeError < RuntimeError; end - API_SCOPES = %i[show_dashboard index_rubygems push_rubygem yank_rubygem add_owner remove_owner access_webhooks + API_SCOPES = %i[show_dashboard index_rubygems push_rubygem yank_rubygem add_owner update_owner remove_owner access_webhooks configure_trusted_publishers].freeze - APPLICABLE_GEM_API_SCOPES = %i[push_rubygem yank_rubygem add_owner remove_owner configure_trusted_publishers].freeze + APPLICABLE_GEM_API_SCOPES = %i[push_rubygem yank_rubygem add_owner update_owner remove_owner configure_trusted_publishers].freeze EXCLUSIVE_SCOPES = %i[show_dashboard].freeze self.ignored_columns += API_SCOPES diff --git a/app/models/rubygem.rb b/app/models/rubygem.rb index a306bcbfd31..5ba7ceeefb4 100644 --- a/app/models/rubygem.rb +++ b/app/models/rubygem.rb @@ -188,6 +188,11 @@ def owned_by?(user) ownerships.exists?(user_id: user.id) end + def owned_by_with_access_level?(user, access_level) + return false unless user + ownerships.where("user_id = ? AND access_level >= ?", user.id, access_level).exists? + end + def unconfirmed_ownerships ownerships_including_unconfirmed.unconfirmed end diff --git a/app/policies/api/rubygem_policy.rb b/app/policies/api/rubygem_policy.rb index e181f66850a..368b2547d1b 100644 --- a/app/policies/api/rubygem_policy.rb +++ b/app/policies/api/rubygem_policy.rb @@ -33,6 +33,13 @@ def remove_owner? user_authorized?(rubygem, :remove_owner?) end + def update_owner? + user_api_key? && + mfa_requirement_satisfied?(rubygem) && + api_key_scope?(:update_owner, rubygem) && + user_authorized?(rubygem, :update_owner?) + end + def configure_trusted_publishers? user_api_key? && mfa_requirement_satisfied?(rubygem) && diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 0ec698761ba..86f6e27a55d 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -76,8 +76,8 @@ def current_user?(record_user) user && user == record_user end - def rubygem_owned_by?(user) - rubygem.owned_by?(user) || deny(t(:forbidden)) + def rubygem_owned_by?(user, required_access_level: Access::MAINTAINER) + rubygem.owned_by_with_access_level?(user, required_access_level) || deny(t(:forbidden)) end def policy!(user, record) = Pundit.policy!(user, record) diff --git a/app/policies/ownership_policy.rb b/app/policies/ownership_policy.rb index de8fbc958fa..d01f8d65385 100644 --- a/app/policies/ownership_policy.rb +++ b/app/policies/ownership_policy.rb @@ -8,6 +8,10 @@ def create? policy!(user, rubygem).add_owner? end + def update? + policy!(user, rubygem).update_owner? + end + def destroy? policy!(user, rubygem).remove_owner? end diff --git a/app/policies/rubygem_policy.rb b/app/policies/rubygem_policy.rb index 56a5c02ec58..5cdd219920d 100644 --- a/app/policies/rubygem_policy.rb +++ b/app/policies/rubygem_policy.rb @@ -23,24 +23,16 @@ def destroy? false end - def add_owner? - rubygem_owned_by?(user) - end - def configure_oidc? - rubygem_owned_by?(user) + rubygem_owned_by?(user, required_access_level: Access::OWNER) end def configure_trusted_publishers? - rubygem_owned_by?(user) + rubygem_owned_by?(user, required_access_level: Access::OWNER) end def manage_adoption? - rubygem_owned_by?(user) - end - - def remove_owner? - rubygem_owned_by?(user) + rubygem_owned_by?(user, required_access_level: Access::OWNER) end def request_ownership? @@ -61,4 +53,16 @@ def show_events? def show_unconfirmed_ownerships? rubygem_owned_by?(user) end + + def add_owner? + rubygem_owned_by?(user, required_access_level: Access::OWNER) + end + + def update_owner? + rubygem_owned_by?(user, required_access_level: Access::OWNER) + end + + def remove_owner? + rubygem_owned_by?(user, required_access_level: Access::OWNER) + end end diff --git a/config/initializers/requires.rb b/config/initializers/requires.rb index 966db0b8de5..8a6f441cff6 100644 --- a/config/initializers/requires.rb +++ b/config/initializers/requires.rb @@ -5,3 +5,4 @@ require 'rack/rewindable_input' require 'elastic_searcher' require 'github_secret_scanning' +require 'access' diff --git a/config/locales/en.yml b/config/locales/en.yml index b3c9b6c60fd..8c394728b0c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -553,6 +553,8 @@ en: resent_notice: A confirmation mail has been re-sent to your email create: success_notice: "%{handle} was added as an unconfirmed owner. Ownership access will be enabled after the user clicks on the confirmation mail sent to their email." + update: + success_notice: "%{handle} was succesfully updated." destroy: removed_notice: "%{owner_name} was removed from the owners successfully" failed_notice: Can't remove the only owner of the gem @@ -976,3 +978,7 @@ en: api_key_gem_html: "Gem: %{gem}" api_key_mfa: "MFA: %{mfa}" not_required: "Not required" + access: + roles: + owner: Owner + maintainer: Maintainer diff --git a/config/routes.rb b/config/routes.rb index fd5871e8c0c..f3c458175db 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -87,7 +87,7 @@ delete :yank, to: "deletions#create" end constraints rubygem_id: Patterns::ROUTE_PATTERN do - resource :owners, only: %i[show create destroy] + resource :owners, only: %i[show create update destroy] resources :trusted_publishers, controller: 'oidc/rubygem_trusted_publishers', only: %i[index create destroy show] end end @@ -211,7 +211,7 @@ get '/dependencies', to: 'dependencies#show', constraints: { format: /json|html/ } end resources :reverse_dependencies, only: %i[index] - resources :owners, only: %i[index destroy create], param: :handle do + resources :owners, only: %i[index destroy update create], param: :handle do get 'confirm', to: 'owners#confirm', as: :confirm, on: :collection get 'resend_confirmation', to: 'owners#resend_confirmation', as: :resend_confirmation, on: :collection end diff --git a/db/migrate/20240717081704_add_access_level_to_ownership.rb b/db/migrate/20240717081704_add_access_level_to_ownership.rb new file mode 100644 index 00000000000..7c9bbde3229 --- /dev/null +++ b/db/migrate/20240717081704_add_access_level_to_ownership.rb @@ -0,0 +1,5 @@ +class AddAccessLevelToOwnership < ActiveRecord::Migration[7.1] + def change + add_column :ownerships, :access_level, :integer, null: false, default: 0 + end +end diff --git a/db/schema.rb b/db/schema.rb index 8fbfefc8343..e0ceb3bbea9 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_07_22_182907) do +ActiveRecord::Schema[7.1].define(version: 2024_08_02_182907) do # These are extensions that must be enabled in order to support this database enable_extension "hstore" enable_extension "pgcrypto" @@ -455,6 +455,7 @@ t.boolean "owner_notifier", default: true, null: false t.integer "authorizer_id" t.boolean "ownership_request_notifier", default: true, null: false + t.integer "access_level", default: 0, null: false t.index ["rubygem_id"], name: "index_ownerships_on_rubygem_id" t.index ["user_id", "rubygem_id"], name: "index_ownerships_on_user_id_and_rubygem_id", unique: true end diff --git a/lib/access.rb b/lib/access.rb new file mode 100644 index 00000000000..81672f1c861 --- /dev/null +++ b/lib/access.rb @@ -0,0 +1,30 @@ +module Access + AccessDeniedError = Class.new(StandardError) + + MAINTAINER = 50 + OWNER = 70 + + ROLES = { + maintainer: MAINTAINER, + owner: OWNER + } + + def self.label_for_role(role) + key = ROLES.fetch(role.to_sym) { nil } + return nil if key.nil? + I18n.t("access.roles.#{role}") + end + + def self.label_for_role_flag(flag) + raise ArgumentError, "flag must be an integer" unless flag.is_a?(Integer) + role = ROLES.key(flag) { nil } + return nil if role.nil? + I18n.t("access.roles.#{role}") + end + + def self.options + ROLES.map do |_, permission| + [self.label_for_role(role), permission] + end + end +end diff --git a/test/factories/ownership.rb b/test/factories/ownership.rb index 196cb80382a..92a40d096ab 100644 --- a/test/factories/ownership.rb +++ b/test/factories/ownership.rb @@ -7,5 +7,10 @@ trait :unconfirmed do confirmed_at { nil } end + access_level { Access::OWNER } + + trait :maintainer do + access_level { Access::MAINTAINER } + end end end diff --git a/test/factories/rubygem.rb b/test/factories/rubygem.rb index 78224af58d9..fd877c4f5bb 100644 --- a/test/factories/rubygem.rb +++ b/test/factories/rubygem.rb @@ -2,6 +2,7 @@ factory :rubygem do transient do owners { [] } + maintainers { [] } number { nil } downloads { 0 } end @@ -18,7 +19,11 @@ after(:create) do |rubygem, evaluator| evaluator.owners.each do |owner| - create(:ownership, rubygem: rubygem, user: owner) + create(:ownership, rubygem: rubygem, user: owner, access_level: Access::OWNER) + end + + evaluator.maintainers.each do |maintainer| + create(:ownership, rubygem: rubygem, user: maintainer, access_level: Access::MAINTAINER) end create(:version, rubygem: rubygem, number: evaluator.number) if evaluator.number diff --git a/test/functional/api/v1/owners_controller_test.rb b/test/functional/api/v1/owners_controller_test.rb index c0755b85999..52556cd0950 100644 --- a/test/functional/api/v1/owners_controller_test.rb +++ b/test/functional/api/v1/owners_controller_test.rb @@ -927,4 +927,31 @@ def self.should_respond_to(format) assert_equal "This rubygem could not be found.", @response.body end + + should "route PUT /api/v1/gems/rubygem/owners.yaml" do + route = { controller: "api/v1/owners", + action: "update", + rubygem_id: "rails", + format: "yaml" } + + assert_recognizes(route, path: "/api/v1/gems/rails/owners.yaml", method: :put) + end + + context "on PATCH to owner gem" do + setup do + @user = create(:user) + @maintainer = create(:user) + @rubygem = create(:rubygem, owners: [@user, @maintainer]) + + @api_key = create(:api_key, key: "12223", scopes: %i[update_owner], owner: @user, rubygem: @rubygem) + @request.env["HTTP_AUTHORIZATION"] = "12223" + end + + should "set the maintainer to a lower access level" do + patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, access_level: Access::MAINTAINER } + + assert_response :success + assert_equal Access::MAINTAINER, @maintainer.reload.ownerships.find_by(rubygem: @rubygem).access_level + end + end end diff --git a/test/functional/owners_controller_test.rb b/test/functional/owners_controller_test.rb index 30cbb604abc..a93096d9a4a 100644 --- a/test/functional/owners_controller_test.rb +++ b/test/functional/owners_controller_test.rb @@ -344,6 +344,23 @@ class OwnersControllerTest < ActionController::TestCase end end end + + context "on PATCH to update ownership" do + setup do + @owner = create(:user) + @maintainer = create(:user) + @rubygem = create(:rubygem, owners: [@owner, @maintainer]) + + verified_sign_in_as(@owner) + patch :update, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id, access_level: Access::MAINTAINER } + end + + should redirect_to("rubygem show") { rubygem_owners_path(@rubygem.slug) } + should "set success notice flash" do + success_flash = "#{@maintainer.name} was succesfully updated." + assert_equal success_flash, flash[:notice] + end + end end context "when logged in and unverified" do diff --git a/test/lib/access_test.rb b/test/lib/access_test.rb new file mode 100644 index 00000000000..03877438e1f --- /dev/null +++ b/test/lib/access_test.rb @@ -0,0 +1,35 @@ +require "test_helper" + +class AccessTest < ActiveSupport::TestCase + context ".label_for_role" do + should "return the correct label for the given role" do + assert_equal "Owner", Access.label_for_role(:owner) + end + + should "cast the given input into the correct type" do + assert_equal "Owner", Access.label_for_role("owner") + end + + should "return nil when the role is unknown" do + assert_nil Access.label_for_role(:unknown) + end + end + + context ".label_for_role_flag" do + should "return the label for the role flag" do + assert_equal "Owner", Access.label_for_role_flag(Access::OWNER) + end + + context "when the role flag is invalid" do + should "return nil" do + assert_nil Access.label_for_role_flag(999) + end + end + + context "when the input is not a string" do + should "raise an ArgumentError" do + assert_raises(ArgumentError) { Access.label_for_role_flag("owner") } + end + end + end +end diff --git a/test/policies/ownership_policy_test.rb b/test/policies/ownership_policy_test.rb index b6d77fa0846..601f8ab8fb8 100644 --- a/test/policies/ownership_policy_test.rb +++ b/test/policies/ownership_policy_test.rb @@ -2,10 +2,13 @@ class OwnershipPolicyTest < ActiveSupport::TestCase setup do - @authorizer = FactoryBot.create(:user) - @rubygem = FactoryBot.create(:rubygem, owners: [@authorizer]) + @authorizer = FactoryBot.create(:user, handle: "owner") + @maintainer = FactoryBot.create(:user, handle: "maintainer") + @rubygem = FactoryBot.create(:rubygem, owners: [@authorizer], maintainers: [@maintainer]) @confirmed_ownership = @rubygem.ownerships.first + @confirmed_maintainer_ownership = @maintainer.ownerships.first @unconfirmed_ownership = FactoryBot.build(:ownership, :unconfirmed, rubygem: @rubygem, authorizer: @authorizer) + @unconfirmed_maintainer_ownership = FactoryBot.build(:ownership, :maintainer, rubygem: @rubygem, authorizer: @authorizer) @invited = @unconfirmed_ownership.user @user = FactoryBot.create(:user) @@ -15,11 +18,24 @@ def test_create assert_predicate Pundit.policy!(@authorizer, @unconfirmed_ownership), :create? refute_predicate Pundit.policy!(@invited, @unconfirmed_ownership), :create? refute_predicate Pundit.policy!(@user, @unconfirmed_ownership), :create? + refute_predicate Pundit.policy!(@maintainer, @confirmed_maintainer_ownership), :create? + refute_predicate Pundit.policy!(@maintainer, @unconfirmed_maintainer_ownership), :create? + end + + def test_update + assert_predicate Pundit.policy!(@authorizer, @confirmed_ownership), :update? + refute_predicate Pundit.policy!(@invited, @unconfirmed_ownership), :update? + refute_predicate Pundit.policy!(@user, @unconfirmed_ownership), :update? + refute_predicate Pundit.policy!(@user, @unconfirmed_maintainer_ownership), :update? + refute_predicate Pundit.policy!(@maintainer, @confirmed_maintainer_ownership), :update? + refute_predicate Pundit.policy!(@maintainer, @unconfirmed_maintainer_ownership), :update? end def test_destroy assert_predicate Pundit.policy!(@authorizer, @confirmed_ownership), :destroy? refute_predicate Pundit.policy!(@owner, @confirmed_ownership), :destroy? refute_predicate Pundit.policy!(@user, @confirmed_ownership), :destroy? + refute_predicate Pundit.policy!(@user, @confirmed_maintainer_ownership), :destroy? + refute_predicate Pundit.policy!(@user, @unconfirmed_maintainer_ownership), :destroy? end end diff --git a/test/policies/rubygem_policy_test.rb b/test/policies/rubygem_policy_test.rb index c5cefe7a214..deca04c2a1b 100644 --- a/test/policies/rubygem_policy_test.rb +++ b/test/policies/rubygem_policy_test.rb @@ -3,7 +3,8 @@ class RubygemPolicyTest < PolicyTestCase setup do @owner = create(:user, handle: "owner") - @rubygem = create(:rubygem, owners: [@owner]) + @maintainer = create(:user, handle: "maintainer") + @rubygem = create(:rubygem, owners: [@owner], maintainers: [@maintainer]) @user = create(:user, handle: "user") end @@ -19,14 +20,6 @@ def policy!(user) end end - context "#configure_trusted_publishers?" do - should "only allow the owner" do - assert_authorized @owner, :configure_trusted_publishers? - refute_authorized @user, :configure_trusted_publishers? - refute_authorized nil, :configure_trusted_publishers? - end - end - context "#manage_adoption?" do should "only allow the owner" do assert_authorized @owner, :manage_adoption? @@ -71,6 +64,7 @@ def policy!(user) context "#show_adoption?" do should "be true if the gem is owned by the user" do assert_authorized @owner, :show_adoption? + refute_authorized @maintainer, :show_adoption? end should "be true if the rubygem is adoptable" do @@ -83,16 +77,54 @@ def policy!(user) context "#show_events?" do should "only allow the owner" do assert_authorized @owner, :show_events? + assert_authorized @maintainer, :show_events? refute_authorized @user, :show_events? refute_authorized nil, :show_events? end end + context "#configure_trusted_publishers?" do + should "only allow the owner" do + assert_authorized @owner, :configure_trusted_publishers? + assert_authorized @maintainer, :configure_trusted_publishers? + refute_authorized @user, :configure_trusted_publishers? + refute_authorized nil, :configure_trusted_publishers? + end + end + context "#show_unconfirmed_ownerships?" do should "only allow the owner" do assert_authorized @owner, :show_unconfirmed_ownerships? + assert_authorized @maintainer, :show_unconfirmed_ownerships? refute_authorized @user, :show_unconfirmed_ownerships? refute_authorized nil, :show_unconfirmed_ownerships? end end + + context "#add_owner?" do + should "only allow the owner" do + assert_authorized @owner, :add_owner? + refute_authorized @maintainer, :add_owner? + refute_authorized @user, :add_owner? + refute_authorized nil, :add_owner? + end + end + + context "#update_owner?" do + should "only allow the owner" do + assert_authorized @owner, :update_owner? + refute_authorized @maintainer, :update_owner? + refute_authorized @user, :update_owner? + refute_authorized nil, :update_owner? + end + end + + context "#remove_owner?" do + should "only allow the owner" do + assert_authorized @owner, :remove_owner? + refute_authorized @maintainer, :remove_owner? + refute_authorized @user, :remove_owner? + refute_authorized nil, :remove_owner? + end + end end From bed0451ad53be4a9c1ec578a848e43981f7be32e Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Fri, 6 Sep 2024 15:25:21 +1000 Subject: [PATCH 02/28] Add Ownership edit form to owners table --- app/controllers/api/v1/owners_controller.rb | 1 + app/controllers/owners_controller.rb | 15 ++- app/views/owners/_owners_table.html.erb | 7 ++ app/views/owners/edit.html.erb | 20 ++++ config/locales/en.yml | 3 + config/routes.rb | 4 +- ...02151324_add_access_level_to_ownership.rb} | 2 +- db/schema.rb | 4 +- lib/access.rb | 2 +- test/factories/ownership.rb | 3 +- test/functional/owners_controller_test.rb | 93 ++++++++++++++++++- test/integration/owner_test.rb | 17 ++++ 12 files changed, 157 insertions(+), 14 deletions(-) create mode 100644 app/views/owners/edit.html.erb rename db/migrate/{20240717081704_add_access_level_to_ownership.rb => 20240802151324_add_access_level_to_ownership.rb} (83%) diff --git a/app/controllers/api/v1/owners_controller.rb b/app/controllers/api/v1/owners_controller.rb index 2b502a5447b..8b69ffecbc2 100644 --- a/app/controllers/api/v1/owners_controller.rb +++ b/app/controllers/api/v1/owners_controller.rb @@ -15,6 +15,7 @@ def show def update authorize @rubygem, :update_owner? ownership = @rubygem.ownerships.find_by!(user: User.find_by_name!(email_param)) + if ownership.present? ownership.update!(ownership_update_params) render plain: response_with_mfa_warning("Owner updated successfully.") diff --git a/app/controllers/owners_controller.rb b/app/controllers/owners_controller.rb index 31a8ab68ff0..1fb95708f37 100644 --- a/app/controllers/owners_controller.rb +++ b/app/controllers/owners_controller.rb @@ -2,8 +2,8 @@ class OwnersController < ApplicationController include SessionVerifiable before_action :find_rubygem, except: :confirm - verify_session_before only: %i[index create destroy] - before_action :verify_mfa_requirement, only: %i[create destroy] + verify_session_before only: %i[index edit update create destroy] + before_action :verify_mfa_requirement, only: %i[create edit update destroy] def confirm ownership = Ownership.find_by!(token: token_params) @@ -32,6 +32,11 @@ def index @ownerships = @rubygem.ownerships_including_unconfirmed.includes(:user, :authorizer) end + def edit + authorize @rubygem, :update_owner? + @ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params) + end + def create authorize @rubygem, :add_owner? owner = User.find_by_name(handle_params) @@ -48,6 +53,10 @@ def update authorize @rubygem, :update_owner? owner = User.find_by_name(handle_params) ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params) + + # Don't allow the owner to change the access level of their own ownership + return redirect_to rubygem_owners_path(@rubygem.slug), alert: "You can't update your own access level" if ownership.user == current_user + if ownership.update(update_params) redirect_to rubygem_owners_path(ownership.rubygem.slug), notice: t(".success_notice", handle: ownership.user.name) else @@ -77,7 +86,7 @@ def token_params end def update_params - params.permit(:access_level) + params.require(:ownership).permit(:access_level) end def handle_params diff --git a/app/views/owners/_owners_table.html.erb b/app/views/owners/_owners_table.html.erb index ec78a4160d5..fe90e4979ac 100644 --- a/app/views/owners/_owners_table.html.erb +++ b/app/views/owners/_owners_table.html.erb @@ -13,6 +13,9 @@ <%= t("owners.index.added_by") %> + + <%= t("owners.index.role") %> + <%= t("owners.index.confirmed_at") %> @@ -40,6 +43,9 @@ <%= ownership.authorizer_name %> + + <%= Access.label_for_role_flag(ownership.access_level) %> + <%= ownership.confirmed_at.strftime("%Y-%m-%d %H:%M %Z") if ownership.confirmed? %> @@ -49,6 +55,7 @@ method: "delete", data: { confirm: t("owners.index.confirm_remove") }, class: "form__submit form__submit--small" %> + <%= link_to "Edit", edit_rubygem_owner_path(@rubygem.name, ownership.user.display_id), class: "form__submit form__submit--small" %> <% end %> diff --git a/app/views/owners/edit.html.erb b/app/views/owners/edit.html.erb new file mode 100644 index 00000000000..6b02519d55f --- /dev/null +++ b/app/views/owners/edit.html.erb @@ -0,0 +1,20 @@ +<% @title = t('.title') %> + +<%= form_for @ownership, :url => {:controller => 'owners', :action => 'update'} do |form| %> + <%= error_messages_for(@ownership) %> + +
+ <%= form.label :display_id, "User", :class => 'form__label' %> + <%= form.text_field :display_id, value: @ownership.user.display_id, disabled: true, :class => 'form__input' %> +
+ +
+ <%= form.label :role, :class => 'form__label' %> +
+ <%= form.select :access_level, Access.options.to_a %> +
+ +
+ <%= form.submit 'Update', :data => {:disable_with => t('form_disable_with')}, :class => 'form__submit' %> +
+<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 8c394728b0c..4a0eab71a8c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -532,6 +532,8 @@ en: title: Error - Verification Failed close_browser: Please close this browser and try again. owners: + edit: + title: Edit Owner confirm: confirmed_email: You were added as an owner to %{gem} gem token_expired: The confirmation token has expired. Please try resending the token from the gem page. @@ -543,6 +545,7 @@ en: confirmed_at: CONFIRMED AT added_by: ADDED BY action: ACTION + role: ROLE email_field: Email / Handle submit_button: Add Owner info: add or remove owners diff --git a/config/routes.rb b/config/routes.rb index f3c458175db..03dd015b3d2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -87,7 +87,7 @@ delete :yank, to: "deletions#create" end constraints rubygem_id: Patterns::ROUTE_PATTERN do - resource :owners, only: %i[show create update destroy] + resource :owners, only: %i[show create edit update destroy] resources :trusted_publishers, controller: 'oidc/rubygem_trusted_publishers', only: %i[index create destroy show] end end @@ -211,7 +211,7 @@ get '/dependencies', to: 'dependencies#show', constraints: { format: /json|html/ } end resources :reverse_dependencies, only: %i[index] - resources :owners, only: %i[index destroy update create], param: :handle do + resources :owners, only: %i[index destroy edit update create], param: :handle do get 'confirm', to: 'owners#confirm', as: :confirm, on: :collection get 'resend_confirmation', to: 'owners#resend_confirmation', as: :resend_confirmation, on: :collection end diff --git a/db/migrate/20240717081704_add_access_level_to_ownership.rb b/db/migrate/20240802151324_add_access_level_to_ownership.rb similarity index 83% rename from db/migrate/20240717081704_add_access_level_to_ownership.rb rename to db/migrate/20240802151324_add_access_level_to_ownership.rb index 7c9bbde3229..b858b8cc8cb 100644 --- a/db/migrate/20240717081704_add_access_level_to_ownership.rb +++ b/db/migrate/20240802151324_add_access_level_to_ownership.rb @@ -1,5 +1,5 @@ class AddAccessLevelToOwnership < ActiveRecord::Migration[7.1] def change - add_column :ownerships, :access_level, :integer, null: false, default: 0 + add_column :ownerships, :access_level, :integer, null: false, default: 70 # Access::OWNER end end diff --git a/db/schema.rb b/db/schema.rb index e0ceb3bbea9..0ff0b89b477 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_08_02_182907) do +ActiveRecord::Schema[7.1].define(version: 2024_08_02_151324) do # These are extensions that must be enabled in order to support this database enable_extension "hstore" enable_extension "pgcrypto" @@ -455,7 +455,7 @@ t.boolean "owner_notifier", default: true, null: false t.integer "authorizer_id" t.boolean "ownership_request_notifier", default: true, null: false - t.integer "access_level", default: 0, null: false + t.integer "access_level", default: 70, null: false t.index ["rubygem_id"], name: "index_ownerships_on_rubygem_id" t.index ["user_id", "rubygem_id"], name: "index_ownerships_on_user_id_and_rubygem_id", unique: true end diff --git a/lib/access.rb b/lib/access.rb index 81672f1c861..84e7923227f 100644 --- a/lib/access.rb +++ b/lib/access.rb @@ -24,7 +24,7 @@ def self.label_for_role_flag(flag) def self.options ROLES.map do |_, permission| - [self.label_for_role(role), permission] + [label_for_role_flag(permission), permission] end end end diff --git a/test/factories/ownership.rb b/test/factories/ownership.rb index 92a40d096ab..c4546691905 100644 --- a/test/factories/ownership.rb +++ b/test/factories/ownership.rb @@ -4,10 +4,11 @@ user confirmed_at { Time.current } authorizer { association :user } + access_level { Access::OWNER } + trait :unconfirmed do confirmed_at { nil } end - access_level { Access::OWNER } trait :maintainer do access_level { Access::MAINTAINER } diff --git a/test/functional/owners_controller_test.rb b/test/functional/owners_controller_test.rb index a93096d9a4a..da4e250819c 100644 --- a/test/functional/owners_controller_test.rb +++ b/test/functional/owners_controller_test.rb @@ -345,6 +345,21 @@ class OwnersControllerTest < ActionController::TestCase end end + context "on GET edit ownership" do + setup do + @owner = create(:user) + @maintainer = create(:user) + @rubygem = create(:rubygem, owners: [@owner, @maintainer]) + + verified_sign_in_as(@owner) + + get :edit, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id } + end + + should respond_with :success + should render_template :edit + end + context "on PATCH to update ownership" do setup do @owner = create(:user) @@ -352,14 +367,35 @@ class OwnersControllerTest < ActionController::TestCase @rubygem = create(:rubygem, owners: [@owner, @maintainer]) verified_sign_in_as(@owner) - patch :update, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id, access_level: Access::MAINTAINER } + patch :update, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id, ownership: { access_level: Access::MAINTAINER } } end should redirect_to("rubygem show") { rubygem_owners_path(@rubygem.slug) } should "set success notice flash" do success_flash = "#{@maintainer.name} was succesfully updated." + assert_equal success_flash, flash[:notice] end + + should "downgrade the maintainer" do + owner = @rubygem.ownerships.find_by(user_id: @owner.id) + assert_equal Access::OWNER, owner.access_level + end + + context "when updating the current user" do + setup do + patch :update, params: { rubygem_id: @rubygem.name, handle: @owner.display_id, access_level: Access::OWNER } + end + + should "not update the ownership of the current user" do + owner = @rubygem.ownerships.find_by(user_id: @owner.id) + assert_equal Access::OWNER, owner.access_level + end + + should "set notice flash message" do + assert_equal "You can't update your own access level.", flash[:notice] + end + end end end @@ -397,14 +433,41 @@ class OwnersControllerTest < ActionController::TestCase context "on DELETE to owners" do setup do @second_user = create(:user) - @ownership = create(:ownership, rubygem: @rubygem, user: @second_user) + @ownership = create(:ownership, :unconfirmed, rubygem: @rubygem, user: @second_user) delete :destroy, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id } end should redirect_to("sessions#verify") { verify_session_path } should use_before_action(:redirect_to_verify) - should "remove the ownership record" do - assert_includes @rubygem.owners_including_unconfirmed, @second_user + should "not remove the ownership record" do + assert_includes @rubygem.owners, @second_user + end + end + + context "on EDIT to owners" do + setup do + @second_user = create(:user) + @ownership = create(:ownership, :unconfirmed, rubygem: @rubygem, user: @second_user) + edit :edit, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id } + + should redirect_to("sessions#verify") { verify_session_path } + should use_before_action(:redirect_to_verify) + + should "show the edit form" do + assert_not_template :edit + end + end + end + + context "on UPDATE to owners" do + setup do + @second_user = create(:user) + @ownership = create(:ownership, :unconfirmed, rubygem: @rubygem, user: @second_user) + patch :update, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id, access_level: Access::MAINTAINER } + + should "update the ownership record" do + assert_equal Access::MAINTAINER, @ownership.reload.access + end end end end @@ -502,5 +565,27 @@ class OwnersControllerTest < ActionController::TestCase assert redirect_to("sign in") { sign_in_path } end end + + context "on EDIT to update owner" do + setup do + create(:ownership, rubygem: @rubygem, user: @user) + get :edit, params: { rubygem_id: @rubygem.name, handle: @user.display_id } + end + + should "redirect to sign in path" do + assert redirect_to("sign in") { sign_in_path } + end + end + + context "on PATCH to update owner" do + setup do + create(:ownership, rubygem: @rubygem, user: @user) + patch :update, params: { rubygem_id: @rubygem.name, handle: @user.display_id, access_level: Access::OWNER } + end + + should "redirect to sign in path" do + assert redirect_to("sign in") { sign_in_path } + end + end end end diff --git a/test/integration/owner_test.rb b/test/integration/owner_test.rb index d7aa11e91bb..5e45aea7b27 100644 --- a/test/integration/owner_test.rb +++ b/test/integration/owner_test.rb @@ -256,6 +256,23 @@ class OwnerTest < SystemTest refute page.has_content? @other_user.handle end + test "updating user to maintainer role" do + maintainer = create(:user) + create(:ownership, user: maintainer, rubygem: @rubygem) + + visit_ownerships_page + + within_element owner_row(maintainer) do + click_link "Edit" + end + + select "Maintainer", from: "ownership_access_level" + + click_button "Update" + + assert_cell maintainer, "Role", "Maintainer" + end + teardown do @authenticator&.remove! Capybara.reset_sessions! From 13dc4a5585803bc4ed94937af5adf1a6cc4a4035 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Sat, 3 Aug 2024 02:24:58 +1000 Subject: [PATCH 03/28] Add role field to add Ownership form --- app/controllers/owners_controller.rb | 3 ++- app/views/owners/index.html.erb | 5 +++++ config/locales/en.yml | 1 + test/integration/owner_test.rb | 13 +++++++++++++ 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/controllers/owners_controller.rb b/app/controllers/owners_controller.rb index 1fb95708f37..d1936f89987 100644 --- a/app/controllers/owners_controller.rb +++ b/app/controllers/owners_controller.rb @@ -40,7 +40,8 @@ def edit def create authorize @rubygem, :add_owner? owner = User.find_by_name(handle_params) - ownership = @rubygem.ownerships.new(user: owner, authorizer: current_user) + + ownership = @rubygem.ownerships.new(user: owner, authorizer: current_user, access_level: params[:access_level].to_i) if ownership.save OwnersMailer.ownership_confirmation(ownership).deliver_later redirect_to rubygem_owners_path(@rubygem.slug), notice: t(".success_notice", handle: owner.name) diff --git a/app/views/owners/index.html.erb b/app/views/owners/index.html.erb index 75bc1084b47..0d4b22dd657 100644 --- a/app/views/owners/index.html.erb +++ b/app/views/owners/index.html.erb @@ -14,6 +14,11 @@ <%= label_tag :handle, t(".email_field"), class: "form__label" %> <%= text_field_tag :handle, nil, class: "form__input", required: true %> +
+ <%= label_tag :access_level, t(".access_level"), class: "form__label" %> +
+ <%= select_tag :access_level, options_for_select(Access.options.to_a.reverse) %> +
<%= submit_tag t(".submit_button"), data: {disable_with: t("form_disable_with")}, class: "form__submit" %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index 4a0eab71a8c..cbff385ce1d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -552,6 +552,7 @@ en: confirmed: Confirmed pending: Pending confirm_remove: Are you sure you want to remove this user from the owners? + access_level: Access Level resend_confirmation: resent_notice: A confirmation mail has been re-sent to your email create: diff --git a/test/integration/owner_test.rb b/test/integration/owner_test.rb index 5e45aea7b27..8a4a26a451e 100644 --- a/test/integration/owner_test.rb +++ b/test/integration/owner_test.rb @@ -273,6 +273,19 @@ class OwnerTest < SystemTest assert_cell maintainer, "Role", "Maintainer" end + test "creating new owner with maintainer role" do + maintainer = create(:user) + + visit_ownerships_page + + fill_in "Handle", with: maintainer.handle + select "Maintainer", from: "access_level" + + click_button "Add Owner" + + assert_cell maintainer, "Role", "Maintainer" + end + teardown do @authenticator&.remove! Capybara.reset_sessions! From ead29388655803bfa06f6687fd5c02e5abdbfa27 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Tue, 6 Aug 2024 17:10:15 +1000 Subject: [PATCH 04/28] Replace Access Level field with Role --- app/controllers/api/v1/owners_controller.rb | 24 +++++------ app/controllers/owners_controller.rb | 14 +++--- app/models/rubygem.rb | 4 +- app/policies/rubygem_policy.rb | 6 ++- app/views/owners/edit.html.erb | 12 +++--- app/views/owners/index.html.erb | 4 +- config/locales/de.yml | 14 ++++++ config/locales/en.yml | 6 ++- config/locales/es.yml | 14 ++++++ config/locales/fr.yml | 14 ++++++ config/locales/ja.yml | 14 ++++++ config/locales/nl.yml | 14 ++++++ config/locales/pt-BR.yml | 14 ++++++ config/locales/zh-CN.yml | 14 ++++++ config/locales/zh-TW.yml | 14 ++++++ lib/access.rb | 17 ++++++-- .../api/v1/owners_controller_test.rb | 10 ++--- test/functional/owners_controller_test.rb | 43 +++++++++++-------- test/integration/owner_test.rb | 4 +- test/lib/access_test.rb | 16 +++++++ test/policies/rubygem_policy_test.rb | 12 +++++- test/system/avo/rubygems_test.rb | 3 +- 22 files changed, 222 insertions(+), 65 deletions(-) diff --git a/app/controllers/api/v1/owners_controller.rb b/app/controllers/api/v1/owners_controller.rb index 8b69ffecbc2..72f19be1f71 100644 --- a/app/controllers/api/v1/owners_controller.rb +++ b/app/controllers/api/v1/owners_controller.rb @@ -12,18 +12,6 @@ def show end end - def update - authorize @rubygem, :update_owner? - ownership = @rubygem.ownerships.find_by!(user: User.find_by_name!(email_param)) - - if ownership.present? - ownership.update!(ownership_update_params) - render plain: response_with_mfa_warning("Owner updated successfully.") - else - render plain: response_with_mfa_warning("Unable to update owner."), status: :forbidden - end - end - def create authorize @rubygem, :add_owner? owner = User.find_by_name!(email_param) @@ -38,6 +26,18 @@ def create end end + def update + authorize @rubygem, :update_owner? + ownership = @rubygem.ownerships.find_by!(user: User.find_by_name!(email_param)) + + if ownership.present? + ownership.update!(ownership_update_params) + render plain: response_with_mfa_warning("Owner updated successfully.") + else + render plain: response_with_mfa_warning("Unable to update owner."), status: :forbidden + end + end + def destroy authorize @rubygem, :remove_owner? owner = User.find_by_name!(email_param) diff --git a/app/controllers/owners_controller.rb b/app/controllers/owners_controller.rb index d1936f89987..f6f8bc968d8 100644 --- a/app/controllers/owners_controller.rb +++ b/app/controllers/owners_controller.rb @@ -40,8 +40,10 @@ def edit def create authorize @rubygem, :add_owner? owner = User.find_by_name(handle_params) + access_level = Access.permission_for_role(params[:role]) + return redirect_to rubygem_owners_path(@rubygem.slug), alert: t(".invalid_access_level") if access_level.blank? - ownership = @rubygem.ownerships.new(user: owner, authorizer: current_user, access_level: params[:access_level].to_i) + ownership = @rubygem.ownerships.new(user: owner, authorizer: current_user, access_level: access_level) if ownership.save OwnersMailer.ownership_confirmation(ownership).deliver_later redirect_to rubygem_owners_path(@rubygem.slug), notice: t(".success_notice", handle: owner.name) @@ -52,13 +54,13 @@ def create def update authorize @rubygem, :update_owner? - owner = User.find_by_name(handle_params) ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params) # Don't allow the owner to change the access level of their own ownership - return redirect_to rubygem_owners_path(@rubygem.slug), alert: "You can't update your own access level" if ownership.user == current_user + return redirect_to rubygem_owners_path(@rubygem.slug), alert: t(".update_current_user") if ownership.user == current_user - if ownership.update(update_params) + access_level = Access.permission_for_role(params[:role]) + if ownership.update({ access_level: access_level }) redirect_to rubygem_owners_path(ownership.rubygem.slug), notice: t(".success_notice", handle: ownership.user.name) else index_with_error @ownership.errors.full_messages.to_sentence, :unprocessable_entity @@ -86,10 +88,6 @@ def token_params params.permit(:token).require(:token) end - def update_params - params.require(:ownership).permit(:access_level) - end - def handle_params params.permit(:handle).require(:handle) end diff --git a/app/models/rubygem.rb b/app/models/rubygem.rb index 5ba7ceeefb4..071dab4602d 100644 --- a/app/models/rubygem.rb +++ b/app/models/rubygem.rb @@ -189,8 +189,8 @@ def owned_by?(user) end def owned_by_with_access_level?(user, access_level) - return false unless user - ownerships.where("user_id = ? AND access_level >= ?", user.id, access_level).exists? + return false if user.blank? + ownerships.exists?(["user_id = ? AND access_level >= ?", user.id, access_level]) end def unconfirmed_ownerships diff --git a/app/policies/rubygem_policy.rb b/app/policies/rubygem_policy.rb index 5cdd219920d..32508104aa2 100644 --- a/app/policies/rubygem_policy.rb +++ b/app/policies/rubygem_policy.rb @@ -50,8 +50,12 @@ def show_events? rubygem_owned_by?(user) end + def close_ownership_requests? + rubygem_owned_by?(user, required_access_level: Access::OWNER) + end + def show_unconfirmed_ownerships? - rubygem_owned_by?(user) + rubygem_owned_by?(user, required_access_level: Access::OWNER) end def add_owner? diff --git a/app/views/owners/edit.html.erb b/app/views/owners/edit.html.erb index 6b02519d55f..7233f196269 100644 --- a/app/views/owners/edit.html.erb +++ b/app/views/owners/edit.html.erb @@ -1,20 +1,20 @@ <% @title = t('.title') %> -<%= form_for @ownership, :url => {:controller => 'owners', :action => 'update'} do |form| %> +<%= form_tag rubygem_owner_path(rubygem_id: @rubygem.slug, owner_id: @ownership.user.display_id), method: :patch do |form| %> <%= error_messages_for(@ownership) %>
- <%= form.label :display_id, "User", :class => 'form__label' %> - <%= form.text_field :display_id, value: @ownership.user.display_id, disabled: true, :class => 'form__input' %> + <%= label_tag :display_id, "User", class: 'form__label' %> + <%= text_field_tag :display_id, @ownership.user.display_id, disabled: true, :class => 'form__input' %>
- <%= form.label :role, :class => 'form__label' %> + <%= label_tag :role, t(".role"), class: 'form__label' %>
- <%= form.select :access_level, Access.options.to_a %> + <%= select_tag :role, options_for_select(Access.options.to_a.reverse, Access.role_for_permission(@ownership.access_level)) %>
- <%= form.submit 'Update', :data => {:disable_with => t('form_disable_with')}, :class => 'form__submit' %> + <%= submit_tag 'Update', :data => {:disable_with => t('form_disable_with')}, :class => 'form__submit' %>
<% end %> diff --git a/app/views/owners/index.html.erb b/app/views/owners/index.html.erb index 0d4b22dd657..b641ff371d7 100644 --- a/app/views/owners/index.html.erb +++ b/app/views/owners/index.html.erb @@ -15,9 +15,9 @@ <%= text_field_tag :handle, nil, class: "form__input", required: true %>
- <%= label_tag :access_level, t(".access_level"), class: "form__label" %> + <%= label_tag :role, t(".create_role"), class: "form__label" %>
- <%= select_tag :access_level, options_for_select(Access.options.to_a.reverse) %> + <%= select_tag :role, options_for_select(Access.options.to_a.reverse) %>
<%= submit_tag t(".submit_button"), data: {disable_with: t("form_disable_with")}, class: "form__submit" %> diff --git a/config/locales/de.yml b/config/locales/de.yml index fdbace32003..e03965a0663 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -186,6 +186,7 @@ de: können. Neuer API-Schlüssel:' mfa: MFA expiration: + update_owner: new: new_api_key: Neuer API-Schlüssel reset: @@ -636,14 +637,23 @@ de: confirmed: pending: confirm_remove: + role: + create_role: resend_confirmation: resent_notice: create: success_notice: + invalid_role: destroy: removed_notice: failed_notice: mfa_required: + update: + success_notice: + update_current_user: + edit: + role: + title: settings: edit: title: @@ -1055,3 +1065,7 @@ de: api_key_gem_html: api_key_mfa: not_required: + access: + roles: + owner: + maintainer: diff --git a/config/locales/en.yml b/config/locales/en.yml index cbff385ce1d..5be9890941f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -178,6 +178,7 @@ en: save_key: "Note that we won't be able to show the key to you again. New API key:" mfa: MFA expiration: Expiration + update_owner: Update Owner new: new_api_key: New API key reset: @@ -533,6 +534,7 @@ en: close_browser: Please close this browser and try again. owners: edit: + role: Role title: Edit Owner confirm: confirmed_email: You were added as an owner to %{gem} gem @@ -546,18 +548,20 @@ en: added_by: ADDED BY action: ACTION role: ROLE + create_role: Role email_field: Email / Handle submit_button: Add Owner info: add or remove owners confirmed: Confirmed pending: Pending confirm_remove: Are you sure you want to remove this user from the owners? - access_level: Access Level resend_confirmation: resent_notice: A confirmation mail has been re-sent to your email create: success_notice: "%{handle} was added as an unconfirmed owner. Ownership access will be enabled after the user clicks on the confirmation mail sent to their email." + invalid_role: "Incorrect role. Please select from the dropdown" update: + update_current_user: You can't update your own access level success_notice: "%{handle} was succesfully updated." destroy: removed_notice: "%{owner_name} was removed from the owners successfully" diff --git a/config/locales/es.yml b/config/locales/es.yml index 4c4f5d65958..a17c737b423 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -184,6 +184,7 @@ es: clave de API:' mfa: AMF expiration: + update_owner: new: new_api_key: Nueva clave de API reset: @@ -622,9 +623,12 @@ es: confirmed: Confirmado pending: Pendiente confirm_remove: "¿Seguro que quieres eliminar a este usuario de los propietarios?" + role: + create_role: resend_confirmation: resent_notice: Se ha reenviado un mensaje de confirmación a tu correo electrónico create: + invalid_role: success_notice: Se ha añadido a %{handle} como propietario sin confirmar. Su acceso como propietario se activará cuando haga click en el mensaje de confirmación que se le ha enviado a su correo @@ -633,6 +637,12 @@ es: failed_notice: No se puede eliminar al único propietario de una gema mfa_required: La gema tiene activado el requerimiento de AMF, configura AMF en tu cuenta por favor. + edit: + role: + title: + update: + update_current_user: + success_notice: settings: edit: title: Editar configuración @@ -1110,3 +1120,7 @@ es: api_key_gem_html: api_key_mfa: not_required: + access: + roles: + owner: + maintainer: diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 84a029f80e7..7aa7d703603 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -172,6 +172,7 @@ fr: yank_rubygem: add_owner: remove_owner: + update_owner: access_webhooks: show_dashboard: configure_trusted_publishers: @@ -560,13 +561,22 @@ fr: confirmed: pending: confirm_remove: + role: + create_role: resend_confirmation: resent_notice: create: + invalid_role: success_notice: destroy: removed_notice: failed_notice: + update: + update_current_user: + success_notice: + edit: + role: + title: mfa_required: settings: edit: @@ -1005,3 +1015,7 @@ fr: api_key_gem_html: api_key_mfa: not_required: + access: + roles: + owner: + maintainer: diff --git a/config/locales/ja.yml b/config/locales/ja.yml index a2c008f9079..5c7653e5afb 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -165,6 +165,7 @@ ja: yank_rubygem: rubygemをヤンクする add_owner: 所有者を追加する remove_owner: 所有者を削除する + update_owner: access_webhooks: webhookにアクセスする show_dashboard: ダッシュボードを表示 configure_trusted_publishers: 信頼できる発行元を構成 @@ -540,13 +541,22 @@ ja: confirmed: 確定 pending: 保留 confirm_remove: このユーザーを所有者から削除されたいとのことでよろしいですか。 + role: + create_role: resend_confirmation: resent_notice: 確定メールがEメールに再送されました。 create: + invalid_role: success_notice: "%{handle}が未確定の所有者として追加されました。当該ユーザーがEメールに送られた確定メールでクリックした後に所有権アクセスが有効になります。" destroy: removed_notice: "%{owner_name}は所有者から正常に削除されました" failed_notice: gemの唯一の所有者は削除できません。 + edit: + title: + role: + update: + update_current_user: + success_notice: mfa_required: gemでMFAの要件が有効化されているため、アカウントにMFAを設定してください。 settings: edit: @@ -981,3 +991,7 @@ ja: api_key_gem_html: 'gem: %{gem}' api_key_mfa: 'MFA: %{mfa}' not_required: 必要ではありません + access: + roles: + owner: + maintainer: diff --git a/config/locales/nl.yml b/config/locales/nl.yml index 2c41e12c4f2..29e265f37ac 100644 --- a/config/locales/nl.yml +++ b/config/locales/nl.yml @@ -164,6 +164,7 @@ nl: yank_rubygem: add_owner: remove_owner: + update_owner: access_webhooks: show_dashboard: configure_trusted_publishers: @@ -541,13 +542,22 @@ nl: confirmed: pending: confirm_remove: + role: + create_role: resend_confirmation: resent_notice: create: + invalid_role: success_notice: destroy: removed_notice: failed_notice: + edit: + role: + title: + update: + success_notice: + update_current_user: mfa_required: settings: edit: @@ -960,3 +970,7 @@ nl: api_key_gem_html: api_key_mfa: not_required: + access: + roles: + owner: + maintainer: diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml index 34a5748e8d5..a1cf08ef0b8 100644 --- a/config/locales/pt-BR.yml +++ b/config/locales/pt-BR.yml @@ -171,6 +171,7 @@ pt-BR: yank_rubygem: add_owner: remove_owner: + update_owner: access_webhooks: show_dashboard: configure_trusted_publishers: @@ -552,13 +553,22 @@ pt-BR: confirmed: pending: confirm_remove: + role: + create_role: resend_confirmation: resent_notice: create: + invalid_role: success_notice: destroy: removed_notice: failed_notice: + edit: + title: + role: + update: + update_current_user: + success_notice: mfa_required: settings: edit: @@ -983,3 +993,7 @@ pt-BR: api_key_gem_html: api_key_mfa: not_required: + access: + roles: + owner: + maintainer: diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml index 7bac93b6af7..fd815582979 100644 --- a/config/locales/zh-CN.yml +++ b/config/locales/zh-CN.yml @@ -166,6 +166,7 @@ zh-CN: yank_rubygem: 撤回 Gem add_owner: 添加 Gem 业主 remove_owner: 移除（某个）Gem 业主 + update_owner: access_webhooks: 访问后 (Access) Webhook show_dashboard: 显示仪表盘 configure_trusted_publishers: @@ -546,13 +547,22 @@ zh-CN: confirmed: 已批准 pending: 待定 confirm_remove: 您确定您想要从业主中移除该用户吗？ + role: + create_role: resend_confirmation: resent_notice: 一封确认邮件已被重新发送到您的邮箱中 create: + invalid_role: success_notice: "%{handle} 已添加为还未经批准的业主。在用户点击发送到其邮箱的批准邮件后，所有权访问才将被启用。" destroy: removed_notice: "%{owner_name} 已成功从业主中移除" failed_notice: 不能删除该 Gem 的唯一业主 + edit: + role: + title: + update: + success_notice: + update_current_user: mfa_required: 该 Gem 已启用多因素验证，请在您的帐户上设置多因素验证。 settings: edit: @@ -974,3 +984,7 @@ zh-CN: api_key_gem_html: api_key_mfa: not_required: + access: + roles: + owner: + maintainer: diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml index 3dca89fcae8..17d414006ee 100644 --- a/config/locales/zh-TW.yml +++ b/config/locales/zh-TW.yml @@ -165,6 +165,7 @@ zh-TW: yank_rubygem: 移除 rubygem add_owner: 新增擁有者 remove_owner: 移除擁有者 + update_owner: access_webhooks: 存取 Webhooks show_dashboard: 顯示儀表板 configure_trusted_publishers: @@ -541,13 +542,22 @@ zh-TW: confirmed: 已確認 pending: 待確認 confirm_remove: 您確定要將此使用者從擁有者名單中移除嗎？ + role: + create_role: resend_confirmation: resent_notice: create: + invalid_role: success_notice: "%{handle} 已以未確認擁有者的身分加入。所有權存取將在使用者點擊傳送到他們的電子郵件地址的確認信後啟用。" destroy: removed_notice: failed_notice: 無法移除 Gem 的唯一擁有者 + edit: + role: + title: + update: + update_current_user: + success_notice: mfa_required: Gem 啟用了 MFA 要求，請設定您的帳號的 MFA。 settings: edit: @@ -965,3 +975,7 @@ zh-TW: api_key_gem_html: api_key_mfa: not_required: + access: + roles: + owner: + maintainer: diff --git a/lib/access.rb b/lib/access.rb index 84e7923227f..3e53faae164 100644 --- a/lib/access.rb +++ b/lib/access.rb @@ -7,10 +7,10 @@ module Access ROLES = { maintainer: MAINTAINER, owner: OWNER - } + }.freeze def self.label_for_role(role) - key = ROLES.fetch(role.to_sym) { nil } + key = ROLES.fetch(role.to_sym, nil) return nil if key.nil? I18n.t("access.roles.#{role}") end @@ -22,9 +22,18 @@ def self.label_for_role_flag(flag) I18n.t("access.roles.#{role}") end + def self.permission_for_role(role) + raise ArgumentError, "A role must be provided" if role.blank? + ROLES.fetch(role&.to_sym, nil) + end + + def self.role_for_permission(permission) + ROLES.key(permission) + end + def self.options - ROLES.map do |_, permission| - [label_for_role_flag(permission), permission] + ROLES.map do |role, permission| + [label_for_role_flag(permission), role] end end end diff --git a/test/functional/api/v1/owners_controller_test.rb b/test/functional/api/v1/owners_controller_test.rb index 52556cd0950..0238e2d1244 100644 --- a/test/functional/api/v1/owners_controller_test.rb +++ b/test/functional/api/v1/owners_controller_test.rb @@ -929,12 +929,12 @@ def self.should_respond_to(format) end should "route PUT /api/v1/gems/rubygem/owners.yaml" do - route = { controller: "api/v1/owners", - action: "update", - rubygem_id: "rails", - format: "yaml" } + route = { controller: "api/v1/owners", + action: "update", + rubygem_id: "rails", + format: "yaml" } - assert_recognizes(route, path: "/api/v1/gems/rails/owners.yaml", method: :put) + assert_recognizes(route, path: "/api/v1/gems/rails/owners.yaml", method: :put) end context "on PATCH to owner gem" do diff --git a/test/functional/owners_controller_test.rb b/test/functional/owners_controller_test.rb index da4e250819c..61000fee001 100644 --- a/test/functional/owners_controller_test.rb +++ b/test/functional/owners_controller_test.rb @@ -52,7 +52,7 @@ class OwnersControllerTest < ActionController::TestCase context "with invalid handle" do setup do perform_enqueued_jobs only: ActionMailer::MailDeliveryJob do - post :create, params: { handle: "no_user", rubygem_id: @rubygem.name } + post :create, params: { handle: "no_user", rubygem_id: @rubygem.name, role: :owner } end end @@ -72,7 +72,7 @@ class OwnersControllerTest < ActionController::TestCase context "with valid handle" do setup do @new_owner = create(:user) - post :create, params: { handle: @new_owner.display_id, rubygem_id: @rubygem.name } + post :create, params: { handle: @new_owner.display_id, rubygem_id: @rubygem.name, role: :owner } end should redirect_to("ownerships index") { rubygem_owners_path(@rubygem.slug) } @@ -132,7 +132,7 @@ class OwnersControllerTest < ActionController::TestCase context "owner has enabled mfa" do setup do @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api) - post :create, params: { handle: @new_owner.display_id, rubygem_id: @rubygem.name } + post :create, params: { handle: @new_owner.display_id, rubygem_id: @rubygem.name, role: :owner } end should redirect_to("ownerships index") { rubygem_owners_path(@rubygem.slug) } @@ -367,7 +367,7 @@ class OwnersControllerTest < ActionController::TestCase @rubygem = create(:rubygem, owners: [@owner, @maintainer]) verified_sign_in_as(@owner) - patch :update, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id, ownership: { access_level: Access::MAINTAINER } } + patch :update, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id, role: :maintainer } end should redirect_to("rubygem show") { rubygem_owners_path(@rubygem.slug) } @@ -379,22 +379,27 @@ class OwnersControllerTest < ActionController::TestCase should "downgrade the maintainer" do owner = @rubygem.ownerships.find_by(user_id: @owner.id) + assert_equal Access::OWNER, owner.access_level end + end - context "when updating the current user" do - setup do - patch :update, params: { rubygem_id: @rubygem.name, handle: @owner.display_id, access_level: Access::OWNER } - end + context "on PATCH to update ownership of currently signed in user" do + setup do + @owner = create(:user) + @rubygem = create(:rubygem) + @ownership = create(:ownership, user: @owner, rubygem: @rubygem, access_level: Access::OWNER) - should "not update the ownership of the current user" do - owner = @rubygem.ownerships.find_by(user_id: @owner.id) - assert_equal Access::OWNER, owner.access_level - end + verified_sign_in_as(@owner) + patch :update, params: { rubygem_id: @rubygem.name, handle: @owner.display_id, role: :maintainer } + end - should "set notice flash message" do - assert_equal "You can't update your own access level.", flash[:notice] - end + should "not update the ownership of the current user" do + assert_equal Access::OWNER, @ownership.reload.access_level + end + + should "set notice flash message" do + assert_equal "You can't update your own access level.", flash[:alert] end end end @@ -419,7 +424,7 @@ class OwnersControllerTest < ActionController::TestCase context "on POST to create ownership" do setup do @new_owner = create(:user) - post :create, params: { handle: @new_owner.display_id, rubygem_id: @rubygem.name } + post :create, params: { handle: @new_owner.display_id, rubygem_id: @rubygem.name, role: :owner } end should redirect_to("sessions#verify") { verify_session_path } @@ -433,7 +438,7 @@ class OwnersControllerTest < ActionController::TestCase context "on DELETE to owners" do setup do @second_user = create(:user) - @ownership = create(:ownership, :unconfirmed, rubygem: @rubygem, user: @second_user) + @ownership = create(:ownership, rubygem: @rubygem, user: @second_user) delete :destroy, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id } end should redirect_to("sessions#verify") { verify_session_path } @@ -463,7 +468,7 @@ class OwnersControllerTest < ActionController::TestCase setup do @second_user = create(:user) @ownership = create(:ownership, :unconfirmed, rubygem: @rubygem, user: @second_user) - patch :update, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id, access_level: Access::MAINTAINER } + patch :update, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id, role: :owner } should "update the ownership record" do assert_equal Access::MAINTAINER, @ownership.reload.access @@ -580,7 +585,7 @@ class OwnersControllerTest < ActionController::TestCase context "on PATCH to update owner" do setup do create(:ownership, rubygem: @rubygem, user: @user) - patch :update, params: { rubygem_id: @rubygem.name, handle: @user.display_id, access_level: Access::OWNER } + patch :update, params: { rubygem_id: @rubygem.name, handle: @user.display_id, role: :owner } end should "redirect to sign in path" do diff --git a/test/integration/owner_test.rb b/test/integration/owner_test.rb index 8a4a26a451e..dded2960177 100644 --- a/test/integration/owner_test.rb +++ b/test/integration/owner_test.rb @@ -266,7 +266,7 @@ class OwnerTest < SystemTest click_link "Edit" end - select "Maintainer", from: "ownership_access_level" + select "Maintainer", from: "role" click_button "Update" @@ -279,7 +279,7 @@ class OwnerTest < SystemTest visit_ownerships_page fill_in "Handle", with: maintainer.handle - select "Maintainer", from: "access_level" + select "Maintainer", from: "role" click_button "Add Owner" diff --git a/test/lib/access_test.rb b/test/lib/access_test.rb index 03877438e1f..0910c8ba2c6 100644 --- a/test/lib/access_test.rb +++ b/test/lib/access_test.rb @@ -15,6 +15,16 @@ class AccessTest < ActiveSupport::TestCase end end + context ".role_for_permission" do + should "return the role for a given permission flag" do + assert_equal :owner, Access.role_for_permission(Access::OWNER) + end + + should "when the permission flag does not exist" do + assert_nil Access.role_for_permission(999) + end + end + context ".label_for_role_flag" do should "return the label for the role flag" do assert_equal "Owner", Access.label_for_role_flag(Access::OWNER) @@ -32,4 +42,10 @@ class AccessTest < ActiveSupport::TestCase end end end + + context ".options" do + should "return an array of options" do + assert_equal [["Maintainer", :maintainer], ["Owner", :owner]], Access.options + end + end end diff --git a/test/policies/rubygem_policy_test.rb b/test/policies/rubygem_policy_test.rb index deca04c2a1b..40b29b2041e 100644 --- a/test/policies/rubygem_policy_test.rb +++ b/test/policies/rubygem_policy_test.rb @@ -61,6 +61,14 @@ def policy!(user) end end + context "#close_ownership_requests" do + should "only allow the owner to close ownership requests" do + assert_authorized @owner, :close_ownership_requests? + refute_authorized @maintainer, :close_ownership_requests? + refute_authorized @user, :close_ownership_requests? + end + end + context "#show_adoption?" do should "be true if the gem is owned by the user" do assert_authorized @owner, :show_adoption? @@ -86,7 +94,7 @@ def policy!(user) context "#configure_trusted_publishers?" do should "only allow the owner" do assert_authorized @owner, :configure_trusted_publishers? - assert_authorized @maintainer, :configure_trusted_publishers? + refute_authorized @maintainer, :configure_trusted_publishers? refute_authorized @user, :configure_trusted_publishers? refute_authorized nil, :configure_trusted_publishers? end @@ -95,7 +103,7 @@ def policy!(user) context "#show_unconfirmed_ownerships?" do should "only allow the owner" do assert_authorized @owner, :show_unconfirmed_ownerships? - assert_authorized @maintainer, :show_unconfirmed_ownerships? + refute_authorized @maintainer, :show_unconfirmed_ownerships? refute_authorized @user, :show_unconfirmed_ownerships? refute_authorized nil, :show_unconfirmed_ownerships? end diff --git a/test/system/avo/rubygems_test.rb b/test/system/avo/rubygems_test.rb index b79ccc0ee3f..d29f6c2e17f 100644 --- a/test/system/avo/rubygems_test.rb +++ b/test/system/avo/rubygems_test.rb @@ -314,7 +314,8 @@ def sign_in_as(user) "token_expires_at" => [nil, ownership.token_expires_at.as_json], "owner_notifier" => [nil, true], "authorizer_id" => [nil, security_user.id], - "ownership_request_notifier" => [nil, true] + "ownership_request_notifier" => [nil, true], + "access_level" => [nil, 70] }, "unchanged" => {} }, From 18d4f26b85d0ad99469d0e51878b6cf552244e07 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:42:33 +1000 Subject: [PATCH 05/28] better validate the role in Owners API endpoint --- app/avo/resources/ownership_resource.rb | 2 +- app/controllers/api/v1/owners_controller.rb | 23 ++++++- app/controllers/owners_controller.rb | 2 +- app/models/ownership.rb | 4 ++ lib/access.rb | 12 ++-- .../api/v1/owners_controller_test.rb | 62 +++++++++++++++++-- test/models/ownership_test.rb | 10 +++ 7 files changed, 102 insertions(+), 13 deletions(-) diff --git a/app/avo/resources/ownership_resource.rb b/app/avo/resources/ownership_resource.rb index c9d24d89a09..d319eb6d07f 100644 --- a/app/avo/resources/ownership_resource.rb +++ b/app/avo/resources/ownership_resource.rb @@ -26,7 +26,7 @@ class ConfirmedFilter < ScopeBooleanFilter; end field :authorizer, as: :belongs_to field :confirmed_at, as: :date_time - field :access_level, as: :text do |model, _resource, _view| + field :role, as: :text do |model, _resource, _view| Access.label_for_role_flag(model.access_level) end end diff --git a/app/controllers/api/v1/owners_controller.rb b/app/controllers/api/v1/owners_controller.rb index 72f19be1f71..a804a86c0a0 100644 --- a/app/controllers/api/v1/owners_controller.rb +++ b/app/controllers/api/v1/owners_controller.rb @@ -16,6 +16,17 @@ def create authorize @rubygem, :add_owner? owner = User.find_by_name!(email_param) ownership = @rubygem.ownerships.new(user: owner, authorizer: @api_key.user) + + if params[:role].present? + access_level = Access.permission_for_role(params[:role]) + if access_level.blank? + return render plain: response_with_mfa_warning("Invalid role. Role must be: #{Access.roles.to_sentence(two_words_connector: ' or ')}"), +status: :unprocessable_entity + else + ownership.access_level = access_level + end + end + if ownership.save OwnersMailer.ownership_confirmation(ownership).deliver_later render plain: response_with_mfa_warning("#{owner.display_handle} was added as an unconfirmed owner. " \ @@ -30,8 +41,16 @@ def update authorize @rubygem, :update_owner? ownership = @rubygem.ownerships.find_by!(user: User.find_by_name!(email_param)) + if params[:role].present? + access_level = Access.permission_for_role(ownership_update_params[:role]) + if access_level.blank? + return render plain: response_with_mfa_warning("Invalid role. Role must be: #{Access.roles.to_sentence(two_words_connector: ' or ')}"), +status: :unprocessable_entity + end + end + if ownership.present? - ownership.update!(ownership_update_params) + ownership.update!(access_level: access_level) render plain: response_with_mfa_warning("Owner updated successfully.") else render plain: response_with_mfa_warning("Unable to update owner."), status: :forbidden @@ -74,6 +93,6 @@ def email_param end def ownership_update_params - params.permit(:access_level) + params.permit(:role) end end diff --git a/app/controllers/owners_controller.rb b/app/controllers/owners_controller.rb index f6f8bc968d8..1ef1e6db882 100644 --- a/app/controllers/owners_controller.rb +++ b/app/controllers/owners_controller.rb @@ -41,7 +41,7 @@ def create authorize @rubygem, :add_owner? owner = User.find_by_name(handle_params) access_level = Access.permission_for_role(params[:role]) - return redirect_to rubygem_owners_path(@rubygem.slug), alert: t(".invalid_access_level") if access_level.blank? + return redirect_to rubygem_owners_path(@rubygem.slug), alert: t(".invalid_role") if access_level.blank? ownership = @rubygem.ownerships.new(user: owner, authorizer: current_user, access_level: access_level) if ownership.save diff --git a/app/models/ownership.rb b/app/models/ownership.rb index fd6e4b94d1a..97b374d4f59 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -89,6 +89,10 @@ def validate_unique_user end end + def role + Access.role_for_permission(access_level) + end + private def record_create_event diff --git a/lib/access.rb b/lib/access.rb index 3e53faae164..f7b9fce4a3e 100644 --- a/lib/access.rb +++ b/lib/access.rb @@ -5,10 +5,14 @@ module Access OWNER = 70 ROLES = { - maintainer: MAINTAINER, - owner: OWNER + "maintainer" => MAINTAINER, + "owner" => OWNER }.freeze + def self.roles + ROLES.keys + end + def self.label_for_role(role) key = ROLES.fetch(role.to_sym, nil) return nil if key.nil? @@ -24,11 +28,11 @@ def self.label_for_role_flag(flag) def self.permission_for_role(role) raise ArgumentError, "A role must be provided" if role.blank? - ROLES.fetch(role&.to_sym, nil) + ROLES.fetch(String(role), nil) end def self.role_for_permission(permission) - ROLES.key(permission) + ROLES.key(permission).inquiry end def self.options diff --git a/test/functional/api/v1/owners_controller_test.rb b/test/functional/api/v1/owners_controller_test.rb index 0238e2d1244..36e00895a70 100644 --- a/test/functional/api/v1/owners_controller_test.rb +++ b/test/functional/api/v1/owners_controller_test.rb @@ -518,6 +518,39 @@ def self.should_respond_to(format) end end end + + context "when not supplying a role" do + should "set a default role" do + post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.display_id } + + assert_equal 200, @response.status + assert_predicate Ownership.find_by(user: @second_user, rubygem: @rubygem).role, :owner? + end + end + + context "given a role" do + should "set the role for the given user" do + post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.display_id, role: :maintainer } + + assert_equal 200, @response.status + assert_predicate Ownership.find_by(user: @second_user, rubygem: @rubygem).role, :maintainer? + end + end + + context "when given an invalid role" do + should "raise an error" do + post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.display_id, role: :invalid } + + assert_equal 422, @response.status + assert_equal "Invalid role. Role must be: maintainer or owner", @response.body + end + + should "not create the ownership" do + post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.email, role: :invalid } + + assert_nil @rubygem.ownerships.find_by(user: @second_user) + end + end end context "without add owner api key scope" do @@ -939,19 +972,38 @@ def self.should_respond_to(format) context "on PATCH to owner gem" do setup do - @user = create(:user) + @owner = create(:user) @maintainer = create(:user) - @rubygem = create(:rubygem, owners: [@user, @maintainer]) + @rubygem = create(:rubygem) - @api_key = create(:api_key, key: "12223", scopes: %i[update_owner], owner: @user, rubygem: @rubygem) + @owner_gem_ownership = create(:ownership, user: @owner, rubygem: @rubygem, access_level: Access::OWNER) + @maintainer_gem_ownership = create(:ownership, user: @maintainer, rubygem: @rubygem, access_level: Access::MAINTAINER) + + @api_key = create(:api_key, key: "12223", scopes: %i[update_owner], owner: @owner, rubygem: @rubygem) @request.env["HTTP_AUTHORIZATION"] = "12223" end should "set the maintainer to a lower access level" do - patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, access_level: Access::MAINTAINER } + patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :maintainer } assert_response :success - assert_equal Access::MAINTAINER, @maintainer.reload.ownerships.find_by(rubygem: @rubygem).access_level + assert_equal Access::MAINTAINER, @maintainer_gem_ownership.reload.access_level + end + + context "when the role is invalid" do + should "return a bad request response with the error message" do + patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid } + + respond_with :bad_request + + assert_equal "Invalid role. Role must be: maintainer or owner", @response.body + end + + should "not update the user with the new role" do + patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid } + + assert_equal Access::MAINTAINER, @maintainer_gem_ownership.reload.access_level + end end end end diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index 1b3e5f1d6c5..1f6039248b7 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -248,4 +248,14 @@ class OwnershipTest < ActiveSupport::TestCase refute_predicate @ownership, :unconfirmed? end end + + context "#role" do + setup do + @ownership = create(:ownership) + end + + should "maps the ownership permission level to the role" do + assert_predicate @ownership.role, :owner? + end + end end From 10d3de0b59fc40bf3915416fd0ddf46fbb261aeb Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Thu, 8 Aug 2024 00:39:53 +1000 Subject: [PATCH 06/28] Refactor Access Level field to be Role instead --- app/avo/resources/ownership_resource.rb | 4 +- app/controllers/api/v1/owners_controller.rb | 26 ++------ app/controllers/owners_controller.rb | 22 ++++--- app/models/ownership.rb | 6 +- app/models/rubygem.rb | 4 +- app/policies/application_policy.rb | 4 +- app/policies/rubygem_policy.rb | 8 +-- app/views/owners/_owners_table.html.erb | 8 ++- app/views/owners/edit.html.erb | 2 +- app/views/owners/index.html.erb | 4 +- config/brakeman.ignore | 52 +++++++++++++++ config/initializers/active_model_types.rb | 1 + config/locales/de.yml | 5 +- config/locales/en.yml | 5 +- config/locales/es.yml | 5 +- config/locales/fr.yml | 5 +- config/locales/ja.yml | 5 +- config/locales/nl.yml | 5 +- config/locales/pt-BR.yml | 5 +- config/locales/zh-CN.yml | 5 +- config/locales/zh-TW.yml | 5 +- ...802151324_add_access_level_to_ownership.rb | 2 +- db/schema.rb | 2 +- lib/access.rb | 18 +++--- lib/types/role.rb | 16 +++++ test/factories/ownership.rb | 4 +- test/factories/rubygem.rb | 4 +- .../api/v1/owners_controller_test.rb | 12 ++-- test/functional/owners_controller_test.rb | 63 ++++++++++++++++--- test/integration/owner_test.rb | 2 +- test/lib/access_test.rb | 42 ++++++++----- test/models/ownership_test.rb | 7 +++ test/system/avo/rubygems_test.rb | 2 +- 33 files changed, 240 insertions(+), 120 deletions(-) create mode 100644 config/brakeman.ignore create mode 100644 lib/types/role.rb diff --git a/app/avo/resources/ownership_resource.rb b/app/avo/resources/ownership_resource.rb index d319eb6d07f..26d4b87ff18 100644 --- a/app/avo/resources/ownership_resource.rb +++ b/app/avo/resources/ownership_resource.rb @@ -26,7 +26,5 @@ class ConfirmedFilter < ScopeBooleanFilter; end field :authorizer, as: :belongs_to field :confirmed_at, as: :date_time - field :role, as: :text do |model, _resource, _view| - Access.label_for_role_flag(model.access_level) - end + field :role, as: :text end diff --git a/app/controllers/api/v1/owners_controller.rb b/app/controllers/api/v1/owners_controller.rb index a804a86c0a0..6192ac614d9 100644 --- a/app/controllers/api/v1/owners_controller.rb +++ b/app/controllers/api/v1/owners_controller.rb @@ -15,17 +15,9 @@ def show def create authorize @rubygem, :add_owner? owner = User.find_by_name!(email_param) + role = params[:role] ownership = @rubygem.ownerships.new(user: owner, authorizer: @api_key.user) - - if params[:role].present? - access_level = Access.permission_for_role(params[:role]) - if access_level.blank? - return render plain: response_with_mfa_warning("Invalid role. Role must be: #{Access.roles.to_sentence(two_words_connector: ' or ')}"), -status: :unprocessable_entity - else - ownership.access_level = access_level - end - end + ownership.role = role if role.present? if ownership.save OwnersMailer.ownership_confirmation(ownership).deliver_later @@ -40,20 +32,12 @@ def create def update authorize @rubygem, :update_owner? ownership = @rubygem.ownerships.find_by!(user: User.find_by_name!(email_param)) + ownership.role = params[:role] if params[:role].present? - if params[:role].present? - access_level = Access.permission_for_role(ownership_update_params[:role]) - if access_level.blank? - return render plain: response_with_mfa_warning("Invalid role. Role must be: #{Access.roles.to_sentence(two_words_connector: ' or ')}"), -status: :unprocessable_entity - end - end - - if ownership.present? - ownership.update!(access_level: access_level) + if ownership.save render plain: response_with_mfa_warning("Owner updated successfully.") else - render plain: response_with_mfa_warning("Unable to update owner."), status: :forbidden + render plain: response_with_mfa_warning(ownership.errors.full_messages.to_sentence), status: :unprocessable_entity end end diff --git a/app/controllers/owners_controller.rb b/app/controllers/owners_controller.rb index 1ef1e6db882..e5aaf1940ce 100644 --- a/app/controllers/owners_controller.rb +++ b/app/controllers/owners_controller.rb @@ -40,10 +40,8 @@ def edit def create authorize @rubygem, :add_owner? owner = User.find_by_name(handle_params) - access_level = Access.permission_for_role(params[:role]) - return redirect_to rubygem_owners_path(@rubygem.slug), alert: t(".invalid_role") if access_level.blank? - ownership = @rubygem.ownerships.new(user: owner, authorizer: current_user, access_level: access_level) + ownership = @rubygem.ownerships.new(user: owner, authorizer: current_user, role: params[:role]) if ownership.save OwnersMailer.ownership_confirmation(ownership).deliver_later redirect_to rubygem_owners_path(@rubygem.slug), notice: t(".success_notice", handle: owner.name) @@ -52,18 +50,22 @@ def create end end + # This action is used to update a user's owenrship role. This endpoint currently asssumes + # the role is the only thing that can be updated. If more fields are added to the ownership + # this action will need to be tweaked a bit def update authorize @rubygem, :update_owner? ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params) - # Don't allow the owner to change the access level of their own ownership - return redirect_to rubygem_owners_path(@rubygem.slug), alert: t(".update_current_user") if ownership.user == current_user + if ownership.user == current_user && update_params[:role] != ownership.role + # Don't allow the owner to change the access level of their own ownership + return redirect_to rubygem_owners_path(@rubygem.slug), alert: t(".update_current_user_role") + end - access_level = Access.permission_for_role(params[:role]) - if ownership.update({ access_level: access_level }) + if ownership.update(update_params) redirect_to rubygem_owners_path(ownership.rubygem.slug), notice: t(".success_notice", handle: ownership.user.name) else - index_with_error @ownership.errors.full_messages.to_sentence, :unprocessable_entity + index_with_error ownership.errors.full_messages.to_sentence, :unprocessable_entity end end @@ -92,6 +94,10 @@ def handle_params params.permit(:handle).require(:handle) end + def update_params + params.permit(:role) + end + def notify_owner_added(ownership) ownership.rubygem.ownership_notifiable_owners.each do |notified_user| OwnersMailer.owner_added( diff --git a/app/models/ownership.rb b/app/models/ownership.rb index 97b374d4f59..823ffdb404f 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -4,7 +4,9 @@ class Ownership < ApplicationRecord belongs_to :authorizer, class_name: "User" has_many :api_key_rubygem_scopes, dependent: :destroy + attribute :role, Types::Role.new, default: Access::DEFAULT_ROLE validate :validate_unique_user + validates :role, inclusion: { in: Access.roles }, allow_nil: true delegate :name, to: :user, prefix: :owner delegate :name, to: :authorizer, prefix: true, allow_nil: true @@ -89,10 +91,6 @@ def validate_unique_user end end - def role - Access.role_for_permission(access_level) - end - private def record_create_event diff --git a/app/models/rubygem.rb b/app/models/rubygem.rb index 071dab4602d..387350f27f8 100644 --- a/app/models/rubygem.rb +++ b/app/models/rubygem.rb @@ -188,9 +188,9 @@ def owned_by?(user) ownerships.exists?(user_id: user.id) end - def owned_by_with_access_level?(user, access_level) + def owned_by_with_role?(user, minimum_required_role) return false if user.blank? - ownerships.exists?(["user_id = ? AND access_level >= ?", user.id, access_level]) + ownerships.exists?(["user_id = ? AND role >= ?", user.id, minimum_required_role]) end def unconfirmed_ownerships diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 86f6e27a55d..910a177cf82 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -76,8 +76,8 @@ def current_user?(record_user) user && user == record_user end - def rubygem_owned_by?(user, required_access_level: Access::MAINTAINER) - rubygem.owned_by_with_access_level?(user, required_access_level) || deny(t(:forbidden)) + def rubygem_owned_by?(user, minimum_required_role: Access::MAINTAINER) + rubygem.owned_by_with_role?(user, minimum_required_role) || deny(t(:forbidden)) end def policy!(user, record) = Pundit.policy!(user, record) diff --git a/app/policies/rubygem_policy.rb b/app/policies/rubygem_policy.rb index 32508104aa2..0a858a85490 100644 --- a/app/policies/rubygem_policy.rb +++ b/app/policies/rubygem_policy.rb @@ -55,18 +55,18 @@ def close_ownership_requests? end def show_unconfirmed_ownerships? - rubygem_owned_by?(user, required_access_level: Access::OWNER) + rubygem_owned_by?(user, minimum_required_role: Access::OWNER) end def add_owner? - rubygem_owned_by?(user, required_access_level: Access::OWNER) + rubygem_owned_by?(user, minimum_required_role: Access::OWNER) end def update_owner? - rubygem_owned_by?(user, required_access_level: Access::OWNER) + rubygem_owned_by?(user, minimum_required_role: Access::OWNER) end def remove_owner? - rubygem_owned_by?(user, required_access_level: Access::OWNER) + rubygem_owned_by?(user, minimum_required_role: Access::OWNER) end end diff --git a/app/views/owners/_owners_table.html.erb b/app/views/owners/_owners_table.html.erb index fe90e4979ac..1fc610f6912 100644 --- a/app/views/owners/_owners_table.html.erb +++ b/app/views/owners/_owners_table.html.erb @@ -44,7 +44,7 @@ <%= ownership.authorizer_name %> - <%= Access.label_for_role_flag(ownership.access_level) %> + <%= Access.label_for_role(ownership.role) %> <%= ownership.confirmed_at.strftime("%Y-%m-%d %H:%M %Z") if ownership.confirmed? %> @@ -55,7 +55,11 @@ method: "delete", data: { confirm: t("owners.index.confirm_remove") }, class: "form__submit form__submit--small" %> - <%= link_to "Edit", edit_rubygem_owner_path(@rubygem.name, ownership.user.display_id), class: "form__submit form__submit--small" %> +
+ <%= button_to "Edit", + edit_rubygem_owner_path(@rubygem.name, ownership.user.display_id), + method: "get", + class: "form__submit form__submit--small" %> <% end %> diff --git a/app/views/owners/edit.html.erb b/app/views/owners/edit.html.erb index 7233f196269..e19b1efa69b 100644 --- a/app/views/owners/edit.html.erb +++ b/app/views/owners/edit.html.erb @@ -11,7 +11,7 @@
<%= label_tag :role, t(".role"), class: 'form__label' %>
- <%= select_tag :role, options_for_select(Access.options.to_a.reverse, Access.role_for_permission(@ownership.access_level)) %> + <%= select_tag :role, options_for_select(Access.options.to_a.reverse, @ownership.role), class: "form__input form__select" %>
diff --git a/app/views/owners/index.html.erb b/app/views/owners/index.html.erb index b641ff371d7..43f54200e70 100644 --- a/app/views/owners/index.html.erb +++ b/app/views/owners/index.html.erb @@ -15,9 +15,9 @@ <%= text_field_tag :handle, nil, class: "form__input", required: true %>
- <%= label_tag :role, t(".create_role"), class: "form__label" %> + <%= label_tag :role, t(".role_field"), class: "form__label" %>
- <%= select_tag :role, options_for_select(Access.options.to_a.reverse) %> + <%= select_tag :role, options_for_select(Access.options.to_a.reverse), class: "form__input form__select" %>
<%= submit_tag t(".submit_button"), data: {disable_with: t("form_disable_with")}, class: "form__submit" %> diff --git a/config/brakeman.ignore b/config/brakeman.ignore new file mode 100644 index 00000000000..8fa1ee71005 --- /dev/null +++ b/config/brakeman.ignore @@ -0,0 +1,52 @@ +{ + "ignored_warnings": [ + { + "warning_type": "Mass Assignment", + "warning_code": 105, + "fingerprint": "8129b3f5dfead604b3d9f72a38e52ffe0a7273b2fa5cccc5e75c6066fddf3934", + "check_name": "PermitAttributes", + "message": "Potentially dangerous key allowed for mass assignment", + "file": "app/controllers/api/v1/owners_controller.rb", + "line": 80, + "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/", + "code": "params.permit(:role)", + "render_path": null, + "location": { + "type": "method", + "class": "Api::V1::OwnersController", + "method": "ownership_update_params" + }, + "user_input": ":role", + "confidence": "Medium", + "cwe_id": [ + 915 + ], + "note": "" + }, + { + "warning_type": "Mass Assignment", + "warning_code": 105, + "fingerprint": "85e397bacae5462238e8ce59c0fcd1045a414e603588ae313c5156c09a623fed", + "check_name": "PermitAttributes", + "message": "Potentially dangerous key allowed for mass assignment", + "file": "app/controllers/owners_controller.rb", + "line": 98, + "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/", + "code": "params.permit(:role)", + "render_path": null, + "location": { + "type": "method", + "class": "OwnersController", + "method": "update_params" + }, + "user_input": ":role", + "confidence": "Medium", + "cwe_id": [ + 915 + ], + "note": "" + } + ], + "updated": "2024-08-08 12:28:27 +1000", + "brakeman_version": "6.1.2" +} diff --git a/config/initializers/active_model_types.rb b/config/initializers/active_model_types.rb index 6f1a5ab64c3..9ecd31ce279 100644 --- a/config/initializers/active_model_types.rb +++ b/config/initializers/active_model_types.rb @@ -1,4 +1,5 @@ ActiveSupport.on_load(:active_model) do ActiveModel::Type.register(:global_id, Types::GlobalId) ActiveModel::Type.register(:duration, Types::Duration) + ActiveModel::Type.register(:role, Types::Role) end diff --git a/config/locales/de.yml b/config/locales/de.yml index e03965a0663..e7b0f1de694 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -638,19 +638,18 @@ de: pending: confirm_remove: role: - create_role: + role_field: resend_confirmation: resent_notice: create: success_notice: - invalid_role: destroy: removed_notice: failed_notice: mfa_required: update: success_notice: - update_current_user: + update_current_user_role: edit: role: title: diff --git a/config/locales/en.yml b/config/locales/en.yml index 5be9890941f..65c8d9ce899 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -548,7 +548,7 @@ en: added_by: ADDED BY action: ACTION role: ROLE - create_role: Role + role_field: Role email_field: Email / Handle submit_button: Add Owner info: add or remove owners @@ -559,9 +559,8 @@ en: resent_notice: A confirmation mail has been re-sent to your email create: success_notice: "%{handle} was added as an unconfirmed owner. Ownership access will be enabled after the user clicks on the confirmation mail sent to their email." - invalid_role: "Incorrect role. Please select from the dropdown" update: - update_current_user: You can't update your own access level + update_current_user_role: Can't update your own Role success_notice: "%{handle} was succesfully updated." destroy: removed_notice: "%{owner_name} was removed from the owners successfully" diff --git a/config/locales/es.yml b/config/locales/es.yml index a17c737b423..9a11da17a93 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -624,11 +624,10 @@ es: pending: Pendiente confirm_remove: "¿Seguro que quieres eliminar a este usuario de los propietarios?" role: - create_role: + role_field: resend_confirmation: resent_notice: Se ha reenviado un mensaje de confirmación a tu correo electrónico create: - invalid_role: success_notice: Se ha añadido a %{handle} como propietario sin confirmar. Su acceso como propietario se activará cuando haga click en el mensaje de confirmación que se le ha enviado a su correo @@ -641,7 +640,7 @@ es: role: title: update: - update_current_user: + update_current_user_role: success_notice: settings: edit: diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 7aa7d703603..2656b7b3550 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -562,17 +562,16 @@ fr: pending: confirm_remove: role: - create_role: + role_field: resend_confirmation: resent_notice: create: - invalid_role: success_notice: destroy: removed_notice: failed_notice: update: - update_current_user: + update_current_user_role: success_notice: edit: role: diff --git a/config/locales/ja.yml b/config/locales/ja.yml index 5c7653e5afb..174f6d06811 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -542,11 +542,10 @@ ja: pending: 保留 confirm_remove: このユーザーを所有者から削除されたいとのことでよろしいですか。 role: - create_role: + role_field: resend_confirmation: resent_notice: 確定メールがEメールに再送されました。 create: - invalid_role: success_notice: "%{handle}が未確定の所有者として追加されました。当該ユーザーがEメールに送られた確定メールでクリックした後に所有権アクセスが有効になります。" destroy: removed_notice: "%{owner_name}は所有者から正常に削除されました" @@ -555,7 +554,7 @@ ja: title: role: update: - update_current_user: + update_current_user_role: success_notice: mfa_required: gemでMFAの要件が有効化されているため、アカウントにMFAを設定してください。 settings: diff --git a/config/locales/nl.yml b/config/locales/nl.yml index 29e265f37ac..8d081f2f09c 100644 --- a/config/locales/nl.yml +++ b/config/locales/nl.yml @@ -543,11 +543,10 @@ nl: pending: confirm_remove: role: - create_role: + role_field: resend_confirmation: resent_notice: create: - invalid_role: success_notice: destroy: removed_notice: @@ -557,7 +556,7 @@ nl: title: update: success_notice: - update_current_user: + update_current_user_role: mfa_required: settings: edit: diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml index a1cf08ef0b8..b9e7b08cfa7 100644 --- a/config/locales/pt-BR.yml +++ b/config/locales/pt-BR.yml @@ -554,11 +554,10 @@ pt-BR: pending: confirm_remove: role: - create_role: + role_field: resend_confirmation: resent_notice: create: - invalid_role: success_notice: destroy: removed_notice: @@ -567,7 +566,7 @@ pt-BR: title: role: update: - update_current_user: + update_current_user_role: success_notice: mfa_required: settings: diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml index fd815582979..9d19e4eb52e 100644 --- a/config/locales/zh-CN.yml +++ b/config/locales/zh-CN.yml @@ -548,11 +548,10 @@ zh-CN: pending: 待定 confirm_remove: 您确定您想要从业主中移除该用户吗？ role: - create_role: + role_field: resend_confirmation: resent_notice: 一封确认邮件已被重新发送到您的邮箱中 create: - invalid_role: success_notice: "%{handle} 已添加为还未经批准的业主。在用户点击发送到其邮箱的批准邮件后，所有权访问才将被启用。" destroy: removed_notice: "%{owner_name} 已成功从业主中移除" @@ -562,7 +561,7 @@ zh-CN: title: update: success_notice: - update_current_user: + update_current_user_role: mfa_required: 该 Gem 已启用多因素验证，请在您的帐户上设置多因素验证。 settings: edit: diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml index 17d414006ee..21246d34e4a 100644 --- a/config/locales/zh-TW.yml +++ b/config/locales/zh-TW.yml @@ -543,11 +543,10 @@ zh-TW: pending: 待確認 confirm_remove: 您確定要將此使用者從擁有者名單中移除嗎？ role: - create_role: + role_field: resend_confirmation: resent_notice: create: - invalid_role: success_notice: "%{handle} 已以未確認擁有者的身分加入。所有權存取將在使用者點擊傳送到他們的電子郵件地址的確認信後啟用。" destroy: removed_notice: @@ -556,7 +555,7 @@ zh-TW: role: title: update: - update_current_user: + update_current_user_role: success_notice: mfa_required: Gem 啟用了 MFA 要求，請設定您的帳號的 MFA。 settings: diff --git a/db/migrate/20240802151324_add_access_level_to_ownership.rb b/db/migrate/20240802151324_add_access_level_to_ownership.rb index b858b8cc8cb..1b4e13189b1 100644 --- a/db/migrate/20240802151324_add_access_level_to_ownership.rb +++ b/db/migrate/20240802151324_add_access_level_to_ownership.rb @@ -1,5 +1,5 @@ class AddAccessLevelToOwnership < ActiveRecord::Migration[7.1] def change - add_column :ownerships, :access_level, :integer, null: false, default: 70 # Access::OWNER + add_column :ownerships, :role, :integer, null: false, default: 70 # Access::OWNER end end diff --git a/db/schema.rb b/db/schema.rb index 0ff0b89b477..63f31935d1d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -455,7 +455,7 @@ t.boolean "owner_notifier", default: true, null: false t.integer "authorizer_id" t.boolean "ownership_request_notifier", default: true, null: false - t.integer "access_level", default: 70, null: false + t.integer "role", default: 70, null: false t.index ["rubygem_id"], name: "index_ownerships_on_rubygem_id" t.index ["user_id", "rubygem_id"], name: "index_ownerships_on_user_id_and_rubygem_id", unique: true end diff --git a/lib/access.rb b/lib/access.rb index f7b9fce4a3e..0a5b6c5066f 100644 --- a/lib/access.rb +++ b/lib/access.rb @@ -4,6 +4,8 @@ module Access MAINTAINER = 50 OWNER = 70 + DEFAULT_ROLE = "owner".freeze + ROLES = { "maintainer" => MAINTAINER, "owner" => OWNER @@ -14,30 +16,28 @@ def self.roles end def self.label_for_role(role) - key = ROLES.fetch(role.to_sym, nil) + key = ROLES.fetch(role.to_s, nil) return nil if key.nil? I18n.t("access.roles.#{role}") end def self.label_for_role_flag(flag) - raise ArgumentError, "flag must be an integer" unless flag.is_a?(Integer) role = ROLES.key(flag) { nil } return nil if role.nil? I18n.t("access.roles.#{role}") end - def self.permission_for_role(role) - raise ArgumentError, "A role must be provided" if role.blank? - ROLES.fetch(String(role), nil) + def self.flag_for_role(role) + ROLES.fetch(role.to_s, nil) end - def self.role_for_permission(permission) - ROLES.key(permission).inquiry + def self.role_for_flag(flag) + ROLES.key(flag)&.inquiry end def self.options - ROLES.map do |role, permission| - [label_for_role_flag(permission), role] + ROLES.map do |role, flag| + [label_for_role_flag(flag), role] end end end diff --git a/lib/types/role.rb b/lib/types/role.rb new file mode 100644 index 00000000000..df23141c7cd --- /dev/null +++ b/lib/types/role.rb @@ -0,0 +1,16 @@ +class Types::Role < ActiveRecord::Type::Integer + def cast(value) + return nil if value.blank? + value.to_s + end + + def serialize(value) + return nil if value.blank? + Access.flag_for_role(value) + end + + def deserialize(value) + return nil if value.blank? + Access.role_for_flag(value) + end +end diff --git a/test/factories/ownership.rb b/test/factories/ownership.rb index c4546691905..9931e2754e2 100644 --- a/test/factories/ownership.rb +++ b/test/factories/ownership.rb @@ -4,14 +4,14 @@ user confirmed_at { Time.current } authorizer { association :user } - access_level { Access::OWNER } + role { :owner } trait :unconfirmed do confirmed_at { nil } end trait :maintainer do - access_level { Access::MAINTAINER } + role { :maintainer } end end end diff --git a/test/factories/rubygem.rb b/test/factories/rubygem.rb index fd877c4f5bb..285e833a735 100644 --- a/test/factories/rubygem.rb +++ b/test/factories/rubygem.rb @@ -19,11 +19,11 @@ after(:create) do |rubygem, evaluator| evaluator.owners.each do |owner| - create(:ownership, rubygem: rubygem, user: owner, access_level: Access::OWNER) + create(:ownership, rubygem: rubygem, user: owner, role: :owner) end evaluator.maintainers.each do |maintainer| - create(:ownership, rubygem: rubygem, user: maintainer, access_level: Access::MAINTAINER) + create(:ownership, rubygem: rubygem, user: maintainer, role: :maintainer) end create(:version, rubygem: rubygem, number: evaluator.number) if evaluator.number diff --git a/test/functional/api/v1/owners_controller_test.rb b/test/functional/api/v1/owners_controller_test.rb index 36e00895a70..f1577140ba5 100644 --- a/test/functional/api/v1/owners_controller_test.rb +++ b/test/functional/api/v1/owners_controller_test.rb @@ -542,7 +542,7 @@ def self.should_respond_to(format) post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.display_id, role: :invalid } assert_equal 422, @response.status - assert_equal "Invalid role. Role must be: maintainer or owner", @response.body + assert_equal "Role is not included in the list", @response.body end should "not create the ownership" do @@ -976,8 +976,8 @@ def self.should_respond_to(format) @maintainer = create(:user) @rubygem = create(:rubygem) - @owner_gem_ownership = create(:ownership, user: @owner, rubygem: @rubygem, access_level: Access::OWNER) - @maintainer_gem_ownership = create(:ownership, user: @maintainer, rubygem: @rubygem, access_level: Access::MAINTAINER) + @owner_gem_ownership = create(:ownership, user: @owner, rubygem: @rubygem, role: :owner) + @maintainer_gem_ownership = create(:ownership, user: @maintainer, rubygem: @rubygem, role: :maintainer) @api_key = create(:api_key, key: "12223", scopes: %i[update_owner], owner: @owner, rubygem: @rubygem) @request.env["HTTP_AUTHORIZATION"] = "12223" @@ -987,7 +987,7 @@ def self.should_respond_to(format) patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :maintainer } assert_response :success - assert_equal Access::MAINTAINER, @maintainer_gem_ownership.reload.access_level + assert_predicate @maintainer_gem_ownership.reload.role, :maintainer? end context "when the role is invalid" do @@ -996,13 +996,13 @@ def self.should_respond_to(format) respond_with :bad_request - assert_equal "Invalid role. Role must be: maintainer or owner", @response.body + assert_equal "Role is not included in the list", @response.body end should "not update the user with the new role" do patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid } - assert_equal Access::MAINTAINER, @maintainer_gem_ownership.reload.access_level + assert_predicate @maintainer_gem_ownership.reload.role, :maintainer? end end end diff --git a/test/functional/owners_controller_test.rb b/test/functional/owners_controller_test.rb index 61000fee001..5d07905142d 100644 --- a/test/functional/owners_controller_test.rb +++ b/test/functional/owners_controller_test.rb @@ -144,6 +144,19 @@ class OwnersControllerTest < ActionController::TestCase assert_equal expected_notice, flash[:notice] end end + + context "with invalid role" do + setup do + @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api) + post :create, params: { handle: @new_owner.display_id, rubygem_id: @rubygem.name, role: :invalid } + end + + should render_template :index + + should "set alert notice flash" do + assert_equal "Role is not included in the list", flash[:alert] + end + end end end @@ -377,29 +390,65 @@ class OwnersControllerTest < ActionController::TestCase assert_equal success_flash, flash[:notice] end - should "downgrade the maintainer" do - owner = @rubygem.ownerships.find_by(user_id: @owner.id) + should "downgrade the ownership to a maintainer role" do + ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) + + assert_predicate ownership.role, :maintainer? + end + end + + context "when updating ownership without role" do + setup do + @owner = create(:user) + @maintainer = create(:user) + @rubygem = create(:rubygem, owners: [@owner, @maintainer]) + + verified_sign_in_as(@owner) + patch :update, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id } + end + + should redirect_to("ownerships index") { rubygem_owners_path(@rubygem.slug) } + + should "not update the role" do + ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) + + assert_predicate ownership.role, :owner? + end + end + + context "when updating ownership with invalid role" do + setup do + @owner = create(:user) + @maintainer = create(:user) + @rubygem = create(:rubygem, owners: [@owner, @maintainer]) + + verified_sign_in_as(@owner) + patch :update, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id, role: :invalid } + end + + should respond_with :unprocessable_content - assert_equal Access::OWNER, owner.access_level + should "set error flash message" do + assert_equal "Role is not included in the list", flash[:alert] end end - context "on PATCH to update ownership of currently signed in user" do + context "when updating the role of currently signed in user" do setup do @owner = create(:user) @rubygem = create(:rubygem) - @ownership = create(:ownership, user: @owner, rubygem: @rubygem, access_level: Access::OWNER) + @ownership = create(:ownership, user: @owner, rubygem: @rubygem, role: :owner) verified_sign_in_as(@owner) patch :update, params: { rubygem_id: @rubygem.name, handle: @owner.display_id, role: :maintainer } end should "not update the ownership of the current user" do - assert_equal Access::OWNER, @ownership.reload.access_level + assert_predicate @ownership.reload.role, :owner? end should "set notice flash message" do - assert_equal "You can't update your own access level.", flash[:alert] + assert_equal "Can't update your own Role", flash[:alert] end end end diff --git a/test/integration/owner_test.rb b/test/integration/owner_test.rb index dded2960177..ef1fd53894a 100644 --- a/test/integration/owner_test.rb +++ b/test/integration/owner_test.rb @@ -263,7 +263,7 @@ class OwnerTest < SystemTest visit_ownerships_page within_element owner_row(maintainer) do - click_link "Edit" + click_button "Edit" end select "Maintainer", from: "role" diff --git a/test/lib/access_test.rb b/test/lib/access_test.rb index 0910c8ba2c6..95bf9204a51 100644 --- a/test/lib/access_test.rb +++ b/test/lib/access_test.rb @@ -7,21 +7,41 @@ class AccessTest < ActiveSupport::TestCase end should "cast the given input into the correct type" do - assert_equal "Owner", Access.label_for_role("owner") + assert_equal "Owner", Access.label_for_role(:owner) + end + + context "when the role is unknown" do + should "return nil" do + assert_nil Access.label_for_role(:unknown) + end + end + end + + context ".role_for_flag" do + should "return the role for a given permission flag" do + assert_equal "owner", Access.role_for_flag(Access::OWNER) end - should "return nil when the role is unknown" do - assert_nil Access.label_for_role(:unknown) + context "when the permission flag does not exist" do + should "reutrn nil" do + assert_nil Access.role_for_flag(999) + end end end - context ".role_for_permission" do + context ".flag_for_role" do should "return the role for a given permission flag" do - assert_equal :owner, Access.role_for_permission(Access::OWNER) + assert_equal Access::OWNER, Access.flag_for_role("owner") end - should "when the permission flag does not exist" do - assert_nil Access.role_for_permission(999) + should "cast the given input into the correct type" do + assert_equal Access::OWNER, Access.flag_for_role(:owner) + end + + context "when the role does not exist" do + should "return nil" do + assert_nil Access.flag_for_role(:unknown) + end end end @@ -35,17 +55,11 @@ class AccessTest < ActiveSupport::TestCase assert_nil Access.label_for_role_flag(999) end end - - context "when the input is not a string" do - should "raise an ArgumentError" do - assert_raises(ArgumentError) { Access.label_for_role_flag("owner") } - end - end end context ".options" do should "return an array of options" do - assert_equal [["Maintainer", :maintainer], ["Owner", :owner]], Access.options + assert_equal [%w[Maintainer maintainer], %w[Owner owner]], Access.options end end end diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index 1f6039248b7..d902a592358 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -256,6 +256,13 @@ class OwnershipTest < ActiveSupport::TestCase should "maps the ownership permission level to the role" do assert_predicate @ownership.role, :owner? + assert_equal Access::OWNER, @ownership.read_attribute_before_type_cast("role") + end + + should "correctly serialize the value" do + ownership = Ownership.find(@ownership.id) + + assert_predicate ownership.role, :owner? end end end diff --git a/test/system/avo/rubygems_test.rb b/test/system/avo/rubygems_test.rb index d29f6c2e17f..4fc7a5d9e44 100644 --- a/test/system/avo/rubygems_test.rb +++ b/test/system/avo/rubygems_test.rb @@ -315,7 +315,7 @@ def sign_in_as(user) "owner_notifier" => [nil, true], "authorizer_id" => [nil, security_user.id], "ownership_request_notifier" => [nil, true], - "access_level" => [nil, 70] + "role" => [nil, ownership.role] }, "unchanged" => {} }, From 361310da7d9bbc7b58f69a99933cb198f0f4672d Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Wed, 14 Aug 2024 18:53:13 +1000 Subject: [PATCH 07/28] Notify users when their Role was updated --- app/controllers/api/v1/owners_controller.rb | 7 ++- app/controllers/owners_controller.rb | 1 + app/mailers/owners_mailer.rb | 12 ++++ app/models/rubygem.rb | 2 + .../owners_mailer/owner_updated.html.erb | 40 +++++++++++++ .../owners_mailer/owner_updated.text.erb | 5 ++ .../ownership_confirmation.html.erb | 2 +- config/locales/de.yml | 6 ++ config/locales/en.yml | 6 ++ config/locales/es.yml | 6 ++ config/locales/fr.yml | 6 ++ config/locales/ja.yml | 6 ++ config/locales/nl.yml | 6 ++ config/locales/pt-BR.yml | 6 ++ config/locales/zh-CN.yml | 6 ++ config/locales/zh-TW.yml | 6 ++ lib/access.rb | 17 +++--- .../api/v1/owners_controller_test.rb | 9 +++ test/functional/owners_controller_test.rb | 11 +++- test/lib/access_test.rb | 16 ++--- test/mailers/owners_test.rb | 23 ++++++++ test/mailers/previews/mailer_preview.rb | 12 ++++ test/models/rubygem_test.rb | 59 +++++++++++++++++++ 23 files changed, 246 insertions(+), 24 deletions(-) create mode 100644 app/views/owners_mailer/owner_updated.html.erb create mode 100644 app/views/owners_mailer/owner_updated.text.erb create mode 100644 test/mailers/owners_test.rb diff --git a/app/controllers/api/v1/owners_controller.rb b/app/controllers/api/v1/owners_controller.rb index 6192ac614d9..f4f9f6888de 100644 --- a/app/controllers/api/v1/owners_controller.rb +++ b/app/controllers/api/v1/owners_controller.rb @@ -31,10 +31,11 @@ def create def update authorize @rubygem, :update_owner? - ownership = @rubygem.ownerships.find_by!(user: User.find_by_name!(email_param)) - ownership.role = params[:role] if params[:role].present? + owner = User.find_by_name!(email_param) + ownership = @rubygem.ownerships.find_by!(user: owner) - if ownership.save + if ownership.update(ownership_update_params) + OwnersMailer.with(ownership: ownership, authorizer: @api_key.user).owner_updated.deliver_later render plain: response_with_mfa_warning("Owner updated successfully.") else render plain: response_with_mfa_warning(ownership.errors.full_messages.to_sentence), status: :unprocessable_entity diff --git a/app/controllers/owners_controller.rb b/app/controllers/owners_controller.rb index e5aaf1940ce..6c983913aae 100644 --- a/app/controllers/owners_controller.rb +++ b/app/controllers/owners_controller.rb @@ -63,6 +63,7 @@ def update end if ownership.update(update_params) + OwnersMailer.with(ownership: ownership, authorizer: current_user).owner_updated.deliver_later redirect_to rubygem_owners_path(ownership.rubygem.slug), notice: t(".success_notice", handle: ownership.user.name) else index_with_error ownership.errors.full_messages.to_sentence, :unprocessable_entity diff --git a/app/mailers/owners_mailer.rb b/app/mailers/owners_mailer.rb index 7b6bef127c1..fa9e8ecc639 100644 --- a/app/mailers/owners_mailer.rb +++ b/app/mailers/owners_mailer.rb @@ -13,6 +13,18 @@ def ownership_confirmation(ownership) end end + def owner_updated + @ownership = params[:ownership] + @authorizer = params[:authorizer] + @user = @ownership.user + @rubygem = @ownership.rubygem + + mail( + to: @user.email, + subject: t("mailer.owner_updated.subject", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY, owner_handle: @authorizer.display_handle) + ) + end + def owner_removed(user_id, remover_id, gem_id) @user = User.find(user_id) @remover = User.find(remover_id) diff --git a/app/models/rubygem.rb b/app/models/rubygem.rb index 387350f27f8..8aec3ee710f 100644 --- a/app/models/rubygem.rb +++ b/app/models/rubygem.rb @@ -191,6 +191,8 @@ def owned_by?(user) def owned_by_with_role?(user, minimum_required_role) return false if user.blank? ownerships.exists?(["user_id = ? AND role >= ?", user.id, minimum_required_role]) + rescue KeyError + false end def unconfirmed_ownerships diff --git a/app/views/owners_mailer/owner_updated.html.erb b/app/views/owners_mailer/owner_updated.html.erb new file mode 100644 index 00000000000..201ceeed044 --- /dev/null +++ b/app/views/owners_mailer/owner_updated.html.erb @@ -0,0 +1,40 @@ +<% @title = t("mailer.owner_updated.title") %> +<% @sub_title = t("mailer.owner_updated.subtitle", user_handle: @user.handle) %> + + + + + + + + +
+
+
+

+ <%= t("mailer.owner_updated.body_html", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY) %> +

+
 
+ +

If this change is expected, you do not need to take further action.

+

+ Only if this change is unexpected + please take immediate steps to secure your account and gems: +

+ + <%= render "mailer/compromised_instructions" do %> +
  • Look out for unexpected changes to your gems on RubyGems.org
    • + <% end %> + +

    + + To stop receiving these messages, update your <%= link_to("email notification settings", notifier_url) %>. + +

    +
    + +
     
    + +
     
    + +
    diff --git a/app/views/owners_mailer/owner_updated.text.erb b/app/views/owners_mailer/owner_updated.text.erb new file mode 100644 index 00000000000..71efb03ef9f --- /dev/null +++ b/app/views/owners_mailer/owner_updated.text.erb @@ -0,0 +1,5 @@ +<%= t("mailer.owner_updated.subtitle", user_handle: @user.handle) %> + +<%= t("mailer.owner_updated.body_text", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY) %> + +<%= rubygem_owners_url(@rubygem.slug) %> diff --git a/app/views/owners_mailer/ownership_confirmation.html.erb b/app/views/owners_mailer/ownership_confirmation.html.erb index 1d76dddd526..5e0f8548c4f 100644 --- a/app/views/owners_mailer/ownership_confirmation.html.erb +++ b/app/views/owners_mailer/ownership_confirmation.html.erb @@ -29,7 +29,7 @@
    - VERIFY + VERIFY
    diff --git a/config/locales/de.yml b/config/locales/de.yml index e7b0f1de694..676f6c60a3a 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -370,6 +370,12 @@ de: subtitle: Hallo %{user_handle}! body_html: Du wurdest als Besitzer für das %{gem}-Gem auf %{host} von %{remover} entfernt. + owner_updated: + subject: + title: + subtitle: + body_html: + body_text: ownerhip_request_closed: title: ANFRAGE ZUR BESITZERRECHTIGUNG subtitle: Hallo %{handle}! diff --git a/config/locales/en.yml b/config/locales/en.yml index 65c8d9ce899..f8d8aed89a2 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -336,6 +336,12 @@ en: title: OWNER REMOVED subtitle: Hi %{user_handle}! body_html: You were removed as an owner for %{gem} gem on %{host} by %{remover}. + owner_updated: + subject: Your role was updated for %{gem} gem + title: OWNER ROLE UPDATED + subtitle: Hi %{user_handle}! + body_html: Your role was updated for %{gem} gem by %{authorizer}. + body_text: Your role was updated for %{gem} gem by %{authorizer}. ownerhip_request_closed: title: OWNERSHIP REQUEST subtitle: Hi %{handle}! diff --git a/config/locales/es.yml b/config/locales/es.yml index 9a11da17a93..f511ad9f8e5 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -369,6 +369,12 @@ es: subtitle: "¡Hola %{handle}!" body_html: Has sido eliminado como propietario/a de la gema %{gem} en %{host} por %{remover}. + owner_updated: + subject: + title: + subtitle: + body_html: + body_text: ownerhip_request_closed: title: CANDIDATURA A PROPIETARIO subtitle: "¡Hola %{handle}!" diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 2656b7b3550..6de0edd5824 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -348,6 +348,12 @@ fr: title: subtitle: body_html: + owner_updated: + subject: + title: + subtitle: + body_html: + body_text: ownerhip_request_closed: title: subtitle: diff --git a/config/locales/ja.yml b/config/locales/ja.yml index 174f6d06811..886259682c5 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -336,6 +336,12 @@ ja: subtitle: こんにちは、%{user_handle}！ body_html: あなたは%{remover}によって%{host}上の%{gem} gemの所有者から削除されました。 + owner_updated: + subject: + title: + subtitle: + body_html: + body_text: ownerhip_request_closed: title: 所有権の申請 subtitle: こんにちは、%{handle}！ diff --git a/config/locales/nl.yml b/config/locales/nl.yml index 8d081f2f09c..3d263e9aafc 100644 --- a/config/locales/nl.yml +++ b/config/locales/nl.yml @@ -333,6 +333,12 @@ nl: title: subtitle: body_html: + owner_updated: + subject: + title: + subtitle: + body_html: + body_text: ownerhip_request_closed: title: subtitle: diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml index b9e7b08cfa7..49a3381ee9c 100644 --- a/config/locales/pt-BR.yml +++ b/config/locales/pt-BR.yml @@ -345,6 +345,12 @@ pt-BR: title: subtitle: body_html: + owner_updated: + subject: + title: + subtitle: + body_html: + body_text: ownerhip_request_closed: title: subtitle: diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml index 9d19e4eb52e..2dd019e1c02 100644 --- a/config/locales/zh-CN.yml +++ b/config/locales/zh-CN.yml @@ -338,6 +338,12 @@ zh-CN: subtitle: 你好啊，%{user_handle}！ body_html: 您在 RubyGems.org 上对 Gem %{gem} 的业主身份已被 %{remover} 移除 + owner_updated: + subject: + title: + subtitle: + body_html: + body_text: ownerhip_request_closed: title: 所有权申请 subtitle: 你好啊，%{hand}！ diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml index 21246d34e4a..868457b1e16 100644 --- a/config/locales/zh-TW.yml +++ b/config/locales/zh-TW.yml @@ -333,6 +333,12 @@ zh-TW: title: subtitle: 嗨 %{user_handle}！ body_html: + owner_updated: + subject: + title: + subtitle: + body_html: + body_text: ownerhip_request_closed: title: 所有權請求 subtitle: 嗨 %{handle}！ diff --git a/lib/access.rb b/lib/access.rb index 0a5b6c5066f..c9a71a87831 100644 --- a/lib/access.rb +++ b/lib/access.rb @@ -1,6 +1,4 @@ module Access - AccessDeniedError = Class.new(StandardError) - MAINTAINER = 50 OWNER = 70 @@ -9,30 +7,31 @@ module Access ROLES = { "maintainer" => MAINTAINER, "owner" => OWNER - }.freeze + }.with_indifferent_access.freeze def self.roles ROLES.keys end def self.label_for_role(role) - key = ROLES.fetch(role.to_s, nil) - return nil if key.nil? + ROLES.fetch(role) I18n.t("access.roles.#{role}") end def self.label_for_role_flag(flag) - role = ROLES.key(flag) { nil } - return nil if role.nil? + role = ROLES.key(flag) + raise ArgumentError, "Unknown role flag: #{flag}" if role.blank? I18n.t("access.roles.#{role}") end def self.flag_for_role(role) - ROLES.fetch(role.to_s, nil) + ROLES.fetch(role) end def self.role_for_flag(flag) - ROLES.key(flag)&.inquiry + ROLES.key(flag)&.inquiry.tap do |role| + raise ArgumentError, "Unknown role flag: #{flag}" if role.blank? + end end def self.options diff --git a/test/functional/api/v1/owners_controller_test.rb b/test/functional/api/v1/owners_controller_test.rb index f1577140ba5..307394b3701 100644 --- a/test/functional/api/v1/owners_controller_test.rb +++ b/test/functional/api/v1/owners_controller_test.rb @@ -2,6 +2,7 @@ class Api::V1::OwnersControllerTest < ActionController::TestCase include ActiveJob::TestHelper + include ActionMailer::TestHelper def self.should_respond_to(format) should "route GET show with #{format.to_s.upcase}" do @@ -990,6 +991,14 @@ def self.should_respond_to(format) assert_predicate @maintainer_gem_ownership.reload.role, :maintainer? end + should "schedule an email for the updated user" do + patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :maintainer } + + ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) + + assert_enqueued_email_with OwnersMailer, :owner_updated, params: { ownership: ownership, authorizer: @owner } + end + context "when the role is invalid" do should "return a bad request response with the error message" do patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid } diff --git a/test/functional/owners_controller_test.rb b/test/functional/owners_controller_test.rb index 5d07905142d..4d4f289473e 100644 --- a/test/functional/owners_controller_test.rb +++ b/test/functional/owners_controller_test.rb @@ -384,10 +384,9 @@ class OwnersControllerTest < ActionController::TestCase end should redirect_to("rubygem show") { rubygem_owners_path(@rubygem.slug) } - should "set success notice flash" do - success_flash = "#{@maintainer.name} was succesfully updated." - assert_equal success_flash, flash[:notice] + should "set success notice flash" do + assert_equal "#{@maintainer.name} was succesfully updated.", flash[:notice] end should "downgrade the ownership to a maintainer role" do @@ -395,6 +394,12 @@ class OwnersControllerTest < ActionController::TestCase assert_predicate ownership.role, :maintainer? end + + should "schedule an email for the updated user" do + ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) + + assert_enqueued_email_with OwnersMailer, :owner_updated, params: { ownership: ownership, authorizer: @owner } + end end context "when updating ownership without role" do diff --git a/test/lib/access_test.rb b/test/lib/access_test.rb index 95bf9204a51..5130ac04656 100644 --- a/test/lib/access_test.rb +++ b/test/lib/access_test.rb @@ -11,8 +11,8 @@ class AccessTest < ActiveSupport::TestCase end context "when the role is unknown" do - should "return nil" do - assert_nil Access.label_for_role(:unknown) + should "raise an error" do + assert_raises(KeyError) { Access.label_for_role(:unknown) } end end end @@ -23,8 +23,8 @@ class AccessTest < ActiveSupport::TestCase end context "when the permission flag does not exist" do - should "reutrn nil" do - assert_nil Access.role_for_flag(999) + should "raise an error" do + assert_raises(ArgumentError) { Access.role_for_flag(999) } end end end @@ -39,8 +39,8 @@ class AccessTest < ActiveSupport::TestCase end context "when the role does not exist" do - should "return nil" do - assert_nil Access.flag_for_role(:unknown) + should "raise an error" do + assert_raises(KeyError) { Access.flag_for_role("unknown") } end end end @@ -51,8 +51,8 @@ class AccessTest < ActiveSupport::TestCase end context "when the role flag is invalid" do - should "return nil" do - assert_nil Access.label_for_role_flag(999) + should "raise an error" do + assert_raises(ArgumentError) { Access.label_for_role_flag(999) } end end end diff --git a/test/mailers/owners_test.rb b/test/mailers/owners_test.rb new file mode 100644 index 00000000000..22a719825a9 --- /dev/null +++ b/test/mailers/owners_test.rb @@ -0,0 +1,23 @@ +require "test_helper" + +class OwnersMailerTest < ActionMailer::TestCase + setup do + @owner = create(:user) + @maintainer = create(:user) + @rubygem = create(:rubygem, name: "test-gem") + @owner_ownership = create(:ownership, rubygem: @rubygem, user: @owner) + @maintainer_ownership = create(:ownership, rubygem: @rubygem, user: @maintainer) + end + + context "#owner_updated" do + should "include host in subject" do + email = OwnersMailer.with(ownership: @maintainer_ownership, authorizer: @owner).owner_updated + + assert_emails(1) { email.deliver_now } + + assert_equal email.subject, "Your role was updated for #{@rubygem.name} gem" + end + + should "" + end +end diff --git a/test/mailers/previews/mailer_preview.rb b/test/mailers/previews/mailer_preview.rb index 0f9d7debf59..38e1e9b3a4a 100644 --- a/test/mailers/previews/mailer_preview.rb +++ b/test/mailers/previews/mailer_preview.rb @@ -96,6 +96,18 @@ def owner_added OwnersMailer.owner_added(user.id, owner.id, authorizer.id, gem.id) end + def owner_updated + # owner = create(:user) + # maintainer = create(:user) + # rubygem = create(:rubygem, name: "test-gem") + # owner_ownership = create(:ownership, rubygem: rubygem, user: owner) + # maintainer_ownership = create(:ownership, rubygem: rubygem, user: maintainer) + ownership = Ownership.last + owner = User.last + + OwnersMailer.with(ownership: ownership, authorizer: owner).owner_updated + end + def api_key_created api_key = ApiKey.where(owner_type: "User").last Mailer.api_key_created(api_key.id) diff --git a/test/models/rubygem_test.rb b/test/models/rubygem_test.rb index 68626ff4a44..f27fe34e490 100644 --- a/test/models/rubygem_test.rb +++ b/test/models/rubygem_test.rb @@ -1180,4 +1180,63 @@ class RubygemTest < ActiveSupport::TestCase refute_predicate @version_three, :yanked? end end + + context "#owned_by_with_role?" do + setup do + @rubygem = create(:rubygem) + @owner = create(:user) + end + + context "when the user is not an owner of a gem" do + should "return false" do + refute @rubygem.owned_by_with_role?(@owner, :maintainer) + end + end + + context "when the user is an owner of a gem" do + setup do + @ownership = create(:ownership, user: @owner, rubygem: @rubygem) + end + + should "return true" do + assert @rubygem.owned_by_with_role?(@owner, :maintainer) + end + end + + context "when the role is less than the given value" do + setup do + @ownership = create(:ownership, user: @owner, rubygem: @rubygem, role: :maintainer) + end + + should "return false" do + refute @rubygem.owned_by_with_role?(@owner, :owner) + end + end + + context "when the role is more than the given value" do + setup do + @ownership = create(:ownership, user: @owner, rubygem: @rubygem, role: :owner) + end + + should "return true" do + assert @rubygem.owned_by_with_role?(@owner, :maintainer) + end + end + + context "when the role is equal to the given role" do + setup do + @ownership = create(:ownership, user: @owner, rubygem: @rubygem, role: :owner) + end + + should "return true" do + assert @rubygem.owned_by_with_role?(@owner, :owner) + end + end + + context "when the given role does not exist" do + should "not raise an argument" do + refute @rubygem.owned_by_with_role?(@owner, :nonexistent_role) + end + end + end end From 75c540aa7bdfe6a47428443f8f42a57a5e0162c2 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Tue, 3 Sep 2024 21:09:18 +1000 Subject: [PATCH 08/28] =?UTF-8?q?Don=E2=80=99t=20allow=20a=20user=20to=20u?= =?UTF-8?q?pdate=20their=20own=20Ownership?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/views/owners/_owners_table.html.erb | 1 + test/integration/owner_test.rb | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/app/views/owners/_owners_table.html.erb b/app/views/owners/_owners_table.html.erb index 1fc610f6912..70ee9db2bc9 100644 --- a/app/views/owners/_owners_table.html.erb +++ b/app/views/owners/_owners_table.html.erb @@ -58,6 +58,7 @@
    <%= button_to "Edit", edit_rubygem_owner_path(@rubygem.name, ownership.user.display_id), + disabled: ownership.user == current_user, method: "get", class: "form__submit form__submit--small" %> diff --git a/test/integration/owner_test.rb b/test/integration/owner_test.rb index ef1fd53894a..e13015369cc 100644 --- a/test/integration/owner_test.rb +++ b/test/integration/owner_test.rb @@ -273,6 +273,14 @@ class OwnerTest < SystemTest assert_cell maintainer, "Role", "Maintainer" end + test "editing the ownership of the current user" do + visit_ownerships_page + + within_element owner_row(@user) do + assert_selector "button[disabled]", text: "Edit" + end + end + test "creating new owner with maintainer role" do maintainer = create(:user) From c11b67e2f534a4e473c688c34f517da57fa83677 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Wed, 4 Sep 2024 14:52:00 +1000 Subject: [PATCH 09/28] prevent maintainers for accessing trusted publishing --- .../oidc/rubygem_trusted_publisher_policy.rb | 6 +-- .../rubygem_trusted_publishing_policy_test.rb | 42 +++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) create mode 100644 test/policies/rubygem_trusted_publishing_policy_test.rb diff --git a/app/policies/oidc/rubygem_trusted_publisher_policy.rb b/app/policies/oidc/rubygem_trusted_publisher_policy.rb index b035c911d03..f8f87383af6 100644 --- a/app/policies/oidc/rubygem_trusted_publisher_policy.rb +++ b/app/policies/oidc/rubygem_trusted_publisher_policy.rb @@ -5,14 +5,14 @@ class Scope < ApplicationPolicy::Scope delegate :rubygem, to: :record def show? - rubygem_owned_by?(user) + rubygem_owned_by?(user, minimum_required_role: Access::OWNER) end def create? - rubygem_owned_by?(user) + rubygem_owned_by?(user, minimum_required_role: Access::OWNER) end def destroy? - rubygem_owned_by?(user) + rubygem_owned_by?(user, minimum_required_role: Access::OWNER) end end diff --git a/test/policies/rubygem_trusted_publishing_policy_test.rb b/test/policies/rubygem_trusted_publishing_policy_test.rb new file mode 100644 index 00000000000..953044e4c7a --- /dev/null +++ b/test/policies/rubygem_trusted_publishing_policy_test.rb @@ -0,0 +1,42 @@ +require "test_helper" + +class RubygemTrustedPublishingPolicyTest < PolicyTestCase + setup do + @owner = create(:user, handle: "owner") + @maintainer = create(:user, handle: "maintainer") + @user = create(:user, handle: "user") + @rubygem = create(:rubygem, owners: [@owner], maintainers: [@maintainer]) + @rubygem_trusted_publisher = create(:oidc_rubygem_trusted_publisher, rubygem: @rubygem) + end + + def policy!(user) + Pundit.policy!(user, @rubygem_trusted_publisher) + end + + context "#show?" do + should "only allow the owner" do + assert_authorized @owner, :show? + refute_authorized @maintainer, :show? + refute_authorized @user, :show? + refute_authorized nil, :show? + end + end + + context "create?" do + should "only allow the owner" do + assert_authorized @owner, :create? + refute_authorized @maintainer, :create? + refute_authorized @user, :create? + refute_authorized nil, :create? + end + end + + context "#destroy?" do + should "only allow the owner" do + assert_authorized @owner, :destroy? + refute_authorized @maintainer, :destroy? + refute_authorized @user, :destroy? + refute_authorized nil, :destroy? + end + end +end From 3765cb2d86c7fc474f79e6fbd51581f24571d7ff Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Thu, 5 Sep 2024 00:42:58 +1000 Subject: [PATCH 10/28] Replace Role type object with an Enum --- app/avo/resources/ownership_resource.rb | 2 +- app/models/ownership.rb | 4 +-- app/models/rubygem.rb | 2 +- app/policies/application_policy.rb | 2 +- .../oidc/rubygem_trusted_publisher_policy.rb | 6 ++-- app/policies/rubygem_policy.rb | 16 ++++----- app/views/owners/_owners_table.html.erb | 2 +- app/views/owners/edit.html.erb | 2 +- app/views/owners/index.html.erb | 2 +- config/initializers/active_model_types.rb | 1 - config/locales/de.yml | 6 +--- config/locales/en.yml | 6 +--- config/locales/es.yml | 6 +--- config/locales/fr.yml | 6 +--- config/locales/ja.yml | 6 +--- config/locales/nl.yml | 6 +--- config/locales/pt-BR.yml | 6 +--- config/locales/zh-CN.yml | 6 +--- config/locales/zh-TW.yml | 6 +--- lib/access.rb | 21 ------------ lib/types/role.rb | 16 --------- .../api/v1/owners_controller_test.rb | 8 ++--- test/functional/owners_controller_test.rb | 6 ++-- test/lib/access_test.rb | 34 ------------------- test/models/ownership_test.rb | 4 +-- 25 files changed, 37 insertions(+), 145 deletions(-) delete mode 100644 lib/types/role.rb diff --git a/app/avo/resources/ownership_resource.rb b/app/avo/resources/ownership_resource.rb index 26d4b87ff18..ed4a6f686bd 100644 --- a/app/avo/resources/ownership_resource.rb +++ b/app/avo/resources/ownership_resource.rb @@ -26,5 +26,5 @@ class ConfirmedFilter < ScopeBooleanFilter; end field :authorizer, as: :belongs_to field :confirmed_at, as: :date_time - field :role, as: :text + field :role, as: :select, enum: Ownership.roles end diff --git a/app/models/ownership.rb b/app/models/ownership.rb index 823ffdb404f..023023b35c8 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -4,9 +4,7 @@ class Ownership < ApplicationRecord belongs_to :authorizer, class_name: "User" has_many :api_key_rubygem_scopes, dependent: :destroy - attribute :role, Types::Role.new, default: Access::DEFAULT_ROLE validate :validate_unique_user - validates :role, inclusion: { in: Access.roles }, allow_nil: true delegate :name, to: :user, prefix: :owner delegate :name, to: :authorizer, prefix: true, allow_nil: true @@ -20,6 +18,8 @@ class Ownership < ApplicationRecord scope :confirmed, -> { where.not(confirmed_at: nil) } scope :unconfirmed, -> { where(confirmed_at: nil) } + enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER }, validate: true, _default: :owner + def self.by_indexed_gem_name select("ownerships.*, rubygems.name") .left_joins(rubygem: :versions) diff --git a/app/models/rubygem.rb b/app/models/rubygem.rb index 8aec3ee710f..87c31ee783a 100644 --- a/app/models/rubygem.rb +++ b/app/models/rubygem.rb @@ -190,7 +190,7 @@ def owned_by?(user) def owned_by_with_role?(user, minimum_required_role) return false if user.blank? - ownerships.exists?(["user_id = ? AND role >= ?", user.id, minimum_required_role]) + ownerships.exists?(["user_id = ? AND role >= ?", user.id, Access.flag_for_role(minimum_required_role)]) rescue KeyError false end diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 910a177cf82..7e80a2f2a29 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -76,7 +76,7 @@ def current_user?(record_user) user && user == record_user end - def rubygem_owned_by?(user, minimum_required_role: Access::MAINTAINER) + def rubygem_owned_by?(user, minimum_required_role: :maintainer) rubygem.owned_by_with_role?(user, minimum_required_role) || deny(t(:forbidden)) end diff --git a/app/policies/oidc/rubygem_trusted_publisher_policy.rb b/app/policies/oidc/rubygem_trusted_publisher_policy.rb index f8f87383af6..85ea19aef2d 100644 --- a/app/policies/oidc/rubygem_trusted_publisher_policy.rb +++ b/app/policies/oidc/rubygem_trusted_publisher_policy.rb @@ -5,14 +5,14 @@ class Scope < ApplicationPolicy::Scope delegate :rubygem, to: :record def show? - rubygem_owned_by?(user, minimum_required_role: Access::OWNER) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end def create? - rubygem_owned_by?(user, minimum_required_role: Access::OWNER) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end def destroy? - rubygem_owned_by?(user, minimum_required_role: Access::OWNER) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end end diff --git a/app/policies/rubygem_policy.rb b/app/policies/rubygem_policy.rb index 0a858a85490..ec343dd0e0c 100644 --- a/app/policies/rubygem_policy.rb +++ b/app/policies/rubygem_policy.rb @@ -24,15 +24,15 @@ def destroy? end def configure_oidc? - rubygem_owned_by?(user, required_access_level: Access::OWNER) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end def configure_trusted_publishers? - rubygem_owned_by?(user, required_access_level: Access::OWNER) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end def manage_adoption? - rubygem_owned_by?(user, required_access_level: Access::OWNER) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end def request_ownership? @@ -51,22 +51,22 @@ def show_events? end def close_ownership_requests? - rubygem_owned_by?(user, required_access_level: Access::OWNER) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end def show_unconfirmed_ownerships? - rubygem_owned_by?(user, minimum_required_role: Access::OWNER) + rubygem_owned_by?(user, minimum_required_role: :owner) end def add_owner? - rubygem_owned_by?(user, minimum_required_role: Access::OWNER) + rubygem_owned_by?(user, minimum_required_role: :owner) end def update_owner? - rubygem_owned_by?(user, minimum_required_role: Access::OWNER) + rubygem_owned_by?(user, minimum_required_role: :owner) end def remove_owner? - rubygem_owned_by?(user, minimum_required_role: Access::OWNER) + rubygem_owned_by?(user, minimum_required_role: :owner) end end diff --git a/app/views/owners/_owners_table.html.erb b/app/views/owners/_owners_table.html.erb index 70ee9db2bc9..8d26dcee374 100644 --- a/app/views/owners/_owners_table.html.erb +++ b/app/views/owners/_owners_table.html.erb @@ -44,7 +44,7 @@ <%= ownership.authorizer_name %> - <%= Access.label_for_role(ownership.role) %> + <%= ownership.role %> <%= ownership.confirmed_at.strftime("%Y-%m-%d %H:%M %Z") if ownership.confirmed? %> diff --git a/app/views/owners/edit.html.erb b/app/views/owners/edit.html.erb index e19b1efa69b..fe9ccc8328c 100644 --- a/app/views/owners/edit.html.erb +++ b/app/views/owners/edit.html.erb @@ -11,7 +11,7 @@
    <%= label_tag :role, t(".role"), class: 'form__label' %>
    - <%= select_tag :role, options_for_select(Access.options.to_a.reverse, @ownership.role), class: "form__input form__select" %> + <%= select_tag :role, options_for_select(Ownership.roles, @ownership.role), class: "form__input form__select" %>
    diff --git a/app/views/owners/index.html.erb b/app/views/owners/index.html.erb index 43f54200e70..0a44ada1cef 100644 --- a/app/views/owners/index.html.erb +++ b/app/views/owners/index.html.erb @@ -17,7 +17,7 @@
    <%= label_tag :role, t(".role_field"), class: "form__label" %>
    - <%= select_tag :role, options_for_select(Access.options.to_a.reverse), class: "form__input form__select" %> + <%= select_tag :role, options_for_select(Ownership.roles), class: "form__input form__select" %>
    <%= submit_tag t(".submit_button"), data: {disable_with: t("form_disable_with")}, class: "form__submit" %> diff --git a/config/initializers/active_model_types.rb b/config/initializers/active_model_types.rb index 9ecd31ce279..6f1a5ab64c3 100644 --- a/config/initializers/active_model_types.rb +++ b/config/initializers/active_model_types.rb @@ -1,5 +1,4 @@ ActiveSupport.on_load(:active_model) do ActiveModel::Type.register(:global_id, Types::GlobalId) ActiveModel::Type.register(:duration, Types::Duration) - ActiveModel::Type.register(:role, Types::Role) end diff --git a/config/locales/de.yml b/config/locales/de.yml index 676f6c60a3a..d1edb7ff80a 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -1069,8 +1069,4 @@ de: api_key_scopes: api_key_gem_html: api_key_mfa: - not_required: - access: - roles: - owner: - maintainer: + not_required: \ No newline at end of file diff --git a/config/locales/en.yml b/config/locales/en.yml index f8d8aed89a2..3f5fd2b793f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -990,8 +990,4 @@ en: api_key_scopes: "Scopes: %{scopes}" api_key_gem_html: "Gem: %{gem}" api_key_mfa: "MFA: %{mfa}" - not_required: "Not required" - access: - roles: - owner: Owner - maintainer: Maintainer + not_required: "Not required" \ No newline at end of file diff --git a/config/locales/es.yml b/config/locales/es.yml index f511ad9f8e5..7052cc6879b 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -1124,8 +1124,4 @@ es: api_key_scopes: api_key_gem_html: api_key_mfa: - not_required: - access: - roles: - owner: - maintainer: + not_required: \ No newline at end of file diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 6de0edd5824..0ae1b525ed5 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -1019,8 +1019,4 @@ fr: api_key_scopes: api_key_gem_html: api_key_mfa: - not_required: - access: - roles: - owner: - maintainer: + not_required: \ No newline at end of file diff --git a/config/locales/ja.yml b/config/locales/ja.yml index 886259682c5..f4a09ae1d9d 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -995,8 +995,4 @@ ja: api_key_scopes: スコープ：%{scopes} api_key_gem_html: 'gem: %{gem}' api_key_mfa: 'MFA: %{mfa}' - not_required: 必要ではありません - access: - roles: - owner: - maintainer: + not_required: 必要ではありません \ No newline at end of file diff --git a/config/locales/nl.yml b/config/locales/nl.yml index 3d263e9aafc..c7434fb3d52 100644 --- a/config/locales/nl.yml +++ b/config/locales/nl.yml @@ -974,8 +974,4 @@ nl: api_key_scopes: api_key_gem_html: api_key_mfa: - not_required: - access: - roles: - owner: - maintainer: + not_required: \ No newline at end of file diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml index 49a3381ee9c..3eca3e1c19a 100644 --- a/config/locales/pt-BR.yml +++ b/config/locales/pt-BR.yml @@ -997,8 +997,4 @@ pt-BR: api_key_scopes: api_key_gem_html: api_key_mfa: - not_required: - access: - roles: - owner: - maintainer: + not_required: \ No newline at end of file diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml index 2dd019e1c02..3a019fc0461 100644 --- a/config/locales/zh-CN.yml +++ b/config/locales/zh-CN.yml @@ -988,8 +988,4 @@ zh-CN: api_key_scopes: api_key_gem_html: api_key_mfa: - not_required: - access: - roles: - owner: - maintainer: + not_required: \ No newline at end of file diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml index 868457b1e16..222ae4af7bb 100644 --- a/config/locales/zh-TW.yml +++ b/config/locales/zh-TW.yml @@ -979,8 +979,4 @@ zh-TW: api_key_scopes: api_key_gem_html: api_key_mfa: - not_required: - access: - roles: - owner: - maintainer: + not_required: \ No newline at end of file diff --git a/lib/access.rb b/lib/access.rb index c9a71a87831..934575a8e81 100644 --- a/lib/access.rb +++ b/lib/access.rb @@ -9,21 +9,6 @@ module Access "owner" => OWNER }.with_indifferent_access.freeze - def self.roles - ROLES.keys - end - - def self.label_for_role(role) - ROLES.fetch(role) - I18n.t("access.roles.#{role}") - end - - def self.label_for_role_flag(flag) - role = ROLES.key(flag) - raise ArgumentError, "Unknown role flag: #{flag}" if role.blank? - I18n.t("access.roles.#{role}") - end - def self.flag_for_role(role) ROLES.fetch(role) end @@ -33,10 +18,4 @@ def self.role_for_flag(flag) raise ArgumentError, "Unknown role flag: #{flag}" if role.blank? end end - - def self.options - ROLES.map do |role, flag| - [label_for_role_flag(flag), role] - end - end end diff --git a/lib/types/role.rb b/lib/types/role.rb deleted file mode 100644 index df23141c7cd..00000000000 --- a/lib/types/role.rb +++ /dev/null @@ -1,16 +0,0 @@ -class Types::Role < ActiveRecord::Type::Integer - def cast(value) - return nil if value.blank? - value.to_s - end - - def serialize(value) - return nil if value.blank? - Access.flag_for_role(value) - end - - def deserialize(value) - return nil if value.blank? - Access.role_for_flag(value) - end -end diff --git a/test/functional/api/v1/owners_controller_test.rb b/test/functional/api/v1/owners_controller_test.rb index 307394b3701..434ecc4dffb 100644 --- a/test/functional/api/v1/owners_controller_test.rb +++ b/test/functional/api/v1/owners_controller_test.rb @@ -525,7 +525,7 @@ def self.should_respond_to(format) post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.display_id } assert_equal 200, @response.status - assert_predicate Ownership.find_by(user: @second_user, rubygem: @rubygem).role, :owner? + assert_predicate Ownership.find_by(user: @second_user, rubygem: @rubygem), :owner? end end @@ -534,7 +534,7 @@ def self.should_respond_to(format) post :create, params: { rubygem_id: @rubygem.slug, email: @second_user.display_id, role: :maintainer } assert_equal 200, @response.status - assert_predicate Ownership.find_by(user: @second_user, rubygem: @rubygem).role, :maintainer? + assert_predicate Ownership.find_by(user: @second_user, rubygem: @rubygem), :maintainer? end end @@ -988,7 +988,7 @@ def self.should_respond_to(format) patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :maintainer } assert_response :success - assert_predicate @maintainer_gem_ownership.reload.role, :maintainer? + assert_predicate @maintainer_gem_ownership.reload, :maintainer? end should "schedule an email for the updated user" do @@ -1011,7 +1011,7 @@ def self.should_respond_to(format) should "not update the user with the new role" do patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid } - assert_predicate @maintainer_gem_ownership.reload.role, :maintainer? + assert_predicate @maintainer_gem_ownership.reload, :maintainer? end end end diff --git a/test/functional/owners_controller_test.rb b/test/functional/owners_controller_test.rb index 4d4f289473e..ce8e9d823a2 100644 --- a/test/functional/owners_controller_test.rb +++ b/test/functional/owners_controller_test.rb @@ -392,7 +392,7 @@ class OwnersControllerTest < ActionController::TestCase should "downgrade the ownership to a maintainer role" do ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) - assert_predicate ownership.role, :maintainer? + assert_predicate ownership, :maintainer? end should "schedule an email for the updated user" do @@ -417,7 +417,7 @@ class OwnersControllerTest < ActionController::TestCase should "not update the role" do ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) - assert_predicate ownership.role, :owner? + assert_predicate ownership, :owner? end end @@ -449,7 +449,7 @@ class OwnersControllerTest < ActionController::TestCase end should "not update the ownership of the current user" do - assert_predicate @ownership.reload.role, :owner? + assert_predicate @ownership.reload, :owner? end should "set notice flash message" do diff --git a/test/lib/access_test.rb b/test/lib/access_test.rb index 5130ac04656..0ac75fd2485 100644 --- a/test/lib/access_test.rb +++ b/test/lib/access_test.rb @@ -1,22 +1,6 @@ require "test_helper" class AccessTest < ActiveSupport::TestCase - context ".label_for_role" do - should "return the correct label for the given role" do - assert_equal "Owner", Access.label_for_role(:owner) - end - - should "cast the given input into the correct type" do - assert_equal "Owner", Access.label_for_role(:owner) - end - - context "when the role is unknown" do - should "raise an error" do - assert_raises(KeyError) { Access.label_for_role(:unknown) } - end - end - end - context ".role_for_flag" do should "return the role for a given permission flag" do assert_equal "owner", Access.role_for_flag(Access::OWNER) @@ -44,22 +28,4 @@ class AccessTest < ActiveSupport::TestCase end end end - - context ".label_for_role_flag" do - should "return the label for the role flag" do - assert_equal "Owner", Access.label_for_role_flag(Access::OWNER) - end - - context "when the role flag is invalid" do - should "raise an error" do - assert_raises(ArgumentError) { Access.label_for_role_flag(999) } - end - end - end - - context ".options" do - should "return an array of options" do - assert_equal [%w[Maintainer maintainer], %w[Owner owner]], Access.options - end - end end diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index d902a592358..dd052ccac2b 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -255,14 +255,14 @@ class OwnershipTest < ActiveSupport::TestCase end should "maps the ownership permission level to the role" do - assert_predicate @ownership.role, :owner? + assert_predicate @ownership, :owner? assert_equal Access::OWNER, @ownership.read_attribute_before_type_cast("role") end should "correctly serialize the value" do ownership = Ownership.find(@ownership.id) - assert_predicate ownership.role, :owner? + assert_predicate ownership, :owner? end end end From ca1ffb468117a7a96c75875bfd01619b86fda1a1 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Thu, 5 Sep 2024 14:41:11 +1000 Subject: [PATCH 11/28] separate checking for onwnership and permissions into separate methods --- app/policies/application_policy.rb | 6 +++++- app/policies/rubygem_policy.rb | 8 ++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 7e80a2f2a29..e999c1fac18 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -76,7 +76,11 @@ def current_user?(record_user) user && user == record_user end - def rubygem_owned_by?(user, minimum_required_role: :maintainer) + def rubygem_owned_by?(user) + rubygem.owned_by?(user) || deny(t(:forbidden)) + end + + def rubygem_owned_by_with_role?(user, minimum_required_role:) rubygem.owned_by_with_role?(user, minimum_required_role) || deny(t(:forbidden)) end diff --git a/app/policies/rubygem_policy.rb b/app/policies/rubygem_policy.rb index ec343dd0e0c..1c90dc77967 100644 --- a/app/policies/rubygem_policy.rb +++ b/app/policies/rubygem_policy.rb @@ -55,18 +55,18 @@ def close_ownership_requests? end def show_unconfirmed_ownerships? - rubygem_owned_by?(user, minimum_required_role: :owner) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end def add_owner? - rubygem_owned_by?(user, minimum_required_role: :owner) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end def update_owner? - rubygem_owned_by?(user, minimum_required_role: :owner) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end def remove_owner? - rubygem_owned_by?(user, minimum_required_role: :owner) + rubygem_owned_by_with_role?(user, minimum_required_role: :owner) end end From 4608d5642fef5cdccc96a73c255a02399e926436 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Fri, 6 Sep 2024 16:30:15 +1000 Subject: [PATCH 12/28] Add Minimum Role Scope to the Ownership model Replace the raw sql with something nicer --- app/models/ownership.rb | 10 ++++++++++ app/models/rubygem.rb | 2 +- test/models/ownership_test.rb | 12 ++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/app/models/ownership.rb b/app/models/ownership.rb index 023023b35c8..ea8248255fd 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -20,6 +20,8 @@ class Ownership < ApplicationRecord enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER }, validate: true, _default: :owner + scope :user_with_minimum_role, ->(user, role) { where(user: user, role: Access.flag_for_role(role)..) } + def self.by_indexed_gem_name select("ownerships.*, rubygems.name") .left_joins(rubygem: :versions) @@ -109,6 +111,14 @@ def record_confirmation_event actor_gid: Current.user&.to_gid) end + def record_update_event + rubygem.record_event!(Events::RubygemEvent::OWNER_UPDATED, + owner: user.display_handle, + authorizer: authorizer.display_handle, + owner_gid: user.to_gid, + actor_gid: Current.user&.to_gid) + end + def record_destroy_event rubygem.record_event!(Events::RubygemEvent::OWNER_REMOVED, owner: user.display_handle, diff --git a/app/models/rubygem.rb b/app/models/rubygem.rb index 87c31ee783a..03b5793cceb 100644 --- a/app/models/rubygem.rb +++ b/app/models/rubygem.rb @@ -190,7 +190,7 @@ def owned_by?(user) def owned_by_with_role?(user, minimum_required_role) return false if user.blank? - ownerships.exists?(["user_id = ? AND role >= ?", user.id, Access.flag_for_role(minimum_required_role)]) + ownerships.user_with_minimum_role(user, minimum_required_role).exists? rescue KeyError false end diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index dd052ccac2b..2bb4800ee2e 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -265,4 +265,16 @@ class OwnershipTest < ActiveSupport::TestCase assert_predicate ownership, :owner? end end + + context "#user_with_minimum_role" do + setup do + @user = create(:user) + @ownership = create(:ownership, user: @user, role: :owner) + end + + should "return the ownerships with a role less than or equal to the given role" do + assert_includes Ownership.user_with_minimum_role(@user, :owner), @ownership + assert_includes Ownership.user_with_minimum_role(@user, :maintainer), @ownership + end + end end From 6d1c10fa049fce002f24e39fbbc6d024b1ae29a3 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Fri, 6 Sep 2024 17:39:06 +1000 Subject: [PATCH 13/28] Add translations for Role field --- app/views/owners/_owners_table.html.erb | 2 +- app/views/owners/edit.html.erb | 3 ++- app/views/owners/index.html.erb | 3 ++- config/locales/de.yml | 3 +++ config/locales/en.yml | 3 +++ config/locales/es.yml | 3 +++ config/locales/fr.yml | 3 +++ config/locales/ja.yml | 3 +++ config/locales/nl.yml | 3 +++ config/locales/pt-BR.yml | 3 +++ config/locales/zh-CN.yml | 3 +++ config/locales/zh-TW.yml | 3 +++ 12 files changed, 32 insertions(+), 3 deletions(-) diff --git a/app/views/owners/_owners_table.html.erb b/app/views/owners/_owners_table.html.erb index 8d26dcee374..4ad86899233 100644 --- a/app/views/owners/_owners_table.html.erb +++ b/app/views/owners/_owners_table.html.erb @@ -44,7 +44,7 @@ <%= ownership.authorizer_name %> - <%= ownership.role %> + <%= Ownership.human_attribute_name("role.#{ownership.role}") %> <%= ownership.confirmed_at.strftime("%Y-%m-%d %H:%M %Z") if ownership.confirmed? %> diff --git a/app/views/owners/edit.html.erb b/app/views/owners/edit.html.erb index fe9ccc8328c..bb29eddeea2 100644 --- a/app/views/owners/edit.html.erb +++ b/app/views/owners/edit.html.erb @@ -1,4 +1,5 @@ <% @title = t('.title') %> +<% @roles = Ownership.roles.transform_keys { |key| Ownership.human_attribute_name("role.#{key}") } %> <%= form_tag rubygem_owner_path(rubygem_id: @rubygem.slug, owner_id: @ownership.user.display_id), method: :patch do |form| %> <%= error_messages_for(@ownership) %> @@ -11,7 +12,7 @@
    <%= label_tag :role, t(".role"), class: 'form__label' %>
    - <%= select_tag :role, options_for_select(Ownership.roles, @ownership.role), class: "form__input form__select" %> + <%= select_tag :role, options_for_select(@roles, @ownership.role), class: "form__input form__select" %>
    diff --git a/app/views/owners/index.html.erb b/app/views/owners/index.html.erb index 0a44ada1cef..2e0dae6f19e 100644 --- a/app/views/owners/index.html.erb +++ b/app/views/owners/index.html.erb @@ -1,5 +1,6 @@ <% @title = @rubygem.name %> <% @subtitle = t(".info", gem: @rubygem.name) %> +<% @roles = Ownership.roles.transform_keys { |key| Ownership.human_attribute_name("role.#{key}") } %>
    <%= render "owners_table" %> @@ -17,7 +18,7 @@
    <%= label_tag :role, t(".role_field"), class: "form__label" %>
    - <%= select_tag :role, options_for_select(Ownership.roles), class: "form__input form__select" %> + <%= select_tag :role, options_for_select(@roles), class: "form__input form__select" %>
    <%= submit_tag t(".submit_button"), data: {disable_with: t("form_disable_with")}, class: "form__submit" %> diff --git a/config/locales/de.yml b/config/locales/de.yml index d1edb7ff80a..2bc139e6cee 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -73,6 +73,9 @@ de: full_name: Vollständiger Name handle: Benutzername password: Passwort + ownership/role: + owner: + maintainer: api_key: oidc_api_key_role: OIDC-API-Schlüsselrolle oidc/id_token: diff --git a/config/locales/en.yml b/config/locales/en.yml index 3f5fd2b793f..b8807c69928 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -70,6 +70,9 @@ en: full_name: Full name handle: Username password: Password + ownership/role: + owner: Owner + maintainer: Maintainer api_key: oidc_api_key_role: OIDC API Key Role oidc/id_token: diff --git a/config/locales/es.yml b/config/locales/es.yml index 7052cc6879b..6ba9ca6a1e7 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -71,6 +71,9 @@ es: full_name: Nombre completo handle: Usuario password: Contraseña + ownership/role: + owner: + maintainer: api_key: oidc_api_key_role: oidc/id_token: diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 0ae1b525ed5..d7d0230028f 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -71,6 +71,9 @@ fr: full_name: Nom complet handle: Pseudonyme password: Mot de passe + ownership/role: + owner: + maintainer: api_key: oidc_api_key_role: oidc/id_token: diff --git a/config/locales/ja.yml b/config/locales/ja.yml index f4a09ae1d9d..072af4a16a3 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -64,6 +64,9 @@ ja: full_name: フルネーム handle: ユーザー名 password: パスワード + ownership/role: + owner: + maintainer: api_key: oidc_api_key_role: OIDC APIキーのロール oidc/id_token: diff --git a/config/locales/nl.yml b/config/locales/nl.yml index c7434fb3d52..86e0c5bd776 100644 --- a/config/locales/nl.yml +++ b/config/locales/nl.yml @@ -63,6 +63,9 @@ nl: full_name: Voor-en achternaam handle: Gebruikersnaam password: Wachtwoord + ownership/role: + owner: + maintainer: api_key: oidc_api_key_role: oidc/id_token: diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml index 3eca3e1c19a..2a4bfb06a42 100644 --- a/config/locales/pt-BR.yml +++ b/config/locales/pt-BR.yml @@ -70,6 +70,9 @@ pt-BR: full_name: Nome completo handle: Usuário password: Senha + ownership/role: + owner: + maintainer: api_key: oidc_api_key_role: oidc/id_token: diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml index 3a019fc0461..02005853b80 100644 --- a/config/locales/zh-CN.yml +++ b/config/locales/zh-CN.yml @@ -65,6 +65,9 @@ zh-CN: full_name: 全名 handle: 用户名 password: 密码 + ownership/role: + owner: + maintainer: api_key: oidc_api_key_role: oidc/id_token: diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml index 222ae4af7bb..5da9695e365 100644 --- a/config/locales/zh-TW.yml +++ b/config/locales/zh-TW.yml @@ -64,6 +64,9 @@ zh-TW: full_name: 全名 handle: 帳號 password: 密碼 + ownership/role: + owner: + maintainer: api_key: oidc_api_key_role: OIDC API 金鑰角色 oidc/id_token: From 20bc75af50407572ec03d369a5627d1b161e5207 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Fri, 6 Sep 2024 18:18:03 +1000 Subject: [PATCH 14/28] Record event when a ownership role is updated --- app/models/events/rubygem_event.rb | 8 ++++++++ app/models/ownership.rb | 5 +++-- test/models/ownership_test.rb | 27 +++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/app/models/events/rubygem_event.rb b/app/models/events/rubygem_event.rb index 11c7dea8b13..2899611f6c0 100644 --- a/app/models/events/rubygem_event.rb +++ b/app/models/events/rubygem_event.rb @@ -59,6 +59,14 @@ class Events::RubygemEvent < ApplicationRecord attribute :owner_gid, :global_id end + OWNER_UPDATED = define_event "rubygem:owner:updated" do + attribute :owner, :string + attribute :updated_by, :string + + attribute :actor_gid, :global_id + attribute :owner_gid, :global_id + end + OWNER_REMOVED = define_event "rubygem:owner:removed" do attribute :owner, :string attribute :removed_by, :string diff --git a/app/models/ownership.rb b/app/models/ownership.rb index ea8248255fd..733c22f4b51 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -13,6 +13,7 @@ class Ownership < ApplicationRecord after_create :record_create_event after_update :record_confirmation_event, if: :saved_change_to_confirmed_at? + after_update :record_role_updated_event, if: :saved_change_to_role? after_destroy :record_destroy_event scope :confirmed, -> { where.not(confirmed_at: nil) } @@ -111,10 +112,10 @@ def record_confirmation_event actor_gid: Current.user&.to_gid) end - def record_update_event + def record_role_updated_event rubygem.record_event!(Events::RubygemEvent::OWNER_UPDATED, owner: user.display_handle, - authorizer: authorizer.display_handle, + updated_by: Current.user&.display_handle, owner_gid: user.to_gid, actor_gid: Current.user&.to_gid) end diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index 2bb4800ee2e..3749169441c 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -123,6 +123,33 @@ class OwnershipTest < ActiveSupport::TestCase end end + context "#update" do + context "when updating the role" do + setup do + @actor = create(:user) + @ownership = create(:ownership) + + Current.user = @actor + end + + should "record an event" do + @ownership.update!(role: :maintainer) + + event = @ownership.rubygem.events.last + attributes = { + "user_agent_info" => nil, + "owner" => @ownership.user.display_handle, + "updated_by" => @actor.display_handle, + "actor_gid" => @actor.to_gid, + "owner_gid" => @ownership.user.to_gid + } + + assert_equal "rubygem:owner:updated", event.tag + assert_equal attributes, event.additional.attributes + end + end + end + context "#valid_confirmation_token?" do setup do @ownership = create(:ownership) From 480db28380599f7d9d68e6ecbb0b79e85320c82d Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Mon, 9 Sep 2024 16:18:07 +1000 Subject: [PATCH 15/28] Indicate what role the user was updated to in the notification email --- app/views/owners_mailer/owner_updated.html.erb | 2 +- app/views/owners_mailer/owner_updated.text.erb | 2 +- config/locales/en.yml | 4 ++-- test/mailers/owners_test.rb | 3 --- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/app/views/owners_mailer/owner_updated.html.erb b/app/views/owners_mailer/owner_updated.html.erb index 201ceeed044..936eef230e1 100644 --- a/app/views/owners_mailer/owner_updated.html.erb +++ b/app/views/owners_mailer/owner_updated.html.erb @@ -9,7 +9,7 @@

    - <%= t("mailer.owner_updated.body_html", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY) %> + <%= t("mailer.owner_updated.body_html", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %>

     
    diff --git a/app/views/owners_mailer/owner_updated.text.erb b/app/views/owners_mailer/owner_updated.text.erb index 71efb03ef9f..9ef71d27425 100644 --- a/app/views/owners_mailer/owner_updated.text.erb +++ b/app/views/owners_mailer/owner_updated.text.erb @@ -1,5 +1,5 @@ <%= t("mailer.owner_updated.subtitle", user_handle: @user.handle) %> -<%= t("mailer.owner_updated.body_text", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY) %> +<%= t("mailer.owner_updated.body_text", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %> <%= rubygem_owners_url(@rubygem.slug) %> diff --git a/config/locales/en.yml b/config/locales/en.yml index b8807c69928..57e678dda62 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -343,8 +343,8 @@ en: subject: Your role was updated for %{gem} gem title: OWNER ROLE UPDATED subtitle: Hi %{user_handle}! - body_html: Your role was updated for %{gem} gem by %{authorizer}. - body_text: Your role was updated for %{gem} gem by %{authorizer}. + body_html: Your role was updated to %{role} for %{gem} gem by %{authorizer}. + body_text: Your role was updated to %{role} for %{gem} gem by %{authorizer}. ownerhip_request_closed: title: OWNERSHIP REQUEST subtitle: Hi %{handle}! diff --git a/test/mailers/owners_test.rb b/test/mailers/owners_test.rb index 22a719825a9..f1fc94ea358 100644 --- a/test/mailers/owners_test.rb +++ b/test/mailers/owners_test.rb @@ -14,10 +14,7 @@ class OwnersMailerTest < ActionMailer::TestCase email = OwnersMailer.with(ownership: @maintainer_ownership, authorizer: @owner).owner_updated assert_emails(1) { email.deliver_now } - assert_equal email.subject, "Your role was updated for #{@rubygem.name} gem" end - - should "" end end From 0bbbde046e25a482bcbe8db67a0abe4b09748e51 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Mon, 9 Sep 2024 16:30:13 +1000 Subject: [PATCH 16/28] Fix the wrong select values being passed to the edit/create ownership form --- app/views/owners/edit.html.erb | 2 +- app/views/owners/index.html.erb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/owners/edit.html.erb b/app/views/owners/edit.html.erb index bb29eddeea2..e1e7bfcd9d4 100644 --- a/app/views/owners/edit.html.erb +++ b/app/views/owners/edit.html.erb @@ -1,5 +1,5 @@ <% @title = t('.title') %> -<% @roles = Ownership.roles.transform_keys { |key| Ownership.human_attribute_name("role.#{key}") } %> +<% @roles = Ownership.roles.map { |k,_| [Ownership.human_attribute_name("role.#{k}"), k] } %> <%= form_tag rubygem_owner_path(rubygem_id: @rubygem.slug, owner_id: @ownership.user.display_id), method: :patch do |form| %> <%= error_messages_for(@ownership) %> diff --git a/app/views/owners/index.html.erb b/app/views/owners/index.html.erb index 2e0dae6f19e..957f5a24daa 100644 --- a/app/views/owners/index.html.erb +++ b/app/views/owners/index.html.erb @@ -1,6 +1,6 @@ <% @title = @rubygem.name %> <% @subtitle = t(".info", gem: @rubygem.name) %> -<% @roles = Ownership.roles.transform_keys { |key| Ownership.human_attribute_name("role.#{key}") } %> +<% @roles = Ownership.roles.map { |k,_| [Ownership.human_attribute_name("role.#{k}"), k] } %>
    <%= render "owners_table" %> From e37bcc30f56bb2736763b1e4425639adb5152929 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Fri, 13 Sep 2024 04:59:08 +0000 Subject: [PATCH 17/28] Always email owners of any changes to their roles --- app/controllers/api/v1/owners_controller.rb | 1 - app/mailers/owners_mailer.rb | 3 +-- app/models/ownership.rb | 5 +++++ app/views/owners_mailer/owner_updated.html.erb | 2 +- app/views/owners_mailer/owner_updated.text.erb | 2 +- config/locales/en.yml | 4 ++-- test/mailers/owners_test.rb | 2 +- test/models/ownership_test.rb | 8 ++++++++ 8 files changed, 19 insertions(+), 8 deletions(-) diff --git a/app/controllers/api/v1/owners_controller.rb b/app/controllers/api/v1/owners_controller.rb index f4f9f6888de..6c85cc60b48 100644 --- a/app/controllers/api/v1/owners_controller.rb +++ b/app/controllers/api/v1/owners_controller.rb @@ -35,7 +35,6 @@ def update ownership = @rubygem.ownerships.find_by!(user: owner) if ownership.update(ownership_update_params) - OwnersMailer.with(ownership: ownership, authorizer: @api_key.user).owner_updated.deliver_later render plain: response_with_mfa_warning("Owner updated successfully.") else render plain: response_with_mfa_warning(ownership.errors.full_messages.to_sentence), status: :unprocessable_entity diff --git a/app/mailers/owners_mailer.rb b/app/mailers/owners_mailer.rb index fa9e8ecc639..2130ee0abcb 100644 --- a/app/mailers/owners_mailer.rb +++ b/app/mailers/owners_mailer.rb @@ -15,13 +15,12 @@ def ownership_confirmation(ownership) def owner_updated @ownership = params[:ownership] - @authorizer = params[:authorizer] @user = @ownership.user @rubygem = @ownership.rubygem mail( to: @user.email, - subject: t("mailer.owner_updated.subject", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY, owner_handle: @authorizer.display_handle) + subject: t("mailer.owner_updated.subject", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY) ) end diff --git a/app/models/ownership.rb b/app/models/ownership.rb index 733c22f4b51..0dcb4e0fabf 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -14,6 +14,7 @@ class Ownership < ApplicationRecord after_create :record_create_event after_update :record_confirmation_event, if: :saved_change_to_confirmed_at? after_update :record_role_updated_event, if: :saved_change_to_role? + after_update :notify_user_role_of_role_change, if: :saved_change_to_role? after_destroy :record_destroy_event scope :confirmed, -> { where.not(confirmed_at: nil) } @@ -127,4 +128,8 @@ def record_destroy_event owner_gid: user.to_gid, actor_gid: Current.user&.to_gid) end + + def notify_user_role_of_role_change + OwnersMailer.with(ownership: self).owner_updated.deliver_later + end end diff --git a/app/views/owners_mailer/owner_updated.html.erb b/app/views/owners_mailer/owner_updated.html.erb index 936eef230e1..9c7a8a1e286 100644 --- a/app/views/owners_mailer/owner_updated.html.erb +++ b/app/views/owners_mailer/owner_updated.html.erb @@ -9,7 +9,7 @@

    - <%= t("mailer.owner_updated.body_html", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %> + <%= t("mailer.owner_updated.body_html", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %>

     
    diff --git a/app/views/owners_mailer/owner_updated.text.erb b/app/views/owners_mailer/owner_updated.text.erb index 9ef71d27425..b68561ccfd8 100644 --- a/app/views/owners_mailer/owner_updated.text.erb +++ b/app/views/owners_mailer/owner_updated.text.erb @@ -1,5 +1,5 @@ <%= t("mailer.owner_updated.subtitle", user_handle: @user.handle) %> -<%= t("mailer.owner_updated.body_text", gem: @rubygem.name, authorizer: @authorizer.display_handle, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %> +<%= t("mailer.owner_updated.body_text", gem: @rubygem.name, host: Gemcutter::HOST_DISPLAY, role: Ownership.human_attribute_name("role.#{@ownership.role}")) %> <%= rubygem_owners_url(@rubygem.slug) %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 57e678dda62..5cfe0836281 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -343,8 +343,8 @@ en: subject: Your role was updated for %{gem} gem title: OWNER ROLE UPDATED subtitle: Hi %{user_handle}! - body_html: Your role was updated to %{role} for %{gem} gem by %{authorizer}. - body_text: Your role was updated to %{role} for %{gem} gem by %{authorizer}. + body_html: Your role was updated to %{role} for %{gem} gem. + body_text: Your role was updated to %{role} for %{gem} gem. ownerhip_request_closed: title: OWNERSHIP REQUEST subtitle: Hi %{handle}! diff --git a/test/mailers/owners_test.rb b/test/mailers/owners_test.rb index f1fc94ea358..a4bd094fd7d 100644 --- a/test/mailers/owners_test.rb +++ b/test/mailers/owners_test.rb @@ -11,7 +11,7 @@ class OwnersMailerTest < ActionMailer::TestCase context "#owner_updated" do should "include host in subject" do - email = OwnersMailer.with(ownership: @maintainer_ownership, authorizer: @owner).owner_updated + email = OwnersMailer.with(ownership: @maintainer_ownership).owner_updated assert_emails(1) { email.deliver_now } assert_equal email.subject, "Your role was updated for #{@rubygem.name} gem" diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index 3749169441c..3f03ebe744d 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -1,6 +1,8 @@ require "test_helper" class OwnershipTest < ActiveSupport::TestCase + include ActionMailer::TestHelper + should "be valid with factory" do assert_predicate build(:ownership), :valid? end @@ -147,6 +149,12 @@ class OwnershipTest < ActiveSupport::TestCase assert_equal "rubygem:owner:updated", event.tag assert_equal attributes, event.additional.attributes end + + should "enqueue the owner updated email" do + assert_enqueued_emails 1 do + @ownership.update!(role: :maintainer) + end + end end end From 2508f3400d1a9e554b89d14a1715a535533288c7 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Tue, 17 Sep 2024 06:13:40 +0000 Subject: [PATCH 18/28] Fixup breaking test --- test/functional/api/v1/owners_controller_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/api/v1/owners_controller_test.rb b/test/functional/api/v1/owners_controller_test.rb index 434ecc4dffb..751fd0a5138 100644 --- a/test/functional/api/v1/owners_controller_test.rb +++ b/test/functional/api/v1/owners_controller_test.rb @@ -992,11 +992,11 @@ def self.should_respond_to(format) end should "schedule an email for the updated user" do - patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :maintainer } + patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :owner } ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) - assert_enqueued_email_with OwnersMailer, :owner_updated, params: { ownership: ownership, authorizer: @owner } + assert_enqueued_email_with OwnersMailer, :owner_updated, params: { ownership: ownership } end context "when the role is invalid" do From 05e38410921469b3a7348a62b74a426c37cd8bac Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Tue, 17 Sep 2024 06:13:57 +0000 Subject: [PATCH 19/28] Remove tests that are no longer relevant --- test/models/ownership_test.rb | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index 3f03ebe744d..67921ff2306 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -289,15 +289,8 @@ class OwnershipTest < ActiveSupport::TestCase @ownership = create(:ownership) end - should "maps the ownership permission level to the role" do + should "set a default role" do assert_predicate @ownership, :owner? - assert_equal Access::OWNER, @ownership.read_attribute_before_type_cast("role") - end - - should "correctly serialize the value" do - ownership = Ownership.find(@ownership.id) - - assert_predicate ownership, :owner? end end From f766716678daba7f5c8a0093372ff0601d5b28b0 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Tue, 17 Sep 2024 06:24:15 +0000 Subject: [PATCH 20/28] Fixup migration file name --- ..._to_ownership.rb => 20240802151324_add_role_to_ownership.rb} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename db/migrate/{20240802151324_add_access_level_to_ownership.rb => 20240802151324_add_role_to_ownership.rb} (63%) diff --git a/db/migrate/20240802151324_add_access_level_to_ownership.rb b/db/migrate/20240802151324_add_role_to_ownership.rb similarity index 63% rename from db/migrate/20240802151324_add_access_level_to_ownership.rb rename to db/migrate/20240802151324_add_role_to_ownership.rb index 1b4e13189b1..236a6040c4d 100644 --- a/db/migrate/20240802151324_add_access_level_to_ownership.rb +++ b/db/migrate/20240802151324_add_role_to_ownership.rb @@ -1,4 +1,4 @@ -class AddAccessLevelToOwnership < ActiveRecord::Migration[7.1] +class AddRoleToOwnership < ActiveRecord::Migration[7.1] def change add_column :ownerships, :role, :integer, null: false, default: 70 # Access::OWNER end From 1c66eca20b32c71ec1ffb5f49ce10724006b130a Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Tue, 17 Sep 2024 06:34:46 +0000 Subject: [PATCH 21/28] add role to Membership --- app/models/membership.rb | 2 ++ config/locales/de.yml | 1 + config/locales/en.yml | 1 + config/locales/es.yml | 1 + config/locales/fr.yml | 1 + config/locales/ja.yml | 1 + config/locales/nl.yml | 1 + config/locales/pt-BR.yml | 1 + config/locales/zh-CN.yml | 1 + config/locales/zh-TW.yml | 1 + db/migrate/20240917042436_add_role_to_memberships.rb | 5 +++++ db/schema.rb | 3 ++- lib/access.rb | 4 +++- test/models/membership_test.rb | 6 ++++++ 14 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20240917042436_add_role_to_memberships.rb diff --git a/app/models/membership.rb b/app/models/membership.rb index c15753d8b9a..37a532fcb41 100644 --- a/app/models/membership.rb +++ b/app/models/membership.rb @@ -5,6 +5,8 @@ class Membership < ApplicationRecord scope :unconfirmed, -> { where(confirmed_at: nil) } scope :confirmed, -> { where.not(confirmed_at: nil) } + enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER, admin: Access::ADMIN }, validate: true, _default: :maintainer + def confirmed? !confirmed_at.nil? end diff --git a/config/locales/de.yml b/config/locales/de.yml index 2bc139e6cee..627a2adf4bb 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -75,6 +75,7 @@ de: password: Passwort ownership/role: owner: + admin: maintainer: api_key: oidc_api_key_role: OIDC-API-Schlüsselrolle diff --git a/config/locales/en.yml b/config/locales/en.yml index 5cfe0836281..afd16064cbe 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -72,6 +72,7 @@ en: password: Password ownership/role: owner: Owner + admin: Admin maintainer: Maintainer api_key: oidc_api_key_role: OIDC API Key Role diff --git a/config/locales/es.yml b/config/locales/es.yml index 6ba9ca6a1e7..7bf36aebc6d 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -73,6 +73,7 @@ es: password: Contraseña ownership/role: owner: + admin: maintainer: api_key: oidc_api_key_role: diff --git a/config/locales/fr.yml b/config/locales/fr.yml index d7d0230028f..522c1af96f9 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -73,6 +73,7 @@ fr: password: Mot de passe ownership/role: owner: + admin: maintainer: api_key: oidc_api_key_role: diff --git a/config/locales/ja.yml b/config/locales/ja.yml index 072af4a16a3..766301b0cb1 100644 --- a/config/locales/ja.yml +++ b/config/locales/ja.yml @@ -66,6 +66,7 @@ ja: password: パスワード ownership/role: owner: + admin: maintainer: api_key: oidc_api_key_role: OIDC APIキーのロール diff --git a/config/locales/nl.yml b/config/locales/nl.yml index 86e0c5bd776..72a9c9723d0 100644 --- a/config/locales/nl.yml +++ b/config/locales/nl.yml @@ -65,6 +65,7 @@ nl: password: Wachtwoord ownership/role: owner: + admin: maintainer: api_key: oidc_api_key_role: diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml index 2a4bfb06a42..30309f31284 100644 --- a/config/locales/pt-BR.yml +++ b/config/locales/pt-BR.yml @@ -72,6 +72,7 @@ pt-BR: password: Senha ownership/role: owner: + admin: maintainer: api_key: oidc_api_key_role: diff --git a/config/locales/zh-CN.yml b/config/locales/zh-CN.yml index 02005853b80..3d239b5ea14 100644 --- a/config/locales/zh-CN.yml +++ b/config/locales/zh-CN.yml @@ -67,6 +67,7 @@ zh-CN: password: 密码 ownership/role: owner: + admin: maintainer: api_key: oidc_api_key_role: diff --git a/config/locales/zh-TW.yml b/config/locales/zh-TW.yml index 5da9695e365..0aabca1a80e 100644 --- a/config/locales/zh-TW.yml +++ b/config/locales/zh-TW.yml @@ -66,6 +66,7 @@ zh-TW: password: 密碼 ownership/role: owner: + admin: maintainer: api_key: oidc_api_key_role: OIDC API 金鑰角色 diff --git a/db/migrate/20240917042436_add_role_to_memberships.rb b/db/migrate/20240917042436_add_role_to_memberships.rb new file mode 100644 index 00000000000..1a1d311325a --- /dev/null +++ b/db/migrate/20240917042436_add_role_to_memberships.rb @@ -0,0 +1,5 @@ +class AddRoleToMemberships < ActiveRecord::Migration[7.1] + def change + add_column :memberships, :role, :integer, null: false, default: 50 # Access::MAINTAINER + end +end diff --git a/db/schema.rb b/db/schema.rb index 63f31935d1d..fe03e03761d 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_08_02_151324) do +ActiveRecord::Schema[7.1].define(version: 2024_09_17_042436) do # These are extensions that must be enabled in order to support this database enable_extension "hstore" enable_extension "pgcrypto" @@ -337,6 +337,7 @@ t.datetime "confirmed_at", precision: nil t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.integer "role", default: 50, null: false t.index ["organization_id"], name: "index_memberships_on_organization_id" t.index ["user_id", "organization_id"], name: "index_memberships_on_user_id_and_organization_id", unique: true t.index ["user_id"], name: "index_memberships_on_user_id" diff --git a/lib/access.rb b/lib/access.rb index 934575a8e81..897015712c6 100644 --- a/lib/access.rb +++ b/lib/access.rb @@ -1,12 +1,14 @@ module Access MAINTAINER = 50 OWNER = 70 + ADMIN = 90 DEFAULT_ROLE = "owner".freeze ROLES = { "maintainer" => MAINTAINER, - "owner" => OWNER + "owner" => OWNER, + "admin" => ADMIN }.with_indifferent_access.freeze def self.flag_for_role(role) diff --git a/test/models/membership_test.rb b/test/models/membership_test.rb index 194cbde5da8..08d0bedce0b 100644 --- a/test/models/membership_test.rb +++ b/test/models/membership_test.rb @@ -22,4 +22,10 @@ class MembershipTest < ActiveSupport::TestCase assert_predicate(membership, :confirmed?) assert_equal(Membership.confirmed, [membership]) end + + should "have a default role" do + membership = Membership.create!(organization: @organization, user: @user) + + assert_predicate membership, :maintainer? + end end From 3374305751655a076ec540b177b1f02ff342c578 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Sat, 21 Sep 2024 13:29:42 -0700 Subject: [PATCH 22/28] Refactor Ownership policies to cover updating user's own ownership Make authorization failures more helpful on ownership changes. We were sometimes returning just the text 'Forbidden' when really we need to say that you can't edit your own role or explain the authorization problem, if possible, then redirect back to the rubygem page. --- app/controllers/api/v1/owners_controller.rb | 20 +++--- app/controllers/owners_controller.rb | 34 ++++----- app/models/ownership.rb | 4 +- app/policies/api/application_policy.rb | 9 +++ app/policies/api/ownership_policy.rb | 11 +++ app/policies/api/rubygem_policy.rb | 12 ++-- app/policies/ownership_policy.rb | 2 + .../ownership_confirmation.html.erb | 2 +- config/brakeman.ignore | 24 +++---- config/brakeman.yml | 2 +- .../api/v1/owners_controller_test.rb | 50 +++++++++---- test/functional/owners_controller_test.rb | 71 +++++++++---------- test/models/ownership_test.rb | 10 ++- test/policies/api/ownership_policy_test.rb | 31 ++++++++ test/policies/ownership_policy_test.rb | 19 ++--- 15 files changed, 189 insertions(+), 112 deletions(-) create mode 100644 app/policies/api/ownership_policy.rb create mode 100644 test/policies/api/ownership_policy_test.rb diff --git a/app/controllers/api/v1/owners_controller.rb b/app/controllers/api/v1/owners_controller.rb index 6c85cc60b48..ec4e6ed3f1c 100644 --- a/app/controllers/api/v1/owners_controller.rb +++ b/app/controllers/api/v1/owners_controller.rb @@ -15,9 +15,7 @@ def show def create authorize @rubygem, :add_owner? owner = User.find_by_name!(email_param) - role = params[:role] - ownership = @rubygem.ownerships.new(user: owner, authorizer: @api_key.user) - ownership.role = role if role.present? + ownership = @rubygem.ownerships.new(user: owner, authorizer: @api_key.user, **ownership_params) if ownership.save OwnersMailer.ownership_confirmation(ownership).deliver_later @@ -30,11 +28,16 @@ def create end def update - authorize @rubygem, :update_owner? - owner = User.find_by_name!(email_param) - ownership = @rubygem.ownerships.find_by!(user: owner) + owner = User.find_by_name(email_param) + ownership = @rubygem.ownerships.find_by(user: owner) if owner + if ownership + authorize(ownership) + else + authorize(@rubygem, :update_owner?) # don't leak presence of an email unless authorized + return render_not_found + end - if ownership.update(ownership_update_params) + if ownership.update(ownership_params) render plain: response_with_mfa_warning("Owner updated successfully.") else render plain: response_with_mfa_warning(ownership.errors.full_messages.to_sentence), status: :unprocessable_entity @@ -45,6 +48,7 @@ def destroy authorize @rubygem, :remove_owner? owner = User.find_by_name!(email_param) ownership = @rubygem.ownerships_including_unconfirmed.find_by!(user: owner) + if ownership.safe_destroy OwnersMailer.owner_removed(ownership.user_id, @api_key.user.id, ownership.rubygem_id).deliver_later render plain: response_with_mfa_warning("Owner removed successfully.") @@ -76,7 +80,7 @@ def email_param params.permit(:email).require(:email) end - def ownership_update_params + def ownership_params params.permit(:role) end end diff --git a/app/controllers/owners_controller.rb b/app/controllers/owners_controller.rb index 6c983913aae..1c393af0dfa 100644 --- a/app/controllers/owners_controller.rb +++ b/app/controllers/owners_controller.rb @@ -4,6 +4,11 @@ class OwnersController < ApplicationController before_action :find_rubygem, except: :confirm verify_session_before only: %i[index edit update create destroy] before_action :verify_mfa_requirement, only: %i[create edit update destroy] + before_action :find_ownership, only: %i[edit update destroy] + + rescue_from(Pundit::NotAuthorizedError) do |e| + redirect_to rubygem_path(@rubygem.slug), alert: e.policy.error + end def confirm ownership = Ownership.find_by!(token: token_params) @@ -33,8 +38,6 @@ def index end def edit - authorize @rubygem, :update_owner? - @ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params) end def create @@ -54,25 +57,15 @@ def create # the role is the only thing that can be updated. If more fields are added to the ownership # this action will need to be tweaked a bit def update - authorize @rubygem, :update_owner? - ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params) - - if ownership.user == current_user && update_params[:role] != ownership.role - # Don't allow the owner to change the access level of their own ownership - return redirect_to rubygem_owners_path(@rubygem.slug), alert: t(".update_current_user_role") - end - - if ownership.update(update_params) - OwnersMailer.with(ownership: ownership, authorizer: current_user).owner_updated.deliver_later - redirect_to rubygem_owners_path(ownership.rubygem.slug), notice: t(".success_notice", handle: ownership.user.name) + if @ownership.update(update_params) + OwnersMailer.with(ownership: @ownership, authorizer: current_user).owner_updated.deliver_later + redirect_to rubygem_owners_path(@ownership.rubygem.slug), notice: t(".success_notice", handle: @ownership.user.name) else - index_with_error ownership.errors.full_messages.to_sentence, :unprocessable_entity + index_with_error @ownership.errors.full_messages.to_sentence, :unprocessable_entity end end def destroy - authorize @rubygem, :remove_owner? - @ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle!(handle_params) if @ownership.safe_destroy OwnersMailer.owner_removed(@ownership.user_id, current_user.id, @ownership.rubygem_id).deliver_later redirect_to rubygem_owners_path(@ownership.rubygem.slug), notice: t(".removed_notice", owner_name: @ownership.owner_name) @@ -87,6 +80,15 @@ def verify_session_redirect_path rubygem_owners_url(params[:rubygem_id]) end + def find_ownership + @ownership = @rubygem.ownerships_including_unconfirmed.find_by_owner_handle(handle_params) + return authorize(@ownership) if @ownership + + predicate = params[:action] == "destroy" ? :remove_owner? : :update_owner? + authorize(@rubygem, predicate) + render_not_found + end + def token_params params.permit(:token).require(:token) end diff --git a/app/models/ownership.rb b/app/models/ownership.rb index 0dcb4e0fabf..44941dec8c1 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -32,8 +32,8 @@ def self.by_indexed_gem_name .order("rubygems.name ASC") end - def self.find_by_owner_handle!(handle) - joins(:user).find_by(users: { handle: handle }) || joins(:user).find_by!(users: { id: handle }) + def self.find_by_owner_handle(handle) + joins(:user).find_by(users: { handle: handle }) || joins(:user).find_by(users: { id: handle }) end def self.create_confirmed(rubygem, user, approver) diff --git a/app/policies/api/application_policy.rb b/app/policies/api/application_policy.rb index 3b69c8e5f12..3d7d9b7f873 100644 --- a/app/policies/api/application_policy.rb +++ b/app/policies/api/application_policy.rb @@ -42,10 +42,19 @@ def deny(error) false end + def api_policy!(record) + Pundit.policy!(api_key, [:api, record]) + end + def user_policy!(record) Pundit.policy!(api_key.user, record) end + def api_authorized?(record, action) + policy = api_policy!(record) + policy.send(action) || deny(policy.error) + end + def user_authorized?(record, action) policy = user_policy!(record) policy.send(action) || deny(policy.error) diff --git a/app/policies/api/ownership_policy.rb b/app/policies/api/ownership_policy.rb new file mode 100644 index 00000000000..464b2376bf0 --- /dev/null +++ b/app/policies/api/ownership_policy.rb @@ -0,0 +1,11 @@ +class Api::OwnershipPolicy < Api::ApplicationPolicy + class Scope < Api::ApplicationPolicy::Scope + end + + delegate :rubygem, to: :record + + def update? + api_authorized?(rubygem, :update_owner?) && + user_authorized?(record, :update?) + end +end diff --git a/app/policies/api/rubygem_policy.rb b/app/policies/api/rubygem_policy.rb index 368b2547d1b..690ede046c3 100644 --- a/app/policies/api/rubygem_policy.rb +++ b/app/policies/api/rubygem_policy.rb @@ -26,18 +26,18 @@ def add_owner? user_authorized?(rubygem, :add_owner?) end - def remove_owner? + def update_owner? user_api_key? && mfa_requirement_satisfied?(rubygem) && - api_key_scope?(:remove_owner, rubygem) && - user_authorized?(rubygem, :remove_owner?) + api_key_scope?(:update_owner, rubygem) && + user_authorized?(rubygem, :update_owner?) end - def update_owner? + def remove_owner? user_api_key? && mfa_requirement_satisfied?(rubygem) && - api_key_scope?(:update_owner, rubygem) && - user_authorized?(rubygem, :update_owner?) + api_key_scope?(:remove_owner, rubygem) && + user_authorized?(rubygem, :remove_owner?) end def configure_trusted_publishers? diff --git a/app/policies/ownership_policy.rb b/app/policies/ownership_policy.rb index d01f8d65385..ad31e15c265 100644 --- a/app/policies/ownership_policy.rb +++ b/app/policies/ownership_policy.rb @@ -9,8 +9,10 @@ def create? end def update? + return deny(t("owners.update.update_current_user_role")) if current_user?(record.user) policy!(user, rubygem).update_owner? end + alias edit? update? def destroy? policy!(user, rubygem).remove_owner? diff --git a/app/views/owners_mailer/ownership_confirmation.html.erb b/app/views/owners_mailer/ownership_confirmation.html.erb index 5e0f8548c4f..1d76dddd526 100644 --- a/app/views/owners_mailer/ownership_confirmation.html.erb +++ b/app/views/owners_mailer/ownership_confirmation.html.erb @@ -29,7 +29,7 @@
    - VERIFY + VERIFY
    diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 8fa1ee71005..2a902bfc302 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -3,18 +3,18 @@ { "warning_type": "Mass Assignment", "warning_code": 105, - "fingerprint": "8129b3f5dfead604b3d9f72a38e52ffe0a7273b2fa5cccc5e75c6066fddf3934", + "fingerprint": "85e397bacae5462238e8ce59c0fcd1045a414e603588ae313c5156c09a623fed", "check_name": "PermitAttributes", "message": "Potentially dangerous key allowed for mass assignment", - "file": "app/controllers/api/v1/owners_controller.rb", - "line": 80, + "file": "app/controllers/owners_controller.rb", + "line": 101, "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/", "code": "params.permit(:role)", "render_path": null, "location": { "type": "method", - "class": "Api::V1::OwnersController", - "method": "ownership_update_params" + "class": "OwnersController", + "method": "update_params" }, "user_input": ":role", "confidence": "Medium", @@ -26,18 +26,18 @@ { "warning_type": "Mass Assignment", "warning_code": 105, - "fingerprint": "85e397bacae5462238e8ce59c0fcd1045a414e603588ae313c5156c09a623fed", + "fingerprint": "b57c75e671196846b60667930fc3c1a4d02122b7ec3ae5ae0cf8cecc6c1d0b63", "check_name": "PermitAttributes", "message": "Potentially dangerous key allowed for mass assignment", - "file": "app/controllers/owners_controller.rb", - "line": 98, + "file": "app/controllers/api/v1/owners_controller.rb", + "line": 84, "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/", "code": "params.permit(:role)", "render_path": null, "location": { "type": "method", - "class": "OwnersController", - "method": "update_params" + "class": "Api::V1::OwnersController", + "method": "ownership_params" }, "user_input": ":role", "confidence": "Medium", @@ -47,6 +47,6 @@ "note": "" } ], - "updated": "2024-08-08 12:28:27 +1000", - "brakeman_version": "6.1.2" + "updated": "2024-09-21 16:16:50 -0700", + "brakeman_version": "6.2.1" } diff --git a/config/brakeman.yml b/config/brakeman.yml index d9492b31e1d..26a7bbdc235 100644 --- a/config/brakeman.yml +++ b/config/brakeman.yml @@ -7,5 +7,5 @@ :output_files: - reports/brakeman/brakeman.json - reports/brakeman/brakeman.html -:rails5: true +:rails7: true :github_repo: rubygems/rubygems.org diff --git a/test/functional/api/v1/owners_controller_test.rb b/test/functional/api/v1/owners_controller_test.rb index 751fd0a5138..e1164f85882 100644 --- a/test/functional/api/v1/owners_controller_test.rb +++ b/test/functional/api/v1/owners_controller_test.rb @@ -975,43 +975,65 @@ def self.should_respond_to(format) setup do @owner = create(:user) @maintainer = create(:user) - @rubygem = create(:rubygem) - - @owner_gem_ownership = create(:ownership, user: @owner, rubygem: @rubygem, role: :owner) - @maintainer_gem_ownership = create(:ownership, user: @maintainer, rubygem: @rubygem, role: :maintainer) + @rubygem = create(:rubygem, owners: [@owner]) @api_key = create(:api_key, key: "12223", scopes: %i[update_owner], owner: @owner, rubygem: @rubygem) @request.env["HTTP_AUTHORIZATION"] = "12223" end should "set the maintainer to a lower access level" do + ownership = create(:ownership, user: @maintainer, rubygem: @rubygem, role: :owner) + patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :maintainer } assert_response :success - assert_predicate @maintainer_gem_ownership.reload, :maintainer? + assert_predicate ownership.reload, :maintainer? + assert_enqueued_email_with OwnersMailer, :owner_updated, params: { ownership: ownership } end - should "schedule an email for the updated user" do - patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :owner } + context "when the current user is changing their own role" do + should "forbid changing the role" do + patch :update, params: { rubygem_id: @rubygem.slug, email: @owner.email, role: :maintainer } - ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) + ownership = @rubygem.ownerships.find_by(user: @owner) - assert_enqueued_email_with OwnersMailer, :owner_updated, params: { ownership: ownership } + assert_response :forbidden + assert_predicate ownership.reload, :owner? + end end context "when the role is invalid" do should "return a bad request response with the error message" do - patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid } + ownership = create(:ownership, user: @maintainer, rubygem: @rubygem, role: :maintainer) - respond_with :bad_request + patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid } + assert_response :unprocessable_entity assert_equal "Role is not included in the list", @response.body + assert_predicate ownership.reload, :maintainer? end + end - should "not update the user with the new role" do - patch :update, params: { rubygem_id: @rubygem.slug, email: @maintainer.email, role: :invalid } + context "when the owner is not found" do + context "when the update is authorized" do + should "return a not found response" do + patch :update, params: { rubygem_id: @rubygem.slug, email: "notauser", role: :owner } + + assert_response :not_found + assert_equal "Owner could not be found.", @response.body + end + end - assert_predicate @maintainer_gem_ownership.reload, :maintainer? + context "when the update is not authorized" do + should "return a forbidden response" do + @api_key = create(:api_key, key: "99999", scopes: %i[push_rubygem], owner: @owner) + @request.env["HTTP_AUTHORIZATION"] = "99999" + + patch :update, params: { rubygem_id: @rubygem.slug, email: "notauser", role: :owner } + + assert_response :forbidden + assert_equal "This API key cannot perform the specified action on this gem.", @response.body + end end end end diff --git a/test/functional/owners_controller_test.rb b/test/functional/owners_controller_test.rb index ce8e9d823a2..7cbc4e78dfd 100644 --- a/test/functional/owners_controller_test.rb +++ b/test/functional/owners_controller_test.rb @@ -39,11 +39,8 @@ class OwnersControllerTest < ActionController::TestCase get :index, params: { rubygem_id: @rubygem.name } end - should respond_with :forbidden - - should "render forbidden message" do - assert page.has_content?("Forbidden") - end + should redirect_to("gem info page") { rubygem_path(@rubygem.slug) } + should set_flash[:alert].to "Forbidden" end end @@ -167,7 +164,8 @@ class OwnersControllerTest < ActionController::TestCase post :create, params: { handle: @other_user.display_id, rubygem_id: @rubygem.name } end - should respond_with :forbidden + should redirect_to("gem info page") { rubygem_path(@rubygem.slug) } + should set_flash[:alert].to "Forbidden" should "not add other user as owner" do refute_includes @rubygem.owners_including_unconfirmed, @other_user @@ -192,6 +190,7 @@ class OwnersControllerTest < ActionController::TestCase delete :destroy, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id } end end + should redirect_to("ownership index") { rubygem_owners_path(@rubygem.slug) } should "remove the ownership record" do @@ -231,15 +230,10 @@ class OwnersControllerTest < ActionController::TestCase delete :destroy, params: { rubygem_id: @rubygem.name, handle: @last_owner.display_id } end end - should respond_with :forbidden + should set_flash.now[:alert].to "Can't remove the only owner of the gem" should "not remove the ownership record" do assert_includes @rubygem.owners_including_unconfirmed, @last_owner - end - should "should flash error" do - assert_equal "Can't remove the only owner of the gem", flash[:alert] - end - should "not send email notifications about owner removal" do assert_emails 0 end end @@ -293,7 +287,7 @@ class OwnersControllerTest < ActionController::TestCase delete :destroy, params: { rubygem_id: @rubygem.name, handle: @last_owner.display_id } end - should respond_with :forbidden + should redirect_to("gem info page") { rubygem_path(@rubygem.slug) } should "not remove user as owner" do assert_includes @rubygem.owners, @last_owner @@ -365,12 +359,25 @@ class OwnersControllerTest < ActionController::TestCase @rubygem = create(:rubygem, owners: [@owner, @maintainer]) verified_sign_in_as(@owner) + end - get :edit, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id } + context "when editing another owner's role" do + setup do + get :edit, params: { rubygem_id: @rubygem.name, handle: @maintainer.display_id } + end + + should respond_with :success + should render_template :edit end - should respond_with :success - should render_template :edit + context "when editing your own role" do + setup do + get :edit, params: { rubygem_id: @rubygem.name, handle: @owner.display_id } + end + + should redirect_to("gem info page") { rubygem_path(@rubygem.slug) } + should set_flash[:alert].to "Can't update your own Role" + end end context "on PATCH to update ownership" do @@ -393,11 +400,6 @@ class OwnersControllerTest < ActionController::TestCase ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) assert_predicate ownership, :maintainer? - end - - should "schedule an email for the updated user" do - ownership = Ownership.find_by(rubygem: @rubygem, user: @maintainer) - assert_enqueued_email_with OwnersMailer, :owner_updated, params: { ownership: ownership, authorizer: @owner } end end @@ -503,31 +505,26 @@ class OwnersControllerTest < ActionController::TestCase end end - context "on EDIT to owners" do + context "on GET to edit" do setup do @second_user = create(:user) @ownership = create(:ownership, :unconfirmed, rubygem: @rubygem, user: @second_user) - edit :edit, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id } - - should redirect_to("sessions#verify") { verify_session_path } - should use_before_action(:redirect_to_verify) - - should "show the edit form" do - assert_not_template :edit - end + get :edit, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id } end + + should redirect_to("sessions#verify") { verify_session_path } + should use_before_action(:redirect_to_verify) end - context "on UPDATE to owners" do + context "on PATCH to update" do setup do @second_user = create(:user) - @ownership = create(:ownership, :unconfirmed, rubygem: @rubygem, user: @second_user) - patch :update, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id, role: :owner } - - should "update the ownership record" do - assert_equal Access::MAINTAINER, @ownership.reload.access - end + @ownership = create(:ownership, :unconfirmed, rubygem: @rubygem, user: @second_user, role: :owner) + patch :update, params: { rubygem_id: @rubygem.name, handle: @second_user.display_id, role: :maintainer } end + + should redirect_to("sessions#verify") { verify_session_path } + should use_before_action(:redirect_to_verify) end end diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index 67921ff2306..857e62b717d 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -200,14 +200,12 @@ class OwnershipTest < ActiveSupport::TestCase end should "find owner by matching handle/id" do - assert_equal @ownership, @rubygem.ownerships.find_by_owner_handle!(@user.handle) - assert_equal @ownership, @rubygem.ownerships.find_by_owner_handle!(@user) + assert_equal @ownership, @rubygem.ownerships.find_by_owner_handle(@user.handle) + assert_equal @ownership, @rubygem.ownerships.find_by_owner_handle(@user) end - should "raise not found" do - assert_raise ActiveRecord::RecordNotFound do - @rubygem.ownerships.find_by_owner_handle!("wrong user") - end + should "return nil" do + assert_nil @rubygem.ownerships.find_by_owner_handle("wronguser") end end diff --git a/test/policies/api/ownership_policy_test.rb b/test/policies/api/ownership_policy_test.rb new file mode 100644 index 00000000000..d0641f6533e --- /dev/null +++ b/test/policies/api/ownership_policy_test.rb @@ -0,0 +1,31 @@ +require "test_helper" + +class Api::OwnershipPolicyTest < ApiPolicyTestCase + setup do + @owner = create(:user, handle: "owner") + @user = create(:user, handle: "user") + @rubygem = create(:rubygem, owners: [@owner]) + + @record = create(:ownership, rubygem: @rubygem, user: @user, authorizer: @owner) + + OwnershipPolicy.any_instance.stubs( + update?: true, + destroy?: true + ) + end + + def policy!(api_key) + Pundit.policy!(api_key, [:api, @record]) + end + + context "#update?" do + scope = :update_owner + action = :update? + + should_require_user_key scope, action + should_require_mfa scope, action + should_require_scope scope, action + should_require_rubygem_scope scope, action + should_delegate_to_policy scope, action, OwnershipPolicy + end +end diff --git a/test/policies/ownership_policy_test.rb b/test/policies/ownership_policy_test.rb index 601f8ab8fb8..2fa68b75d37 100644 --- a/test/policies/ownership_policy_test.rb +++ b/test/policies/ownership_policy_test.rb @@ -5,8 +5,8 @@ class OwnershipPolicyTest < ActiveSupport::TestCase @authorizer = FactoryBot.create(:user, handle: "owner") @maintainer = FactoryBot.create(:user, handle: "maintainer") @rubygem = FactoryBot.create(:rubygem, owners: [@authorizer], maintainers: [@maintainer]) - @confirmed_ownership = @rubygem.ownerships.first - @confirmed_maintainer_ownership = @maintainer.ownerships.first + @authorizer_ownership = @rubygem.ownerships.first + @maintainer_ownership = @maintainer.ownerships.first @unconfirmed_ownership = FactoryBot.build(:ownership, :unconfirmed, rubygem: @rubygem, authorizer: @authorizer) @unconfirmed_maintainer_ownership = FactoryBot.build(:ownership, :maintainer, rubygem: @rubygem, authorizer: @authorizer) @@ -18,24 +18,25 @@ def test_create assert_predicate Pundit.policy!(@authorizer, @unconfirmed_ownership), :create? refute_predicate Pundit.policy!(@invited, @unconfirmed_ownership), :create? refute_predicate Pundit.policy!(@user, @unconfirmed_ownership), :create? - refute_predicate Pundit.policy!(@maintainer, @confirmed_maintainer_ownership), :create? + refute_predicate Pundit.policy!(@maintainer, @maintainer_ownership), :create? refute_predicate Pundit.policy!(@maintainer, @unconfirmed_maintainer_ownership), :create? end def test_update - assert_predicate Pundit.policy!(@authorizer, @confirmed_ownership), :update? + assert_predicate Pundit.policy!(@authorizer, @maintainer_ownership), :update? + refute_predicate Pundit.policy!(@authorizer, @authorizer_ownership), :update? refute_predicate Pundit.policy!(@invited, @unconfirmed_ownership), :update? refute_predicate Pundit.policy!(@user, @unconfirmed_ownership), :update? refute_predicate Pundit.policy!(@user, @unconfirmed_maintainer_ownership), :update? - refute_predicate Pundit.policy!(@maintainer, @confirmed_maintainer_ownership), :update? + refute_predicate Pundit.policy!(@maintainer, @maintainer_ownership), :update? refute_predicate Pundit.policy!(@maintainer, @unconfirmed_maintainer_ownership), :update? end def test_destroy - assert_predicate Pundit.policy!(@authorizer, @confirmed_ownership), :destroy? - refute_predicate Pundit.policy!(@owner, @confirmed_ownership), :destroy? - refute_predicate Pundit.policy!(@user, @confirmed_ownership), :destroy? - refute_predicate Pundit.policy!(@user, @confirmed_maintainer_ownership), :destroy? + assert_predicate Pundit.policy!(@authorizer, @authorizer_ownership), :destroy? + refute_predicate Pundit.policy!(@maintainer, @authorizer_ownership), :destroy? + refute_predicate Pundit.policy!(@user, @authorizer_ownership), :destroy? + refute_predicate Pundit.policy!(@user, @maintainer_ownership), :destroy? refute_predicate Pundit.policy!(@user, @unconfirmed_maintainer_ownership), :destroy? end end From c5137390e5366c1d88a9848904c6f729b36afb8f Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Sat, 21 Sep 2024 16:10:48 -0700 Subject: [PATCH 23/28] Improve readability of role access list --- app/models/ownership.rb | 2 +- lib/access.rb | 4 ++++ test/lib/access_test.rb | 9 +++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models/ownership.rb b/app/models/ownership.rb index 44941dec8c1..481d74d5c4c 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -22,7 +22,7 @@ class Ownership < ApplicationRecord enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER }, validate: true, _default: :owner - scope :user_with_minimum_role, ->(user, role) { where(user: user, role: Access.flag_for_role(role)..) } + scope :user_with_minimum_role, ->(user, role) { where(user: user, role: Access.with_minimum_role(role)) } def self.by_indexed_gem_name select("ownerships.*, rubygems.name") diff --git a/lib/access.rb b/lib/access.rb index 897015712c6..12316b95d03 100644 --- a/lib/access.rb +++ b/lib/access.rb @@ -15,6 +15,10 @@ def self.flag_for_role(role) ROLES.fetch(role) end + def self.with_minimum_role(role) + Range.new(flag_for_role(role), nil) + end + def self.role_for_flag(flag) ROLES.key(flag)&.inquiry.tap do |role| raise ArgumentError, "Unknown role flag: #{flag}" if role.blank? diff --git a/test/lib/access_test.rb b/test/lib/access_test.rb index 0ac75fd2485..1173c5f54f6 100644 --- a/test/lib/access_test.rb +++ b/test/lib/access_test.rb @@ -28,4 +28,13 @@ class AccessTest < ActiveSupport::TestCase end end end + + context ".with_minimum_role" do + should "return the range of roles for a given permission flag" do + assert_equal Range.new(Access::OWNER, nil), Access.with_minimum_role("owner") + refute_includes Access.with_minimum_role("owner"), Access::MAINTAINER + assert_includes Access.with_minimum_role("owner"), Access::OWNER + assert_includes Access.with_minimum_role("owner"), Access::ADMIN + end + end end From b51fd5099f45729b23351fed8a01d372deac794a Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Tue, 17 Sep 2024 07:44:04 +0000 Subject: [PATCH 24/28] Add organization policy object --- app/policies/application_policy.rb | 4 ++ app/policies/organization_policy.rb | 48 +++++++++++++ test/factories/memberships.rb | 9 +++ test/factories/organizations.rb | 20 ++++++ test/policies/organization_policy_test.rb | 86 +++++++++++++++++++++++ 5 files changed, 167 insertions(+) create mode 100644 app/policies/organization_policy.rb create mode 100644 test/policies/organization_policy_test.rb diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index e999c1fac18..007d63fb28e 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -84,6 +84,10 @@ def rubygem_owned_by_with_role?(user, minimum_required_role:) rubygem.owned_by_with_role?(user, minimum_required_role) || deny(t(:forbidden)) end + def organization_member_with_role?(user, minimum_required_role:) + record.memberships.exists?(["user_id = ? AND role >= ?", user.id, minimum_required_role]) + end + def policy!(user, record) = Pundit.policy!(user, record) def user_policy!(record) = policy!(user, record) diff --git a/app/policies/organization_policy.rb b/app/policies/organization_policy.rb new file mode 100644 index 00000000000..b61f9bf5f47 --- /dev/null +++ b/app/policies/organization_policy.rb @@ -0,0 +1,48 @@ +class OrganizationPolicy < ApplicationPolicy + class Scope < ApplicationPolicy::Scope + end + + def show? + true + end + + def update? + organization_member_with_role?(user, minimum_required_role: Access::OWNER) + end + + def create? + true + end + + def add_gem? + organization_member_with_role?(user, minimum_required_role: Access::OWNER) + end + + def remove_gem? + organization_member_with_role?(user, minimum_required_role: Access::OWNER) + end + + def add_membership? + organization_member_with_role?(user, minimum_required_role: Access::OWNER) + end + + def update_membership? + organization_member_with_role?(user, minimum_required_role: Access::OWNER) + end + + def remove_membership? + organization_member_with_role?(user, minimum_required_role: Access::OWNER) + end + + def show_membership? + organization_member_with_role?(user, minimum_required_role: Access::MAINTAINER) + end + + def list_memberships? + organization_member_with_role?(user, minimum_required_role: Access::MAINTAINER) + end + + def destroy? + false # For now organizations cannot be deleted + end +end diff --git a/test/factories/memberships.rb b/test/factories/memberships.rb index 7b586fb30f4..5920d25336f 100644 --- a/test/factories/memberships.rb +++ b/test/factories/memberships.rb @@ -3,5 +3,14 @@ user organization confirmed_at { Time.zone.now } + role { :maintainer } + + trait :owner do + role { :owner } + end + + trait :admin do + role { :admin } + end end end diff --git a/test/factories/organizations.rb b/test/factories/organizations.rb index 1a520024daa..77155e05655 100644 --- a/test/factories/organizations.rb +++ b/test/factories/organizations.rb @@ -1,9 +1,29 @@ FactoryBot.define do factory :organization do + transient do + owners { [] } + admins { [] } + maintainers { [] } + end + handle name deleted_at { nil } + after(:create) do |organization, evaluator| + evaluator.owners.each do |user| + create(:membership, user: user, organization: organization, role: :owner) + end + + evaluator.admins.each do |user| + create(:membership, user: user, organization: organization, role: :admin) + end + + evaluator.maintainers.each do |user| + create(:membership, user: user, organization: organization, role: :maintainer) + end + end + trait :with_members do memberships { build_list(:membership, 2) } end diff --git a/test/policies/organization_policy_test.rb b/test/policies/organization_policy_test.rb new file mode 100644 index 00000000000..d65ac86769a --- /dev/null +++ b/test/policies/organization_policy_test.rb @@ -0,0 +1,86 @@ +require "test_helper" +class OrganisationPolicyTest < PolicyTestCase + setup do + @owner = create(:user) + @admin = create(:user) + @maintainer = create(:user) + @guest = create(:user) + @organization = create(:organization, owners: [@owner], admins: [@admin], maintainers: [@maintainer]) + end + + def policy!(user) + Pundit.policy!(user, @organization) + end + + context "#update?" do + should "only be authorized if the user is an owner" do + assert_authorized @owner, :update? + assert_authorized @admin, :update? + refute_authorized @maintainer, :update? + refute_authorized @guest, :update? + end + end + + context "add_gem?" do + should "only be authorized if the user is an owner" do + assert_authorized @owner, :add_gem? + assert_authorized @admin, :add_gem? + refute_authorized @maintainer, :add_gem? + refute_authorized @guest, :add_gem? + end + end + + context "#remove_gem?" do + should "only be authorized if the user is an owner" do + assert_authorized @owner, :remove_gem? + assert_authorized @admin, :remove_gem? + refute_authorized @maintainer, :remove_gem? + refute_authorized @guest, :remove_gem? + end + end + + context "#add_membership?" do + should "only be authorized if the user is an owner" do + assert_authorized @owner, :add_membership? + assert_authorized @admin, :add_membership? + refute_authorized @maintainer, :add_membership? + refute_authorized @guest, :add_membership? + end + end + + context "#update_membership?" do + should "only be authorized if the user is an owner" do + assert_authorized @owner, :update_membership? + assert_authorized @admin, :update_membership? + refute_authorized @maintainer, :update_membership? + refute_authorized @guest, :update_membership? + end + end + + context "#remove_membership?" do + should "only be authorized if the user is an owner" do + assert_authorized @owner, :remove_membership? + assert_authorized @admin, :remove_membership? + refute_authorized @maintainer, :remove_membership? + refute_authorized @guest, :remove_membership? + end + end + + context "#show_membership?" do + should "only be authorized if the user is an owner or maintainer" do + assert_authorized @owner, :show_membership? + assert_authorized @admin, :show_membership? + assert_authorized @maintainer, :show_membership? + refute_authorized @guest, :show_membership? + end + end + + context "#list_memberships?" do + should "only be authorized if the user is an owner or maintainer" do + assert_authorized @owner, :list_memberships? + assert_authorized @admin, :list_memberships? + assert_authorized @maintainer, :list_memberships? + refute_authorized @guest, :list_memberships? + end + end +end From 65788adc86ccc3e61023b9aa7edfb6cb8e0c3953 Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Mon, 23 Sep 2024 06:23:27 +0000 Subject: [PATCH 25/28] record the previous value when recording the change role event --- app/models/events/rubygem_event.rb | 5 ++++- app/models/ownership.rb | 6 ++++-- test/models/ownership_test.rb | 6 ++++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/app/models/events/rubygem_event.rb b/app/models/events/rubygem_event.rb index 2899611f6c0..d153a0515ab 100644 --- a/app/models/events/rubygem_event.rb +++ b/app/models/events/rubygem_event.rb @@ -59,12 +59,15 @@ class Events::RubygemEvent < ApplicationRecord attribute :owner_gid, :global_id end - OWNER_UPDATED = define_event "rubygem:owner:updated" do + OWNER_ROLE_UPDATED = define_event "rubygem:owner:updated" do attribute :owner, :string attribute :updated_by, :string attribute :actor_gid, :global_id attribute :owner_gid, :global_id + + attribute :previous_role, :string + attribute :current_role, :string end OWNER_REMOVED = define_event "rubygem:owner:removed" do diff --git a/app/models/ownership.rb b/app/models/ownership.rb index 481d74d5c4c..7f208d82e57 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -114,11 +114,13 @@ def record_confirmation_event end def record_role_updated_event - rubygem.record_event!(Events::RubygemEvent::OWNER_UPDATED, + rubygem.record_event!(Events::RubygemEvent::OWNER_ROLE_UPDATED, owner: user.display_handle, updated_by: Current.user&.display_handle, owner_gid: user.to_gid, - actor_gid: Current.user&.to_gid) + actor_gid: Current.user&.to_gid, + previous_role: role_previously_was, + current_role: role) end def record_destroy_event diff --git a/test/models/ownership_test.rb b/test/models/ownership_test.rb index 857e62b717d..cff5096734b 100644 --- a/test/models/ownership_test.rb +++ b/test/models/ownership_test.rb @@ -129,7 +129,7 @@ class OwnershipTest < ActiveSupport::TestCase context "when updating the role" do setup do @actor = create(:user) - @ownership = create(:ownership) + @ownership = create(:ownership, role: :owner) Current.user = @actor end @@ -143,7 +143,9 @@ class OwnershipTest < ActiveSupport::TestCase "owner" => @ownership.user.display_handle, "updated_by" => @actor.display_handle, "actor_gid" => @actor.to_gid, - "owner_gid" => @ownership.user.to_gid + "owner_gid" => @ownership.user.to_gid, + "previous_role" => "owner", + "current_role" => "maintainer" } assert_equal "rubygem:owner:updated", event.tag From 6f1e863b97a643be45a41e818dc57a62e347eb23 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Tue, 24 Sep 2024 13:58:11 -0700 Subject: [PATCH 26/28] Move method to OrganizationPolicy --- app/policies/application_policy.rb | 4 ---- app/policies/organization_policy.rb | 6 ++++++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 007d63fb28e..e999c1fac18 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -84,10 +84,6 @@ def rubygem_owned_by_with_role?(user, minimum_required_role:) rubygem.owned_by_with_role?(user, minimum_required_role) || deny(t(:forbidden)) end - def organization_member_with_role?(user, minimum_required_role:) - record.memberships.exists?(["user_id = ? AND role >= ?", user.id, minimum_required_role]) - end - def policy!(user, record) = Pundit.policy!(user, record) def user_policy!(record) = policy!(user, record) diff --git a/app/policies/organization_policy.rb b/app/policies/organization_policy.rb index b61f9bf5f47..249750d60bd 100644 --- a/app/policies/organization_policy.rb +++ b/app/policies/organization_policy.rb @@ -45,4 +45,10 @@ def list_memberships? def destroy? false # For now organizations cannot be deleted end + + private + + def organization_member_with_role?(user, minimum_required_role:) + record.memberships.exists?(["user_id = ? AND role >= ?", user.id, minimum_required_role]) + end end From 559946639d3eb6adf4d2953f9fceb9bc6fa3f770 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Tue, 24 Sep 2024 14:00:22 -0700 Subject: [PATCH 27/28] Fix new lint error --- app/models/membership.rb | 2 +- app/models/ownership.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/membership.rb b/app/models/membership.rb index 37a532fcb41..e1e73cf2a9c 100644 --- a/app/models/membership.rb +++ b/app/models/membership.rb @@ -5,7 +5,7 @@ class Membership < ApplicationRecord scope :unconfirmed, -> { where(confirmed_at: nil) } scope :confirmed, -> { where.not(confirmed_at: nil) } - enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER, admin: Access::ADMIN }, validate: true, _default: :maintainer + enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER, admin: Access::ADMIN }, validate: true, default: :maintainer def confirmed? !confirmed_at.nil? diff --git a/app/models/ownership.rb b/app/models/ownership.rb index 7f208d82e57..bbe1f3776c8 100644 --- a/app/models/ownership.rb +++ b/app/models/ownership.rb @@ -20,7 +20,7 @@ class Ownership < ApplicationRecord scope :confirmed, -> { where.not(confirmed_at: nil) } scope :unconfirmed, -> { where(confirmed_at: nil) } - enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER }, validate: true, _default: :owner + enum :role, { owner: Access::OWNER, maintainer: Access::MAINTAINER }, validate: true, default: :owner scope :user_with_minimum_role, ->(user, role) { where(user: user, role: Access.with_minimum_role(role)) } From 764b75c39c99f7af0752da2f4c6cab61e415b21a Mon Sep 17 00:00:00 2001 From: Colby Swandale <996377+colby-swandale@users.noreply.github.com> Date: Fri, 27 Sep 2024 12:51:12 +1000 Subject: [PATCH 28/28] Update app/models/events/rubygem_event.rb Co-authored-by: Martin Emde --- app/models/events/rubygem_event.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/events/rubygem_event.rb b/app/models/events/rubygem_event.rb index d153a0515ab..6f4895dd442 100644 --- a/app/models/events/rubygem_event.rb +++ b/app/models/events/rubygem_event.rb @@ -59,7 +59,7 @@ class Events::RubygemEvent < ApplicationRecord attribute :owner_gid, :global_id end - OWNER_ROLE_UPDATED = define_event "rubygem:owner:updated" do + OWNER_ROLE_UPDATED = define_event "rubygem:owner:role_updated" do attribute :owner, :string attribute :updated_by, :string