From f72c6bc5edaac5f4967a274259bc7558740a7dc0 Mon Sep 17 00:00:00 2001 From: Emma Lejeck Date: Sat, 31 Oct 2020 12:58:03 -0700 Subject: [PATCH 1/5] Add automatic RecordInvalid handling in GraphQL --- .../errors/active_record/record_invalid.rb | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 app/graphql/errors/active_record/record_invalid.rb diff --git a/app/graphql/errors/active_record/record_invalid.rb b/app/graphql/errors/active_record/record_invalid.rb new file mode 100644 index 0000000000..941bac120f --- /dev/null +++ b/app/graphql/errors/active_record/record_invalid.rb @@ -0,0 +1,20 @@ +module Errors + module ActiveRecord + class RecordInvalid + def self.graphql_error(error) + record = error.record + errors = record.errors.map do |attribute, message| + { + code: 'ValidationError', + message: record.errors.full_message(attribute, message), + path: ['attributes', attribute.to_s.camelize(:lower)] + } + end + + { + errors: errors + } + end + end + end +end From ab7eb9fdfe85422ce59fbb89e28db893f57c7041 Mon Sep 17 00:00:00 2001 From: Emma Lejeck Date: Sat, 31 Oct 2020 13:05:45 -0700 Subject: [PATCH 2/5] Add granting and revocation mutations for site permissions --- app/actions/accounts/grant_site_permission.rb | 17 +++++++++++ .../accounts/revoke_site_permission.rb | 17 +++++++++++ .../account/grant_site_permission.rb | 29 +++++++++++++++++++ .../account/revoke_site_permission.rb | 29 +++++++++++++++++++ app/graphql/types/mutation_type.rb | 1 + .../types/mutations/account_mutation.rb | 8 +++++ .../accounts/grant_site_permission_spec.rb | 22 ++++++++++++++ .../accounts/revoke_site_permission_spec.rb | 22 ++++++++++++++ 8 files changed, 145 insertions(+) create mode 100644 app/actions/accounts/grant_site_permission.rb create mode 100644 app/actions/accounts/revoke_site_permission.rb create mode 100644 app/graphql/mutations/account/grant_site_permission.rb create mode 100644 app/graphql/mutations/account/revoke_site_permission.rb create mode 100644 app/graphql/types/mutations/account_mutation.rb create mode 100644 spec/actions/accounts/grant_site_permission_spec.rb create mode 100644 spec/actions/accounts/revoke_site_permission_spec.rb diff --git a/app/actions/accounts/grant_site_permission.rb b/app/actions/accounts/grant_site_permission.rb new file mode 100644 index 0000000000..ca2d822e37 --- /dev/null +++ b/app/actions/accounts/grant_site_permission.rb @@ -0,0 +1,17 @@ +module Accounts + class GrantSitePermission < Action + class UnknownPermission < StandardError; end + + parameter :user, load: User, required: true + parameter :permission, required: true + + def call + raise UnknownPermission unless User.permissions.keys.include?(permission) + + user.permissions.set(permission) + user.save! + + { user: user, permissions: user.permissions } + end + end +end diff --git a/app/actions/accounts/revoke_site_permission.rb b/app/actions/accounts/revoke_site_permission.rb new file mode 100644 index 0000000000..d6fd5ad1e6 --- /dev/null +++ b/app/actions/accounts/revoke_site_permission.rb @@ -0,0 +1,17 @@ +module Accounts + class RevokeSitePermission < Action + class UnknownPermission < StandardError; end + + parameter :user, load: User, required: true + parameter :permission, required: true + + def call + raise UnknownPermission unless User.permissions.keys.include?(permission) + + user.permissions.unset(permission) + user.save! + + { user: user, permissions: user.permissions } + end + end +end diff --git a/app/graphql/mutations/account/grant_site_permission.rb b/app/graphql/mutations/account/grant_site_permission.rb new file mode 100644 index 0000000000..70852fec69 --- /dev/null +++ b/app/graphql/mutations/account/grant_site_permission.rb @@ -0,0 +1,29 @@ +class Mutations::Account::GrantSitePermission < Mutations::Base + argument :account, ID, + required: true, + description: 'Who to grant permissions to' + + argument :permission, Types::Enum::SitePermission, + required: true, + description: 'The permission to grant to this user' + + field :id, ID, null: false + field :permissions, [Types::Enum::SitePermission], null: false + + def load_account(value) + ::User.find(value) + end + + def authorized? + current_user.permissions.admin? + end + + def resolve(account:, permission:) + res = Accounts::GrantSitePermission.call(user: account, permission: permission.to_sym) + + { + id: res.user.id, + permissions: res.permissions + } + end +end diff --git a/app/graphql/mutations/account/revoke_site_permission.rb b/app/graphql/mutations/account/revoke_site_permission.rb new file mode 100644 index 0000000000..5b16107d23 --- /dev/null +++ b/app/graphql/mutations/account/revoke_site_permission.rb @@ -0,0 +1,29 @@ +class Mutations::Account::RevokeSitePermission < Mutations::Base + argument :account, ID, + required: true, + description: 'Who to grant permissions to' + + argument :permission, Types::Enum::SitePermission, + required: true, + description: 'The permission to grant to this user' + + field :id, ID, null: false + field :permissions, [Types::Enum::SitePermission], null: false + + def load_account(value) + ::User.find(value) + end + + def authorized? + current_user.permissions.admin? + end + + def resolve(account:, permission:) + res = Accounts::RevokeSitePermission.call(user: account, permission: permission.to_sym) + + { + id: res.user.id, + permissions: res.permissions + } + end +end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 9f181eb267..ebf7172bd7 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -1,5 +1,6 @@ class Types::MutationType < Types::BaseObject field :pro, Types::Mutations::ProMutation, null: false + field :account, Types::Mutations::AccountMutation, null: true field :anime, Types::Mutations::AnimeMutation, null: true field :episode, Types::Mutations::EpisodeMutation, null: true field :library_entry, Types::Mutations::LibraryEntryMutation, null: true diff --git a/app/graphql/types/mutations/account_mutation.rb b/app/graphql/types/mutations/account_mutation.rb new file mode 100644 index 0000000000..0740a03aaf --- /dev/null +++ b/app/graphql/types/mutations/account_mutation.rb @@ -0,0 +1,8 @@ +class Types::Mutations::AccountMutation < Types::BaseObject + field :grant_site_permission, + mutation: ::Mutations::Account::GrantSitePermission, + description: 'Grant a sitewide permission to a user' + field :revoke_site_permission, + mutation: ::Mutations::Account::RevokeSitePermission, + description: 'Revoke a sitewide permission for a user' +end diff --git a/spec/actions/accounts/grant_site_permission_spec.rb b/spec/actions/accounts/grant_site_permission_spec.rb new file mode 100644 index 0000000000..7790fc9379 --- /dev/null +++ b/spec/actions/accounts/grant_site_permission_spec.rb @@ -0,0 +1,22 @@ +require 'rails_helper' + +RSpec.describe Accounts::GrantSitePermission do + let(:user) { create(:user, permissions: %i[admin]) } + + context 'with a valid permission' do + it 'should return the user and their new permissions' do + res = described_class.call(user: user, permission: :community_mod) + expect(res.user.id).to eq(user.id) + expect(res.permissions).to be_set(:community_mod) + expect(res.permissions).to be_set(:admin) + end + end + + context 'with an invalid permission' do + it 'should throw UnknownPermission error' do + expect { + described_class.call(user: user, permission: :poopy) + }.to raise_exception(Accounts::GrantSitePermission::UnknownPermission) + end + end +end diff --git a/spec/actions/accounts/revoke_site_permission_spec.rb b/spec/actions/accounts/revoke_site_permission_spec.rb new file mode 100644 index 0000000000..e4c2a9cc83 --- /dev/null +++ b/spec/actions/accounts/revoke_site_permission_spec.rb @@ -0,0 +1,22 @@ +require 'rails_helper' + +RSpec.describe Accounts::RevokeSitePermission do + let(:user) { create(:user, permissions: %i[admin community_mod]) } + + context 'with a valid permission' do + it 'should return the user and their new permissions' do + res = described_class.call(user: user, permission: :community_mod) + expect(res.user.id).to eq(user.id) + expect(res.permissions).not_to be_set(:community_mod) + expect(res.permissions).to be_set(:admin) + end + end + + context 'with an invalid permission' do + it 'should throw UnknownPermission error' do + expect { + described_class.call(user: user, permission: :poopy) + }.to raise_exception(Accounts::RevokeSitePermission::UnknownPermission) + end + end +end From 32233c2569b3c3b5bf4ba5923380900cefe6869b Mon Sep 17 00:00:00 2001 From: Emma Lejeck Date: Sun, 1 Nov 2020 12:57:53 -0800 Subject: [PATCH 3/5] Unify grant/revoke mutations and use an input type --- app/actions/accounts/grant_site_permission.rb | 17 ----------- .../accounts/revoke_site_permission.rb | 17 ----------- app/actions/accounts/set_site_permissions.rb | 20 +++++++++++++ .../account/grant_site_permission.rb | 29 ------------------- .../account/revoke_site_permission.rb | 29 ------------------- .../mutations/account/set_site_permissions.rb | 19 ++++++++++++ .../input/account/set_site_permissions.rb | 9 ++++++ .../types/mutations/account_mutation.rb | 9 ++---- .../accounts/grant_site_permission_spec.rb | 22 -------------- .../accounts/revoke_site_permission_spec.rb | 22 -------------- .../accounts/set_site_permissions_spec.rb | 23 +++++++++++++++ 11 files changed, 74 insertions(+), 142 deletions(-) delete mode 100644 app/actions/accounts/grant_site_permission.rb delete mode 100644 app/actions/accounts/revoke_site_permission.rb create mode 100644 app/actions/accounts/set_site_permissions.rb delete mode 100644 app/graphql/mutations/account/grant_site_permission.rb delete mode 100644 app/graphql/mutations/account/revoke_site_permission.rb create mode 100644 app/graphql/mutations/account/set_site_permissions.rb create mode 100644 app/graphql/types/input/account/set_site_permissions.rb delete mode 100644 spec/actions/accounts/grant_site_permission_spec.rb delete mode 100644 spec/actions/accounts/revoke_site_permission_spec.rb create mode 100644 spec/actions/accounts/set_site_permissions_spec.rb diff --git a/app/actions/accounts/grant_site_permission.rb b/app/actions/accounts/grant_site_permission.rb deleted file mode 100644 index ca2d822e37..0000000000 --- a/app/actions/accounts/grant_site_permission.rb +++ /dev/null @@ -1,17 +0,0 @@ -module Accounts - class GrantSitePermission < Action - class UnknownPermission < StandardError; end - - parameter :user, load: User, required: true - parameter :permission, required: true - - def call - raise UnknownPermission unless User.permissions.keys.include?(permission) - - user.permissions.set(permission) - user.save! - - { user: user, permissions: user.permissions } - end - end -end diff --git a/app/actions/accounts/revoke_site_permission.rb b/app/actions/accounts/revoke_site_permission.rb deleted file mode 100644 index d6fd5ad1e6..0000000000 --- a/app/actions/accounts/revoke_site_permission.rb +++ /dev/null @@ -1,17 +0,0 @@ -module Accounts - class RevokeSitePermission < Action - class UnknownPermission < StandardError; end - - parameter :user, load: User, required: true - parameter :permission, required: true - - def call - raise UnknownPermission unless User.permissions.keys.include?(permission) - - user.permissions.unset(permission) - user.save! - - { user: user, permissions: user.permissions } - end - end -end diff --git a/app/actions/accounts/set_site_permissions.rb b/app/actions/accounts/set_site_permissions.rb new file mode 100644 index 0000000000..ba35fad0e0 --- /dev/null +++ b/app/actions/accounts/set_site_permissions.rb @@ -0,0 +1,20 @@ +module Accounts + class SetSitePermissions < Action + class UnknownPermissions < StandardError; end + + parameter :user, load: User, required: true + parameter :permissions, required: true + + def call + sym_permissions = permissions.map(&:to_sym) + + unknown_permissions = sym_permissions - User.permissions.keys + raise UnknownPermissions, unknown_permissions if unknown_permissions.present? + + user.permissions = sym_permissions + user.save! + + { user: user, permissions: user.permissions } + end + end +end diff --git a/app/graphql/mutations/account/grant_site_permission.rb b/app/graphql/mutations/account/grant_site_permission.rb deleted file mode 100644 index 70852fec69..0000000000 --- a/app/graphql/mutations/account/grant_site_permission.rb +++ /dev/null @@ -1,29 +0,0 @@ -class Mutations::Account::GrantSitePermission < Mutations::Base - argument :account, ID, - required: true, - description: 'Who to grant permissions to' - - argument :permission, Types::Enum::SitePermission, - required: true, - description: 'The permission to grant to this user' - - field :id, ID, null: false - field :permissions, [Types::Enum::SitePermission], null: false - - def load_account(value) - ::User.find(value) - end - - def authorized? - current_user.permissions.admin? - end - - def resolve(account:, permission:) - res = Accounts::GrantSitePermission.call(user: account, permission: permission.to_sym) - - { - id: res.user.id, - permissions: res.permissions - } - end -end diff --git a/app/graphql/mutations/account/revoke_site_permission.rb b/app/graphql/mutations/account/revoke_site_permission.rb deleted file mode 100644 index 5b16107d23..0000000000 --- a/app/graphql/mutations/account/revoke_site_permission.rb +++ /dev/null @@ -1,29 +0,0 @@ -class Mutations::Account::RevokeSitePermission < Mutations::Base - argument :account, ID, - required: true, - description: 'Who to grant permissions to' - - argument :permission, Types::Enum::SitePermission, - required: true, - description: 'The permission to grant to this user' - - field :id, ID, null: false - field :permissions, [Types::Enum::SitePermission], null: false - - def load_account(value) - ::User.find(value) - end - - def authorized? - current_user.permissions.admin? - end - - def resolve(account:, permission:) - res = Accounts::RevokeSitePermission.call(user: account, permission: permission.to_sym) - - { - id: res.user.id, - permissions: res.permissions - } - end -end diff --git a/app/graphql/mutations/account/set_site_permissions.rb b/app/graphql/mutations/account/set_site_permissions.rb new file mode 100644 index 0000000000..ba475f82e5 --- /dev/null +++ b/app/graphql/mutations/account/set_site_permissions.rb @@ -0,0 +1,19 @@ +class Mutations::Account::SetSitePermissions < Mutations::Base + argument :input, Types::Input::Account::SetSitePermissions, + required: true + + field :account, Types::Account, null: false + + def authorized? + current_user.permissions.admin? + end + + def resolve(input:) + account = User.find(input[:account_id]) + permissions = input[:permissions].map(&:to_sym) + + res = Accounts::SetSitePermissions.call(user: account, permissions: permissions) + + { account: res.user } + end +end diff --git a/app/graphql/types/input/account/set_site_permissions.rb b/app/graphql/types/input/account/set_site_permissions.rb new file mode 100644 index 0000000000..ad53351b33 --- /dev/null +++ b/app/graphql/types/input/account/set_site_permissions.rb @@ -0,0 +1,9 @@ +class Types::Input::Account::SetSitePermissions < Types::Input::Base + argument :account_id, ID, + required: true, + description: 'Who to grant permissions to' + + argument :permissions, [Types::Enum::SitePermission], + required: true, + description: 'The permissions to set for this user' +end diff --git a/app/graphql/types/mutations/account_mutation.rb b/app/graphql/types/mutations/account_mutation.rb index 0740a03aaf..4a85873510 100644 --- a/app/graphql/types/mutations/account_mutation.rb +++ b/app/graphql/types/mutations/account_mutation.rb @@ -1,8 +1,5 @@ class Types::Mutations::AccountMutation < Types::BaseObject - field :grant_site_permission, - mutation: ::Mutations::Account::GrantSitePermission, - description: 'Grant a sitewide permission to a user' - field :revoke_site_permission, - mutation: ::Mutations::Account::RevokeSitePermission, - description: 'Revoke a sitewide permission for a user' + field :set_site_permissions, + mutation: ::Mutations::Account::SetSitePermissions, + description: 'Set the sitewide permissions for a user' end diff --git a/spec/actions/accounts/grant_site_permission_spec.rb b/spec/actions/accounts/grant_site_permission_spec.rb deleted file mode 100644 index 7790fc9379..0000000000 --- a/spec/actions/accounts/grant_site_permission_spec.rb +++ /dev/null @@ -1,22 +0,0 @@ -require 'rails_helper' - -RSpec.describe Accounts::GrantSitePermission do - let(:user) { create(:user, permissions: %i[admin]) } - - context 'with a valid permission' do - it 'should return the user and their new permissions' do - res = described_class.call(user: user, permission: :community_mod) - expect(res.user.id).to eq(user.id) - expect(res.permissions).to be_set(:community_mod) - expect(res.permissions).to be_set(:admin) - end - end - - context 'with an invalid permission' do - it 'should throw UnknownPermission error' do - expect { - described_class.call(user: user, permission: :poopy) - }.to raise_exception(Accounts::GrantSitePermission::UnknownPermission) - end - end -end diff --git a/spec/actions/accounts/revoke_site_permission_spec.rb b/spec/actions/accounts/revoke_site_permission_spec.rb deleted file mode 100644 index e4c2a9cc83..0000000000 --- a/spec/actions/accounts/revoke_site_permission_spec.rb +++ /dev/null @@ -1,22 +0,0 @@ -require 'rails_helper' - -RSpec.describe Accounts::RevokeSitePermission do - let(:user) { create(:user, permissions: %i[admin community_mod]) } - - context 'with a valid permission' do - it 'should return the user and their new permissions' do - res = described_class.call(user: user, permission: :community_mod) - expect(res.user.id).to eq(user.id) - expect(res.permissions).not_to be_set(:community_mod) - expect(res.permissions).to be_set(:admin) - end - end - - context 'with an invalid permission' do - it 'should throw UnknownPermission error' do - expect { - described_class.call(user: user, permission: :poopy) - }.to raise_exception(Accounts::RevokeSitePermission::UnknownPermission) - end - end -end diff --git a/spec/actions/accounts/set_site_permissions_spec.rb b/spec/actions/accounts/set_site_permissions_spec.rb new file mode 100644 index 0000000000..09d6266950 --- /dev/null +++ b/spec/actions/accounts/set_site_permissions_spec.rb @@ -0,0 +1,23 @@ +require 'rails_helper' + +RSpec.describe Accounts::SetSitePermissions do + let(:user) { create(:user, permissions: %i[admin]) } + + context 'with valid permissions' do + it 'should return the user and their new permissions' do + res = described_class.call(user: user, permissions: [:community_mod]) + expect(res.user.id).to eq(user.id) + expect(res.permissions).to be_set(:community_mod) + expect(res.permissions).not_to be_set(:admin) + end + end + + context 'with an invalid permission' do + it 'should throw UnknownPermissions error' do + expect { + described_class.call(user: user, permissions: %i[poopy community_mod]) + }.to raise_exception(Accounts::SetSitePermissions::UnknownPermissions) + expect(user.reload.permissions).not_to be_set(:community_mod) + end + end +end From b31eccd02821fe7a448e232315887ba449b8e003 Mon Sep 17 00:00:00 2001 From: Emma Lejeck Date: Sun, 1 Nov 2020 13:06:02 -0800 Subject: [PATCH 4/5] Shove the auth check into the UserPolicy --- .../mutations/account/set_site_permissions.rb | 12 +++++++----- .../types/input/account/set_site_permissions.rb | 7 ++++++- app/policies/user_policy.rb | 4 ++++ 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/app/graphql/mutations/account/set_site_permissions.rb b/app/graphql/mutations/account/set_site_permissions.rb index ba475f82e5..faf885897d 100644 --- a/app/graphql/mutations/account/set_site_permissions.rb +++ b/app/graphql/mutations/account/set_site_permissions.rb @@ -4,15 +4,17 @@ class Mutations::Account::SetSitePermissions < Mutations::Base field :account, Types::Account, null: false - def authorized? - current_user.permissions.admin? + def authorized?(input:) + super(input.account, :set_site_permissions?) end def resolve(input:) - account = User.find(input[:account_id]) - permissions = input[:permissions].map(&:to_sym) + permissions = input.permissions.map(&:to_sym) - res = Accounts::SetSitePermissions.call(user: account, permissions: permissions) + res = Accounts::SetSitePermissions.call( + user: input.account, + permissions: permissions + ) { account: res.user } end diff --git a/app/graphql/types/input/account/set_site_permissions.rb b/app/graphql/types/input/account/set_site_permissions.rb index ad53351b33..2bb1d66851 100644 --- a/app/graphql/types/input/account/set_site_permissions.rb +++ b/app/graphql/types/input/account/set_site_permissions.rb @@ -1,9 +1,14 @@ class Types::Input::Account::SetSitePermissions < Types::Input::Base argument :account_id, ID, required: true, - description: 'Who to grant permissions to' + description: 'Who to grant permissions to', + as: :account argument :permissions, [Types::Enum::SitePermission], required: true, description: 'The permissions to set for this user' + + def load_account(value) + ::User.find(value) + end end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 5ce50adad0..adc3ed81b9 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -10,6 +10,10 @@ def update? end alias_method :destroy?, :update? + def set_site_permissions? + user.permissions.admin? + end + def editable_attributes(all) if has_scope?(:email_password_reset, accept_all: false) [:password] From 62ad2f36f51f93bd214a4cb032f7960275a52f88 Mon Sep 17 00:00:00 2001 From: Emma Lejeck Date: Sun, 1 Nov 2020 13:08:41 -0800 Subject: [PATCH 5/5] Alphabetize mutation fields --- app/graphql/types/mutation_type.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index ebf7172bd7..8b6cc10d78 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -1,8 +1,8 @@ class Types::MutationType < Types::BaseObject - field :pro, Types::Mutations::ProMutation, null: false field :account, Types::Mutations::AccountMutation, null: true field :anime, Types::Mutations::AnimeMutation, null: true field :episode, Types::Mutations::EpisodeMutation, null: true field :library_entry, Types::Mutations::LibraryEntryMutation, null: true field :mapping, Types::Mutations::MappingMutation, null: true + field :pro, Types::Mutations::ProMutation, null: false end