diff --git a/app/controllers/api/ownership_transfer_resolutions_controller.rb b/app/controllers/api/ownership_transfer_resolutions_controller.rb index 2e483c5b1..ecc11d270 100644 --- a/app/controllers/api/ownership_transfer_resolutions_controller.rb +++ b/app/controllers/api/ownership_transfer_resolutions_controller.rb @@ -14,33 +14,41 @@ def decline resolve!(:rejected) end + def cancel + resolve!(:cancelled) + end + private - # Wrapped in a transaction so the row lock below is held across the - # read-and-update, preventing a concurrent accept/decline on the same - # transfer from also finding it pending. Authorizing against the loaded - # transfer (rather than checking cannot? by hand) means an unauthorized - # attempt raises and is rescued below into the same 404 a nonexistent - # transfer gets, instead of leaking that a pending transfer exists. + # The lock is held until update finishes, so two requests can't both act + # on the same pending transfer at once. + # + # Not found and not authorized both raise into the same head :not_found + # below, so the response can't be used to tell whether a transfer exists + # that the user just isn't allowed to touch. + # + # head/render happen after the transaction, not inside it, so a failure + # sending the cancellation email (its after_commit callback) is the only + # thing we respond with. def resolve!(status) - OwnershipTransfer.transaction do - transfer = pending_ownership_transfer - return head(:not_found) if transfer.blank? - - authorize!(action_name.to_sym, transfer) + transfer = OwnershipTransfer.transaction do + loaded = pending_ownership_transfer + authorize!(action_name.to_sym, loaded) + loaded.update(status:) + loaded + end - if transfer.update(status:) - head :ok - else - render json: { error: transfer.errors }, status: :unprocessable_content - end + if transfer.errors.empty? + head :ok + else + render json: { error: transfer.errors }, status: :unprocessable_content end - rescue CanCan::AccessDenied + rescue CanCan::AccessDenied, ActiveRecord::RecordNotFound head :not_found end def pending_ownership_transfer - @school.ownership_transfers.lock.pending.first + @school.ownership_transfers.lock.pending.first! end end end diff --git a/app/controllers/api/ownership_transfers_controller.rb b/app/controllers/api/ownership_transfers_controller.rb index 004d59841..b6de6c55f 100644 --- a/app/controllers/api/ownership_transfers_controller.rb +++ b/app/controllers/api/ownership_transfers_controller.rb @@ -11,7 +11,7 @@ def show if @ownership_transfer.blank? || cannot?(:read, @ownership_transfer) head :not_found - elsif current_user_is_requester? + elsif current_user_is_an_owner? render json: { status: @ownership_transfer.status, you_are: 'owner', nominee_name:, nominee_email: }, status: :ok else render json: { status: @ownership_transfer.status, you_are: 'nominee' }, status: :ok @@ -42,8 +42,8 @@ def most_recent_ownership_transfer @school.ownership_transfers.order(created_at: :desc).first end - def current_user_is_requester? - @ownership_transfer.requested_by_user_id == current_user.id + def current_user_is_an_owner? + current_user.school_owner?(@school) end def nominee_name diff --git a/app/mailers/school_ownership_mailer.rb b/app/mailers/school_ownership_mailer.rb index 1a7820ef4..730bbeac3 100644 --- a/app/mailers/school_ownership_mailer.rb +++ b/app/mailers/school_ownership_mailer.rb @@ -6,8 +6,8 @@ class SchoolOwnershipMailer < ApplicationMailer def request_ownership_transfer @school = ownership_transfer.school - @nominee_name = users_by_id[ownership_transfer.nominated_user_id]&.name - @requested_owner_name = users_by_id[ownership_transfer.requested_by_user_id]&.name + @nominee_name = users_by_id[ownership_transfer.nominated_user_id]&.name.presence || 'there' + @requested_owner_name = users_by_id[ownership_transfer.requested_by_user_id]&.name.presence || 'The school owner' mail(to: ownership_transfer.email_address, subject: "You've been nominated to be an owner of #{@school.name}", @@ -15,6 +15,16 @@ def request_ownership_transfer message_stream: 'outbound') end + def cancel_ownership_transfer + @school = ownership_transfer.school + @nominee_name = users_by_id[ownership_transfer.nominated_user_id]&.name.presence || 'there' + + mail(to: ownership_transfer.email_address, + subject: "The ownership nomination for #{@school.name} has been cancelled", + track_opens: 'true', + message_stream: 'outbound') + end + private def ownership_transfer diff --git a/app/models/ability.rb b/app/models/ability.rb index ffa3b7081..15f03622f 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -13,7 +13,7 @@ def initialize(user) user.schools.active.each do |school| define_school_student_abilities(user:, school:) if user.school_student?(school) define_school_teacher_abilities(user:, school:) if user.school_teacher?(school) - define_school_owner_abilities(user:, school:) if user.school_owner?(school) + define_school_owner_abilities(school:) if user.school_owner?(school) end define_editor_admin_abilities(user) @@ -68,7 +68,7 @@ def define_authenticated_non_student_abilities(user) can %i[read update destroy], Component, project: { user_id: user.id } end - def define_school_owner_abilities(user:, school:) + def define_school_owner_abilities(school:) can(%i[read update], School, id: school.id) can(%i[read], :school_member) can(%i[read create import update destroy regenerate_join_code], SchoolClass, school: { id: school.id }) @@ -83,7 +83,8 @@ def define_school_owner_abilities(user:, school:) can(%i[read create destroy], :school_owner) can(%i[read create destroy], :school_teacher) can(%i[read create], :ownership_transfer) - can(:read, OwnershipTransfer, school_id: school.id, requested_by_user_id: user.id) + can(:read, OwnershipTransfer, school_id: school.id) + can(:cancel, OwnershipTransfer, school_id: school.id, status: 'pending') can(%i[read create create_batch update destroy destroy_batch], :school_student) can(%i[create create_copy], Lesson, school_id: school.id) can(%i[read update destroy], Lesson, school_id: school.id, visibility: %w[teachers students public]) diff --git a/app/models/ownership_transfer.rb b/app/models/ownership_transfer.rb index 9556a9c98..53d33574e 100644 --- a/app/models/ownership_transfer.rb +++ b/app/models/ownership_transfer.rb @@ -19,6 +19,8 @@ class OwnershipTransfer < ApplicationRecord validate :nominee_has_the_school_teacher_role_for_the_school after_create_commit :send_ownership_transfer_request_email + after_update_commit :send_ownership_transfer_cancelled_email, + if: -> { saved_change_to_status?(from: 'pending', to: 'cancelled') } encrypts :email_address private @@ -35,4 +37,8 @@ def nominee_has_the_school_teacher_role_for_the_school def send_ownership_transfer_request_email SchoolOwnershipMailer.with(ownership_transfer: self).request_ownership_transfer.deliver_later end + + def send_ownership_transfer_cancelled_email + SchoolOwnershipMailer.with(ownership_transfer: self).cancel_ownership_transfer.deliver_later + end end diff --git a/app/views/school_ownership_mailer/cancel_ownership_transfer.text.erb b/app/views/school_ownership_mailer/cancel_ownership_transfer.text.erb new file mode 100644 index 000000000..88a0a9c59 --- /dev/null +++ b/app/views/school_ownership_mailer/cancel_ownership_transfer.text.erb @@ -0,0 +1,10 @@ +Hi <%= @nominee_name %>, + +The nomination for you to become the owner of the Code Classroom account for <%= @school.name %> has been cancelled by the school owner. + +No action is needed from you, and ownership remains unchanged. If you were expecting to take over this Code Classroom, please reach out to the school owner directly. + +If you have any questions, please contact us at websupport@raspberrypi.org. + +Kind Regards, +The Code Editor team diff --git a/config/routes.rb b/config/routes.rb index 86278b9ce..4da976888 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -90,6 +90,7 @@ resource :ownership_transfer, only: %i[show create], controller: 'ownership_transfers' do put :accept, to: 'ownership_transfer_resolutions#accept' put :decline, to: 'ownership_transfer_resolutions#decline' + put :cancel, to: 'ownership_transfer_resolutions#cancel' end resources :students, only: %i[index create update destroy], controller: 'school_students' do post :batch, on: :collection, to: 'school_students#create_batch' diff --git a/lib/user_info_api_client.rb b/lib/user_info_api_client.rb index 74f96370b..ed415d21e 100644 --- a/lib/user_info_api_client.rb +++ b/lib/user_info_api_client.rb @@ -13,7 +13,7 @@ def fetch_by_ids(user_ids) r.url '/users' r.body = { userIds: user_ids } end - return if response.body.blank? + return [] if response.body.blank? transform_result(response.body.fetch('users', [])) end diff --git a/spec/features/ownership_transfer/cancelling_an_ownership_transfer_spec.rb b/spec/features/ownership_transfer/cancelling_an_ownership_transfer_spec.rb new file mode 100644 index 000000000..d00db9588 --- /dev/null +++ b/spec/features/ownership_transfer/cancelling_an_ownership_transfer_spec.rb @@ -0,0 +1,141 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Cancelling an ownership transfer', type: :request do + include ActionMailer::TestHelper + + include_context 'with a school owner and nominated teacher' + + it 'responds 401 Unauthorized when no token is given' do + put("/api/schools/#{school.id}/ownership_transfer/cancel") + expect(response).to have_http_status(:unauthorized) + end + + it 'responds 403 Forbidden when the user is a school-student' do + student = create(:student, school:) + authenticated_in_hydra_as(student) + + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the user is the owner of a different school' do + other_owner = create(:owner, school: create(:verified_school)) + authenticated_in_hydra_as(other_owner) + + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(response).to have_http_status(:forbidden) + end + + context 'when the school has never had an ownership transfer' do + before { authenticated_in_hydra_as(owner) } + + it 'responds 404 Not Found' do + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(response).to have_http_status(:not_found) + end + end + + context 'when there is a pending transfer for the school' do + let!(:ownership_transfer) do + create( + :ownership_transfer, + school:, + nominated_user_id: nominee.id, + requested_by_user_id: owner.id, + email_address: nominee.email + ) + end + + context 'when the current user is the owner who requested the transfer' do + before { authenticated_in_hydra_as(owner) } + + it 'responds 200 OK' do + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(response).to have_http_status(:ok) + end + + it 'marks the transfer as cancelled' do + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(ownership_transfer.reload.status).to eq('cancelled') + end + + it 'sends the cancellation email' do + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + + assert_enqueued_email_with( + SchoolOwnershipMailer, :cancel_ownership_transfer, params: { ownership_transfer: } + ) + end + end + + context 'when the current user is a different owner of the same school' do + let(:other_owner) { create(:owner, school:) } + + before { authenticated_in_hydra_as(other_owner) } + + it 'responds 200 OK, since any current owner can cancel, not only the one who requested it' do + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(response).to have_http_status(:ok) + end + end + + context 'when the current user is the nominee' do + before { authenticated_in_hydra_as(nominee) } + + it 'responds 404 Not Found, since only an owner of the school can cancel' do + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(response).to have_http_status(:not_found) + end + + it 'does not change the transfer status' do + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(ownership_transfer.reload.status).to eq('pending') + end + end + + context 'when the current user is a different teacher at the school' do + let(:other_teacher) { create(:teacher, school:) } + + before { authenticated_in_hydra_as(other_teacher) } + + it 'responds 404 Not Found' do + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(response).to have_http_status(:not_found) + end + end + + context 'when the transfer is no longer pending' do + before do + ownership_transfer.update!(status: :completed) + authenticated_in_hydra_as(owner) + end + + it 'responds 404 Not Found' do + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + expect(response).to have_http_status(:not_found) + end + end + + context 'when the transfer has already been cancelled' do + before do + authenticated_in_hydra_as(owner) + put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:) + end + + it 'allows the owner to start a new transfer' do + other_teacher = create(:teacher, school:) + stub_user_info_api_for(other_teacher) + + post( + "/api/schools/#{school.id}/ownership_transfer", + params: { ownership_transfer: { nominated_user_id: other_teacher.id } }, + headers: + ) + + expect(response).to have_http_status(:created) + end + end + end +end diff --git a/spec/features/ownership_transfer/viewing_ownership_transfer_status_spec.rb b/spec/features/ownership_transfer/viewing_ownership_transfer_status_spec.rb index cf03b3f22..54b8b035f 100644 --- a/spec/features/ownership_transfer/viewing_ownership_transfer_status_spec.rb +++ b/spec/features/ownership_transfer/viewing_ownership_transfer_status_spec.rb @@ -78,6 +78,25 @@ end end + context 'when the current user is a different owner of the same school' do + let(:other_owner) { create(:owner, school:) } + + before do + stub_user_info_api_for(nominee) + authenticated_in_hydra_as(other_owner) + end + + it 'responds 200 OK, identifying them as an owner even though they did not request the transfer' do + get("/api/schools/#{school.id}/ownership_transfer", headers:) + + expect(response).to have_http_status(:ok) + json = JSON.parse(response.body) + expect(json).to include( + 'status' => 'pending', 'you_are' => 'owner', 'nominee_name' => nominee.name, 'nominee_email' => nominee.email + ) + end + end + context 'when the current user is the nominee' do before { authenticated_in_hydra_as(nominee) } @@ -155,6 +174,22 @@ end end + context 'when the transfer was cancelled by the owner' do + before do + ownership_transfer.update!(status: :cancelled) + stub_user_info_api_for(nominee) + authenticated_in_hydra_as(owner) + end + + it 'responds 200 OK, still visible to the owner who cancelled it' do + get("/api/schools/#{school.id}/ownership_transfer", headers:) + + expect(response).to have_http_status(:ok) + json = JSON.parse(response.body) + expect(json).to include('status' => 'cancelled', 'you_are' => 'owner', 'nominee_name' => nominee.name) + end + end + context 'when a resolved transfer is viewed by someone who was never involved' do let(:other_teacher) { create(:teacher, school:) } diff --git a/spec/lib/user_info_api_client_spec.rb b/spec/lib/user_info_api_client_spec.rb new file mode 100644 index 000000000..b4b993898 --- /dev/null +++ b/spec/lib/user_info_api_client_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe UserInfoApiClient do + describe '.fetch_by_ids' do + subject(:result) { described_class.fetch_by_ids(user_ids) } + + let(:user_ids) { [SecureRandom.uuid] } + + it 'returns an empty Array when the ids are blank' do + expect(described_class.fetch_by_ids([])).to eq [] + end + + context 'when the API responds with a blank body' do + before do + stub_request(:get, "#{described_class::API_URL}/users") + .with(headers: { Authorization: "Bearer #{described_class::API_KEY}" }) + .to_return(status: 200, body: '') + end + + it 'returns an empty Array rather than nil' do + expect(result).to eq [] + end + end + end +end diff --git a/spec/mailers/previews/school_ownership_mailer_preview.rb b/spec/mailers/previews/school_ownership_mailer_preview.rb index cfa4de6d3..b5a215cc8 100644 --- a/spec/mailers/previews/school_ownership_mailer_preview.rb +++ b/spec/mailers/previews/school_ownership_mailer_preview.rb @@ -17,6 +17,19 @@ def request_ownership_transfer with_stubbed_user_info_api { SchoolOwnershipMailer.with(ownership_transfer:).request_ownership_transfer.message } end + def cancel_ownership_transfer + school = School.new(name: 'Elmwood Secondary School') + ownership_transfer = OwnershipTransfer.new( + email_address: 'teacher@example.com', + school:, + nominated_user_id: NOMINEE[:id], + requested_by_user_id: REQUESTED_OWNER[:id], + status: :cancelled + ) + + with_stubbed_user_info_api { SchoolOwnershipMailer.with(ownership_transfer:).cancel_ownership_transfer.message } + end + private # fake the user info response, but only for the duration of diff --git a/spec/mailers/school_ownership_mailer_spec.rb b/spec/mailers/school_ownership_mailer_spec.rb index 7451a1f59..4f0c08f18 100644 --- a/spec/mailers/school_ownership_mailer_spec.rb +++ b/spec/mailers/school_ownership_mailer_spec.rb @@ -40,5 +40,85 @@ it 'includes the school name in the subject' do expect(email.subject).to include(ownership_transfer.school.name) end + + context 'when the nominee is missing from the user-info response' do + before do + stub_user_info_api_fetch_by_ids( + user_ids: [nominee.id, requested_owner.id], + users: [{ id: requested_owner.id, name: requested_owner.name }] + ) + end + + it 'greets them generically instead of leaving the greeting blank' do + expect(email.body.to_s).to include('Hi there,') + end + end + + context 'when the requested owner is missing from the user-info response' do + before do + stub_user_info_api_fetch_by_ids( + user_ids: [nominee.id, requested_owner.id], + users: [{ id: nominee.id, name: nominee.name }] + ) + end + + it 'falls back to a generic label instead of leaving it blank' do + expect(email.body.to_s).to include('The school owner') + end + end + end + + describe 'cancel_ownership_transfer' do + subject(:email) { described_class.with(ownership_transfer:).cancel_ownership_transfer } + + let(:school) { create(:verified_school) } + let(:nominee) { create(:teacher, school:) } + let(:requested_owner) { create(:owner, school:) } + let(:ownership_transfer) do + create( + :ownership_transfer, + school:, + nominated_user_id: nominee.id, + requested_by_user_id: requested_owner.id, + status: :cancelled + ) + end + + before do + stub_user_info_api_fetch_by_ids( + user_ids: [nominee.id, requested_owner.id], + users: [{ id: nominee.id, name: nominee.name }, { id: requested_owner.id, name: requested_owner.name }] + ) + end + + it 'includes the nominee name in the body' do + expect(email.body.to_s).to include(nominee.name) + end + + it 'attributes the cancellation to the school owner generically, not by name' do + expect(email.body.to_s).to include('the school owner') + expect(email.body.to_s).not_to include(requested_owner.name) + end + + it 'includes the school name in the body' do + expect(email.body.to_s).to include(ownership_transfer.school.name) + end + + it 'includes the school name in the subject' do + expect(email.subject).to include(ownership_transfer.school.name) + end + + context 'when the nominee is missing from the user-info response' do + before do + stub_user_info_api_fetch_by_ids( + user_ids: [nominee.id, requested_owner.id], + users: [{ id: requested_owner.id, name: requested_owner.name }] + ) + end + + it 'greets them generically instead of leaving the greeting blank' do + expect(email.body.to_s).to include('Hi there,') + end + end end end diff --git a/spec/models/ownership_transfer_spec.rb b/spec/models/ownership_transfer_spec.rb index 678d700f7..694d0438f 100644 --- a/spec/models/ownership_transfer_spec.rb +++ b/spec/models/ownership_transfer_spec.rb @@ -173,4 +173,38 @@ ) end end + + describe 'the cancellation email' do + before { ownership_transfer.save! } + + it 'is enqueued with the transfer as the mailer param when the transfer is cancelled' do + ownership_transfer.update!(status: :cancelled) + + assert_enqueued_email_with( + SchoolOwnershipMailer, :cancel_ownership_transfer, params: { ownership_transfer: } + ) + end + + it 'is not enqueued when the transfer resolves to a different status' do + assert_no_enqueued_emails do + ownership_transfer.update!(status: :completed) + end + end + + it 'is not enqueued when an already-cancelled transfer is saved again unchanged' do + ownership_transfer.update!(status: :cancelled) + + assert_no_enqueued_emails do + ownership_transfer.update!(status: :cancelled) + end + end + + it 'is not enqueued when a non-pending transfer is corrected to cancelled' do + ownership_transfer.update!(status: :completed) + + assert_no_enqueued_emails do + ownership_transfer.update!(status: :cancelled) + end + end + end end