From 71621ec512e044f9ca5f06193f810b882f859517 Mon Sep 17 00:00:00 2001 From: Kevin Sedgley Date: Mon, 8 Jan 2024 13:18:52 +0000 Subject: [PATCH 01/10] Update to mailcatcher (#886) Fix building of mailcatcher docker image for Ruby 2.7 --- docker/mailcatcher/Dockerfile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docker/mailcatcher/Dockerfile b/docker/mailcatcher/Dockerfile index d29cadecd..4481d6817 100644 --- a/docker/mailcatcher/Dockerfile +++ b/docker/mailcatcher/Dockerfile @@ -2,7 +2,8 @@ FROM ruby:2.7 ARG VERSION=0.8.2 -RUN gem install mailcatcher -v $VERSION +RUN gem install sqlite3 -v 1.6.9 && \ + gem install mailcatcher -v $VERSION EXPOSE 1025 1080 From 61c8f0dc25c8b5ba0996760c44891bc118c77f80 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 8 Jan 2024 20:38:27 +0000 Subject: [PATCH 02/10] Bump puma from 5.6.7 to 5.6.8 Bumps [puma](https://github.com/puma/puma) from 5.6.7 to 5.6.8. - [Release notes](https://github.com/puma/puma/releases) - [Changelog](https://github.com/puma/puma/blob/master/History.md) - [Commits](https://github.com/puma/puma/compare/v5.6.7...v5.6.8) --- updated-dependencies: - dependency-name: puma dependency-type: direct:production ... Signed-off-by: dependabot[bot] --- Gemfile.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 5fa679a6f..94cd9a272 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -225,7 +225,7 @@ GEM timeout net-smtp (0.4.0) net-protocol - nio4r (2.5.9) + nio4r (2.7.0) nokogiri (1.15.4) mini_portile2 (~> 2.8.2) racc (~> 1.4) @@ -240,7 +240,7 @@ GEM coderay (~> 1.1) method_source (~> 1.0) public_suffix (5.0.3) - puma (5.6.7) + puma (5.6.8) nio4r (~> 2.0) racc (1.7.1) rack (2.2.8) From 4be60495fa6237df2329766f2b32c31423c7ec88 Mon Sep 17 00:00:00 2001 From: Kevin Sedgley Date: Fri, 12 Jan 2024 11:39:48 +0000 Subject: [PATCH 03/10] Allow deletion of government responses (#888) Sometimes a response is posted and the department then changes its mind. --- .../government_response_controller.rb | 10 +++ .../admin/government_response_controller.rb | 10 +++ app/models/archived/government_response.rb | 8 ++ app/models/government_response.rb | 12 +++ ...tition_action_government_response.html.erb | 7 +- ...tition_action_government_response.html.erb | 7 +- config/locales/admin.en-GB.yml | 2 + config/routes.rb | 10 ++- .../government_response_controller_spec.rb | 73 ++++++++++++++++++ .../government_response_controller_spec.rb | 77 +++++++++++++++++++ spec/factories.rb | 5 ++ spec/jobs/delete_petition_job_spec.rb | 8 +- .../petitions/government_response_spec.rb | 4 +- 13 files changed, 221 insertions(+), 12 deletions(-) diff --git a/app/controllers/admin/archived/government_response_controller.rb b/app/controllers/admin/archived/government_response_controller.rb index c479541cf..79650c65e 100644 --- a/app/controllers/admin/archived/government_response_controller.rb +++ b/app/controllers/admin/archived/government_response_controller.rb @@ -26,6 +26,16 @@ def update end end + def destroy + @government_response.destroy + + if @government_response.destroyed? + redirect_to admin_archived_petition_path(@petition), notice: :government_response_deleted + else + redirect_to admin_archived_petition_government_response_path(@petition), alert: :government_response_not_deleted + end + end + private def fetch_petition diff --git a/app/controllers/admin/government_response_controller.rb b/app/controllers/admin/government_response_controller.rb index ec3c888d6..8f3e5f25d 100644 --- a/app/controllers/admin/government_response_controller.rb +++ b/app/controllers/admin/government_response_controller.rb @@ -26,6 +26,16 @@ def update end end + def destroy + @government_response.destroy + + if @government_response.destroyed? + redirect_to admin_petition_path(@petition), notice: :government_response_deleted + else + redirect_to admin_petition_government_response_path(@petition), alert: :government_response_not_deleted + end + end + private def fetch_petition diff --git a/app/models/archived/government_response.rb b/app/models/archived/government_response.rb index 0e8802da0..97873ff17 100644 --- a/app/models/archived/government_response.rb +++ b/app/models/archived/government_response.rb @@ -13,6 +13,14 @@ class GovernmentResponse < ActiveRecord::Base petition.touch(:government_response_at) unless petition.government_response_at? end + after_destroy do + # This prevents any enqueued email jobs from being sent + petition.set_email_requested_at_for("government_response") + + # This removes the petition from the 'Government response' list + petition.update_columns(government_response_at: nil) + end + def responded_on super || default_responded_on end diff --git a/app/models/government_response.rb b/app/models/government_response.rb index af9eaea70..af01a709a 100644 --- a/app/models/government_response.rb +++ b/app/models/government_response.rb @@ -11,6 +11,18 @@ class GovernmentResponse < ActiveRecord::Base petition.touch(:government_response_at) unless petition.government_response_at? end + after_destroy do + unless petition.archived? + Appsignal.increment_counter("petition.responded", -1) + + # This prevents any enqueued email jobs from being sent + petition.set_email_requested_at_for("government_response") + + # This removes the petition from the 'Government response' list + petition.update_columns(government_response_at: nil) + end + end + def responded_on super || default_responded_on end diff --git a/app/views/admin/archived/government_response/_petition_action_government_response.html.erb b/app/views/admin/archived/government_response/_petition_action_government_response.html.erb index 0193f41fd..600412287 100644 --- a/app/views/admin/archived/government_response/_petition_action_government_response.html.erb +++ b/app/views/admin/archived/government_response/_petition_action_government_response.html.erb @@ -21,8 +21,11 @@ <% end %> <%= email_petitioners_with_count_submit_button(f, petition) %> - <%= f.submit "Save", name: 'save', class: 'button-secondary' %> - <%= link_to 'Cancel', admin_archived_petition_path(@petition), class: 'button-secondary' %> + <% if @government_response.persisted? %> + <%= link_to "Delete", admin_archived_petition_government_response_path(@petition), class: "button-warning", method: :delete, data: { confirm: "Are you sure you want to delete the government response?" } %> + <% end %> + <%= f.submit "Save", name: "save", class: "button-secondary" %> + <%= link_to "Cancel", admin_archived_petition_path(@petition), class: "button-secondary" %> <% end %> <%= javascript_include_tag 'character-counter' %> diff --git a/app/views/admin/government_response/_petition_action_government_response.html.erb b/app/views/admin/government_response/_petition_action_government_response.html.erb index 442a8480c..0cddcc753 100644 --- a/app/views/admin/government_response/_petition_action_government_response.html.erb +++ b/app/views/admin/government_response/_petition_action_government_response.html.erb @@ -21,8 +21,11 @@ <% end %> <%= email_petitioners_with_count_submit_button(f, petition, disabled: @petition.editing_disabled?) %> - <%= f.submit "Save", name: 'save', class: 'button-secondary', disabled: @petition.editing_disabled? %> - <%= link_to 'Cancel', admin_petition_path(@petition), class: 'button-secondary' %> + <% if @government_response.persisted? %> + <%= link_to "Delete", admin_petition_government_response_path(@petition), class: "button-warning", method: :delete, data: { confirm: "Are you sure you want to delete the government response?" } %> + <% end %> + <%= f.submit "Save", name: "save", class: "button-secondary", disabled: @petition.editing_disabled? %> + <%= link_to "Cancel", admin_petition_path(@petition), class: "button-secondary" %> <% end %> <%= javascript_include_tag 'character-counter' %> diff --git a/config/locales/admin.en-GB.yml b/config/locales/admin.en-GB.yml index afed6feeb..55993e2f8 100644 --- a/config/locales/admin.en-GB.yml +++ b/config/locales/admin.en-GB.yml @@ -54,6 +54,8 @@ en-GB: email_resent_to_creator: "Resent the email to the petition creator and forwarded a copy to the feedback address" email_sent_overnight: "Email will be sent overnight" enqueued_petition_statistics_update: "Updating the petition statistics - please wait a few minutes and then refresh this page" + government_response_deleted: "Deleted government response successfully" + government_response_not_deleted: "Unable to delete government response - please contact support" government_response_updated: "Updated government response successfully" failed_tasks: "There was a problem starting the tasks - please contact support" invalidation_cant_be_cancelled: "Can't cancel invalidations that have completed" diff --git a/config/routes.rb b/config/routes.rb index 697461023..0f226623b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -152,7 +152,6 @@ scope only: %i[show update] do resource :debate_outcome, path: 'debate-outcome' - resource :government_response, path: 'government-response', controller: 'government_response' resource :notes resource :details, controller: 'petition_details' resource :schedule_debate, path: 'schedule-debate', controller: 'schedule_debate' @@ -163,6 +162,10 @@ resource :removal, controller: 'petition_removals' end + scope only: %i[show update destroy] do + resource :government_response, path: 'government-response', controller: 'government_response' + end + resources :signatures, only: %i[index destroy] do post :validate, :invalidate, on: :member post :subscribe, :unsubscribe, on: :member @@ -213,7 +216,6 @@ scope only: %i[show update] do resource :debate_outcome, path: 'debate-outcome' - resource :government_response, path: 'government-response', controller: 'government_response' resource :notes resource :details, controller: 'petition_details' resource :schedule_debate, path: 'schedule-debate', controller: 'schedule_debate' @@ -222,6 +224,10 @@ resource :topics, controller: 'petition_topics' resource :removal, controller: 'petition_removals' end + + scope only: %i[show update destroy] do + resource :government_response, path: 'government-response', controller: 'government_response' + end end resources :signatures, only: %i[index destroy] do diff --git a/spec/controllers/admin/archived/government_response_controller_spec.rb b/spec/controllers/admin/archived/government_response_controller_spec.rb index 7725d89a4..fac180081 100644 --- a/spec/controllers/admin/archived/government_response_controller_spec.rb +++ b/spec/controllers/admin/archived/government_response_controller_spec.rb @@ -610,5 +610,78 @@ def do_patch(overrides = {}) end end end + + describe 'DELETE /destroy' do + before do + expect(Archived::Petition).to receive_message_chain(:moderated, :find).with(petition.to_param).and_return(petition) + end + + context "when the petition has a government response" do + let!(:petition) { FactoryBot.create(:archived_petition, :response) } + + before do + expect(petition).to receive(:government_response).and_return(government_response) + end + + context "when the government response is succcessfully deleted" do + it "redirects to the petition page and displays a notice" do + delete :destroy, params: { petition_id: petition.id } + + expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/archived/petitions/#{petition.id}") + expect(controller).to set_flash[:notice].to("Deleted government response successfully") + end + + it "updates the email_requested_at timestamp for 'government_response'" do + email_requested_at = 1.hour.ago + petition.set_email_requested_at_for("government_response", to: email_requested_at) + + expect { + delete :destroy, params: { petition_id: petition.id } + }.to change { + petition.get_email_requested_at_for("government_response") + }.from(email_requested_at).to(be_within(1.second).of(Time.current)) + end + + it "updates the government_response_at timestamp to be nil" do + expect { + delete :destroy, params: { petition_id: petition.id } + }.to change { + petition.government_response_at + }.from(be_present).to(be_nil) + end + end + + context "when the government response is not successfully deleted" do + before do + expect(government_response).to receive(:destroy).and_return(false) + expect(government_response).to receive(:destroyed?).and_return(false) + end + + it "redirects to the government response page and displays an alert" do + delete :destroy, params: { petition_id: petition.id } + + expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/archived/petitions/#{petition.id}/government-response") + expect(controller).to set_flash[:alert].to("Unable to delete government response - please contact support") + end + end + end + + context "when the petition has no government response" do + let!(:petition) { FactoryBot.create(:archived_petition) } + let(:new_government_response) { petition.build_government_response } + + before do + expect(petition).to receive(:government_response).and_return(nil) + expect(petition).to receive(:build_government_response).and_return(new_government_response) + end + + it "redirects to the petition page and displays a notice" do + delete :destroy, params: { petition_id: petition.id } + + expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/archived/petitions/#{petition.id}") + expect(controller).to set_flash[:notice].to("Deleted government response successfully") + end + end + end end end diff --git a/spec/controllers/admin/government_response_controller_spec.rb b/spec/controllers/admin/government_response_controller_spec.rb index 978bfeb8b..17f7522ea 100644 --- a/spec/controllers/admin/government_response_controller_spec.rb +++ b/spec/controllers/admin/government_response_controller_spec.rb @@ -642,5 +642,82 @@ def do_patch(overrides = {}) end end end + + describe 'DELETE /destroy' do + before do + expect(Petition).to receive_message_chain(:moderated, :find).with(petition.to_param).and_return(petition) + end + + context "when the petition has a government response" do + let!(:petition) { FactoryBot.create(:responded_petition) } + + before do + expect(petition).to receive(:government_response).and_return(government_response) + end + + context "when the government response is succcessfully deleted" do + before do + expect(Appsignal).to receive(:increment_counter).with("petition.responded", -1) + end + + it "redirects to the petition page and displays a notice" do + delete :destroy, params: { petition_id: petition.id } + + expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/petitions/#{petition.id}") + expect(controller).to set_flash[:notice].to("Deleted government response successfully") + end + + it "updates the email_requested_at timestamp for 'government_response'" do + email_requested_at = 1.hour.ago + petition.set_email_requested_at_for("government_response", to: email_requested_at) + + expect { + delete :destroy, params: { petition_id: petition.id } + }.to change { + petition.get_email_requested_at_for("government_response") + }.from(email_requested_at).to(be_within(1.second).of(Time.current)) + end + + it "updates the government_response_at timestamp to be nil" do + expect { + delete :destroy, params: { petition_id: petition.id } + }.to change { + petition.government_response_at + }.from(be_present).to(be_nil) + end + end + + context "when the government response is not successfully deleted" do + before do + expect(government_response).to receive(:destroy).and_return(false) + expect(government_response).to receive(:destroyed?).and_return(false) + end + + it "redirects to the government response page and displays an alert" do + delete :destroy, params: { petition_id: petition.id } + + expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/petitions/#{petition.id}/government-response") + expect(controller).to set_flash[:alert].to("Unable to delete government response - please contact support") + end + end + end + + context "when the petition has no government response" do + let!(:petition) { FactoryBot.create(:open_petition) } + let(:new_government_response) { petition.build_government_response } + + before do + expect(petition).to receive(:government_response).and_return(nil) + expect(petition).to receive(:build_government_response).and_return(new_government_response) + end + + it "redirects to the petition page and displays a notice" do + delete :destroy, params: { petition_id: petition.id } + + expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/petitions/#{petition.id}") + expect(controller).to set_flash[:notice].to("Deleted government response successfully") + end + end + end end end diff --git a/spec/factories.rb b/spec/factories.rb index c4d03081a..355e89cb2 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -317,6 +317,11 @@ end end + trait :archived do + archived_at { 3.day.ago } + archiving_started_at { 4.days.ago } + end + trait :with_additional_details do additional_details { "Petition additional details" } end diff --git a/spec/jobs/delete_petition_job_spec.rb b/spec/jobs/delete_petition_job_spec.rb index 5cf363d5c..a15cd8846 100644 --- a/spec/jobs/delete_petition_job_spec.rb +++ b/spec/jobs/delete_petition_job_spec.rb @@ -7,7 +7,7 @@ end context "with a stopped petition" do - let!(:petition) { FactoryBot.create(:stopped_petition) } + let!(:petition) { FactoryBot.create(:stopped_petition, :archived) } it "destroys the petition" do expect { @@ -19,7 +19,7 @@ end context "with a closed petition" do - let!(:petition) { FactoryBot.create(:validated_petition, sponsors_signed: true, state: "closed", closed_at: 4.weeks.ago) } + let!(:petition) { FactoryBot.create(:closed_petition, :archived, sponsors_signed: true, closed_at: 4.weeks.ago) } let!(:country_petition_journal) { FactoryBot.create(:country_petition_journal, petition: petition) } let!(:constituency_petition_journal) { FactoryBot.create(:constituency_petition_journal, petition: petition) } @@ -136,7 +136,7 @@ end context "with a rejected petition" do - let!(:petition) { FactoryBot.create(:rejected_petition) } + let!(:petition) { FactoryBot.create(:rejected_petition, :archived) } it "destroys the petition" do expect { @@ -157,7 +157,7 @@ context "with a hidden petition" do let!(:user) { FactoryBot.create(:moderator_user) } - let!(:petition) { FactoryBot.create(:rejected_petition, rejection_code: "libellous", moderated_by: user) } + let!(:petition) { FactoryBot.create(:rejected_petition, :archived, rejection_code: "libellous", moderated_by: user) } it "destroys the petition" do expect { diff --git a/spec/routing/admin/petitions/government_response_spec.rb b/spec/routing/admin/petitions/government_response_spec.rb index 372647a07..436cc20c0 100644 --- a/spec/routing/admin/petitions/government_response_spec.rb +++ b/spec/routing/admin/petitions/government_response_spec.rb @@ -21,7 +21,7 @@ expect(patch("/admin/petitions/1/government-response")).to route_to('admin/government_response#update', petition_id: '1') end - it "doesn't route DELETE /admin/petitions/1/government-response" do - expect(delete("/admin/petitions/1/government-response")).not_to be_routable + it "routes DELETE /admin/petitions/1/government-response to admin/government_response#destroy" do + expect(delete("/admin/petitions/1/government-response")).to route_to('admin/government_response#destroy', petition_id: '1') end end From bc57bb6efa4de777a5e848d33a94de0848367a90 Mon Sep 17 00:00:00 2001 From: Kevin Sedgley Date: Fri, 19 Jan 2024 07:34:59 +0000 Subject: [PATCH 04/10] Track emails about parliamentary business (#889) * Track the number of emails sent * Track when the emails have finished enqueuing --- app/jobs/archive_petition_job.rb | 2 ++ app/jobs/archived/email_petitioners_job.rb | 7 +++++ .../email_all_petition_signatories.rb | 18 ++++++++++-- app/jobs/email_petitioners_job.rb | 7 +++++ app/models/archived/signature.rb | 4 +++ ...petition_action_email_petitioners.html.erb | 4 +++ ...petition_action_email_petitioners.html.erb | 4 +++ ...l_count_to_other_parliamentary_business.rb | 6 ++++ ...o_other_parliamentary_business_archived.rb | 6 ++++ db/schema.rb | 6 +++- spec/factories.rb | 11 +++++++- spec/jobs/archive_petition_job_spec.rb | 27 ++++++++++++++---- .../archived/email_petitioners_job_spec.rb | 28 +++++++++++++++++-- spec/jobs/email_petitioners_job_spec.rb | 26 ++++++++++++++++- spec/jobs/shared_examples.rb | 16 ++++++----- 15 files changed, 153 insertions(+), 19 deletions(-) create mode 100644 db/migrate/20240117134550_add_email_count_to_other_parliamentary_business.rb create mode 100644 db/migrate/20240117150804_add_email_count_to_other_parliamentary_business_archived.rb diff --git a/app/jobs/archive_petition_job.rb b/app/jobs/archive_petition_job.rb index 6165c1de1..b10e73557 100644 --- a/app/jobs/archive_petition_job.rb +++ b/app/jobs/archive_petition_job.rb @@ -75,6 +75,8 @@ def perform(petition) e.subject = email.subject e.body = email.body e.sent_by = email.sent_by + e.email_count = email.email_count + e.emails_enqueued_at = email.emails_enqueued_at e.created_at = email.created_at e.updated_at = email.updated_at end diff --git a/app/jobs/archived/email_petitioners_job.rb b/app/jobs/archived/email_petitioners_job.rb index 7901e6e73..ada6fa7dc 100644 --- a/app/jobs/archived/email_petitioners_job.rb +++ b/app/jobs/archived/email_petitioners_job.rb @@ -12,6 +12,13 @@ class EmailPetitionersJob < ApplicationJob log_exception(exception) end + after_enqueue_send_email_jobs do + email.update_columns( + email_count: petition.signatures.subscribers, + emails_enqueued_at: Time.current + ) + end + def perform(**args) @email = args[:email] super diff --git a/app/jobs/concerns/email_all_petition_signatories.rb b/app/jobs/concerns/email_all_petition_signatories.rb index f8febf1a8..6dc685777 100644 --- a/app/jobs/concerns/email_all_petition_signatories.rb +++ b/app/jobs/concerns/email_all_petition_signatories.rb @@ -13,6 +13,10 @@ module EmailAllPetitionSignatories attr_reader :petition, :requested_at, :scope queue_as :high_priority + + with_options(skip_after_callbacks_if_terminated: true) do + define_callbacks :enqueue_send_email_jobs + end end module ClassMethods @@ -23,6 +27,14 @@ def run_later_tonight(**args) set(wait_until: later_tonight).perform_later(**args.merge(requested_at: requested_at_iso8601)) end + def before_enqueue_send_email_jobs(*filters, &blk) + set_callback(:enqueue_send_email_jobs, :before, *filters, &blk) + end + + def after_enqueue_send_email_jobs(*filters, &blk) + set_callback(:enqueue_send_email_jobs, :after, *filters, &blk) + end + private def requested_at @@ -64,8 +76,10 @@ def perform(**args) # and enqueues a job to do the actual sending def enqueue_send_email_jobs Appsignal.without_instrumentation do - signatures_to_email.find_each do |signature| - email_delivery_job_class.perform_later(**mailer_arguments(signature)) + run_callbacks :enqueue_send_email_jobs do + signatures_to_email.find_each do |signature| + email_delivery_job_class.perform_later(**mailer_arguments(signature)) + end end end end diff --git a/app/jobs/email_petitioners_job.rb b/app/jobs/email_petitioners_job.rb index 764ec6c8e..11aecbd31 100644 --- a/app/jobs/email_petitioners_job.rb +++ b/app/jobs/email_petitioners_job.rb @@ -11,6 +11,13 @@ class EmailPetitionersJob < ApplicationJob log_exception(exception) end + after_enqueue_send_email_jobs do + email.update_columns( + email_count: petition.signatures.subscribers, + emails_enqueued_at: Time.current + ) + end + def perform(**args) @email = args[:email] super diff --git a/app/models/archived/signature.rb b/app/models/archived/signature.rb index 1cb34d5e7..6270dc67e 100644 --- a/app/models/archived/signature.rb +++ b/app/models/archived/signature.rb @@ -108,6 +108,10 @@ def subscribed where(notify_by_email: true) end + def subscribers + validated.subscribed.count + end + def validated where(state: VALIDATED_STATE) end diff --git a/app/views/admin/archived/petition_emails/_petition_action_email_petitioners.html.erb b/app/views/admin/archived/petition_emails/_petition_action_email_petitioners.html.erb index 42ef9b56f..f7dee5716 100644 --- a/app/views/admin/archived/petition_emails/_petition_action_email_petitioners.html.erb +++ b/app/views/admin/archived/petition_emails/_petition_action_email_petitioners.html.erb @@ -5,6 +5,8 @@ Subject + Count + Sent Actions @@ -14,6 +16,8 @@ <%= email.subject %> <%= button_to 'Edit', edit_admin_archived_petition_email_path(petition, email), method: :get, class: 'button' %> + <%= number_with_delimiter(email.email_count) || "–" %> + <%= date_format(email.emails_enqueued_at) || "–" %> <%= button_to 'Delete', admin_archived_petition_email_path(petition, email), method: :delete, class: 'button-warning', data: { confirm: 'Delete other parliamentary business?' } %> diff --git a/app/views/admin/petition_emails/_petition_action_email_petitioners.html.erb b/app/views/admin/petition_emails/_petition_action_email_petitioners.html.erb index 6e975e224..fa5d90405 100644 --- a/app/views/admin/petition_emails/_petition_action_email_petitioners.html.erb +++ b/app/views/admin/petition_emails/_petition_action_email_petitioners.html.erb @@ -5,6 +5,8 @@ Subject + Count + Sent Actions @@ -12,6 +14,8 @@ <% petition.emails.select(&:persisted?).each do |email| %> <%= email.subject %> + <%= number_with_delimiter(email.email_count) || "–" %> + <%= date_format(email.emails_enqueued_at) || "–" %> <%= button_to 'Edit', edit_admin_petition_email_path(petition, email), method: :get, class: 'button', disabled: @petition.editing_disabled? %> <%= button_to 'Delete', admin_petition_email_path(petition, email), method: :delete, class: 'button-warning', disabled: @petition.editing_disabled?, data: { confirm: 'Delete other parliamentary business?' } %> diff --git a/db/migrate/20240117134550_add_email_count_to_other_parliamentary_business.rb b/db/migrate/20240117134550_add_email_count_to_other_parliamentary_business.rb new file mode 100644 index 000000000..f8989f9fc --- /dev/null +++ b/db/migrate/20240117134550_add_email_count_to_other_parliamentary_business.rb @@ -0,0 +1,6 @@ +class AddEmailCountToOtherParliamentaryBusiness < ActiveRecord::Migration[6.1] + def change + add_column :petition_emails, :email_count, :integer + add_column :petition_emails, :emails_enqueued_at, :datetime + end +end diff --git a/db/migrate/20240117150804_add_email_count_to_other_parliamentary_business_archived.rb b/db/migrate/20240117150804_add_email_count_to_other_parliamentary_business_archived.rb new file mode 100644 index 000000000..bee2afa38 --- /dev/null +++ b/db/migrate/20240117150804_add_email_count_to_other_parliamentary_business_archived.rb @@ -0,0 +1,6 @@ +class AddEmailCountToOtherParliamentaryBusinessArchived < ActiveRecord::Migration[6.1] + def change + add_column :archived_petition_emails, :email_count, :integer + add_column :archived_petition_emails, :emails_enqueued_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 6fa87c42f..7c23f575b 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.define(version: 2023_03_27_145255) do +ActiveRecord::Schema.define(version: 2024_01_17_150804) do # These are extensions that must be enabled in order to support this database enable_extension "intarray" @@ -112,6 +112,8 @@ t.string "sent_by" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.integer "email_count" + t.datetime "emails_enqueued_at" t.index ["petition_id"], name: "index_archived_petition_emails_on_petition_id" end @@ -475,6 +477,8 @@ t.string "sent_by" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.integer "email_count" + t.datetime "emails_enqueued_at" t.index ["petition_id"], name: "index_petition_emails_on_petition_id" end diff --git a/spec/factories.rb b/spec/factories.rb index 355e89cb2..cbec28196 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -80,6 +80,10 @@ opened_at { 2.years.ago } closed_at { 1.year.ago } + trait :creator do + association :creator, :validated, :creator, factory: :archived_signature + end + after(:build) do |petition, evaluator| petition.parliament ||= Parliament.archived.first || FactoryBot.create(:parliament, :archived) @@ -258,6 +262,7 @@ creator_name { nil } creator_email { nil } creator_attributes { {} } + email_attributes { nil } sponsors_signed { nil } sponsor_count { Site.minimum_number_of_sponsors } increment { true } @@ -294,6 +299,10 @@ if evaluator.admin_notes petition.build_note details: evaluator.admin_notes end + + if evaluator.email_attributes + petition.emails << build(:petition_email, petition: petition, **evaluator.email_attributes) + end end after(:create) do |petition, evaluator| @@ -319,7 +328,7 @@ trait :archived do archived_at { 3.day.ago } - archiving_started_at { 4.days.ago } + archiving_started_at { 4.days.ago } end trait :with_additional_details do diff --git a/spec/jobs/archive_petition_job_spec.rb b/spec/jobs/archive_petition_job_spec.rb index 2ca51344c..cc9403835 100644 --- a/spec/jobs/archive_petition_job_spec.rb +++ b/spec/jobs/archive_petition_job_spec.rb @@ -287,7 +287,7 @@ end end - context "with a petition that has an government_response email scheduled" do + context "with a petition that has a government_response email scheduled" do let(:petition) do FactoryBot.create(:closed_petition, :email_requested, email_requested_for_government_response_at: Time.current) end @@ -297,7 +297,7 @@ end end - context "with a petition that has an debate_scheduled email scheduled" do + context "with a petition that has a debate_scheduled email scheduled" do let(:petition) do FactoryBot.create(:closed_petition, :email_requested, email_requested_for_debate_scheduled_at: Time.current) end @@ -307,7 +307,7 @@ end end - context "with a petition that has an debate_outcome email scheduled" do + context "with a petition that has a debate_outcome email scheduled" do let(:petition) do FactoryBot.create(:closed_petition, :email_requested, email_requested_for_debate_outcome_at: Time.current) end @@ -317,7 +317,7 @@ end end - context "with a petition that has an petition_email email scheduled" do + context "with a petition that has a petition_email email scheduled" do let(:petition) do FactoryBot.create(:closed_petition, :email_requested, email_requested_for_petition_email_at: Time.current) end @@ -327,7 +327,24 @@ end end - context "with a petition that has an petition_mailshot email scheduled" do + context "with a petition that has a petition_email email that has been sent" do + let(:petition) do + FactoryBot.create(:closed_petition, email_attributes: { email_count: 216, emails_enqueued_at: 6.months.ago }) + end + + let(:email) { petition.emails.first } + let(:archived_email) { archived_petition.emails.first } + + it "copies the email count to the archived petition email" do + expect(archived_email.email_count).to eq(email.email_count) + end + + it "copies the emails enqueued timestamp to the archived petition email" do + expect(archived_email.emails_enqueued_at).to be_usec_precise_with(email.emails_enqueued_at) + end + end + + context "with a petition that has a petition_mailshot email scheduled" do let(:petition) do FactoryBot.create(:closed_petition, :email_requested, email_requested_for_petition_mailshot_at: Time.current) end diff --git a/spec/jobs/archived/email_petitioners_job_spec.rb b/spec/jobs/archived/email_petitioners_job_spec.rb index 990a5d974..8cd4e569f 100644 --- a/spec/jobs/archived/email_petitioners_job_spec.rb +++ b/spec/jobs/archived/email_petitioners_job_spec.rb @@ -3,7 +3,7 @@ RSpec.describe Archived::EmailPetitionersJob, type: :job do let(:email_requested_at) { Time.current } - let(:petition) { FactoryBot.create(:archived_petition) } + let(:petition) { FactoryBot.create(:archived_petition, :creator) } let(:signature) { FactoryBot.create(:archived_signature, petition: petition) } let(:email) { FactoryBot.create(:archived_petition_email, petition: petition) } let(:arguments) { { petition: petition, email: email } } @@ -13,8 +13,32 @@ allow(petition).to receive_message_chain(:signatures_to_email_for, :find_each).and_yield(signature) end - it_behaves_like "job to enqueue signatory mailing jobs" + it_behaves_like "job to enqueue signatory mailing jobs", -> { + subject.perform(petition: petition, email: email, requested_at: requested_at_as_string) + } + context "when sending other parliamentary business emails" do + it "records the number of emails sent" do + expect { + perform_enqueued_jobs { + described_class.run_later_tonight(**arguments) + } + }.to change { + email.reload.email_count + }.from(nil).to(2) + end + + it "records when the emails were enqueued by" do + expect { + perform_enqueued_jobs { + described_class.run_later_tonight(**arguments) + } + }.to change { + email.reload.emails_enqueued_at + }.from(nil).to(be_within(1.second).of(Time.current)) + end + end + context "when the petition email has been deleted" do before do email.destroy diff --git a/spec/jobs/email_petitioners_job_spec.rb b/spec/jobs/email_petitioners_job_spec.rb index 1180b5285..b76ac18a4 100644 --- a/spec/jobs/email_petitioners_job_spec.rb +++ b/spec/jobs/email_petitioners_job_spec.rb @@ -13,7 +13,31 @@ allow(petition).to receive_message_chain(:signatures_to_email_for, :find_each).and_yield(signature) end - it_behaves_like "job to enqueue signatory mailing jobs" + it_behaves_like "job to enqueue signatory mailing jobs", -> { + subject.perform(petition: petition, email: email, requested_at: requested_at_as_string) + } + + context "when sending other parliamentary business emails" do + it "records the number of emails sent" do + expect { + perform_enqueued_jobs { + described_class.run_later_tonight(**arguments) + } + }.to change { + email.reload.email_count + }.from(nil).to(2) + end + + it "records when the emails were enqueued by" do + expect { + perform_enqueued_jobs { + described_class.run_later_tonight(**arguments) + } + }.to change { + email.reload.emails_enqueued_at + }.from(nil).to(be_within(1.second).of(Time.current)) + end + end context "when the petition email has been deleted" do before do diff --git a/spec/jobs/shared_examples.rb b/spec/jobs/shared_examples.rb index 8be28c967..196669a42 100644 --- a/spec/jobs/shared_examples.rb +++ b/spec/jobs/shared_examples.rb @@ -1,9 +1,11 @@ -RSpec.shared_examples_for "job to enqueue signatory mailing jobs" do +RSpec.shared_examples_for "job to enqueue signatory mailing jobs" do |work_proc| let(:requested_at) { Time.current.change(usec: 0) } let(:requested_at_as_string) { requested_at.getutc.iso8601(6) } - def do_work - subject.perform(petition: petition, requested_at: requested_at_as_string) + unless work_proc.present? + work_proc = -> { + subject.perform(petition: petition, requested_at: requested_at_as_string) + } end describe '.run_later_tonight' do @@ -47,18 +49,18 @@ def do_work context "when the petition has not been updated" do it "enqueues a job to send an email to each signatory" do - do_work + instance_exec(&work_proc) expect(enqueued_jobs.size).to eq(1) end it "the job is the correct type for the type of notification" do - do_work + instance_exec(&work_proc) job = enqueued_jobs.first expect(job[:job]).to eq(subject.email_delivery_job_class) end it "the job has the expected arguments" do - do_work + instance_exec(&work_proc) args = enqueued_jobs.first[:args].first expect(args["timestamp_name"]).to eq(subject.timestamp_name) @@ -72,7 +74,7 @@ def do_work end it "does not enqueue any jobs to send emails" do - do_work + instance_exec(&work_proc) expect(enqueued_jobs).to be_empty end end From 5b7adff95051a1f55a04226918002b2870c95aef Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 15 Dec 2023 06:50:18 +0000 Subject: [PATCH 05/10] Replace use of Authlogic token generator The Authlogic gem uses SecureRandom.urlsafe_base64 internally but we can use the SecureRandom.base58 method to restrict it to alphanumeric characters. --- app/models/concerns/perishable_token_generator.rb | 2 +- app/models/signature.rb | 4 ++-- spec/factories.rb | 2 +- spec/models/signature_spec.rb | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/concerns/perishable_token_generator.rb b/app/models/concerns/perishable_token_generator.rb index f591113af..47bf9858c 100644 --- a/app/models/concerns/perishable_token_generator.rb +++ b/app/models/concerns/perishable_token_generator.rb @@ -4,7 +4,7 @@ module PerishableTokenGenerator class_methods do def has_perishable_token(called: 'perishable_token') before_create do - write_attribute(called, Authlogic::Random.friendly_token) + write_attribute(called, SecureRandom.base58(20)) end end end diff --git a/app/models/signature.rb b/app/models/signature.rb index 605f31407..1a0944099 100644 --- a/app/models/signature.rb +++ b/app/models/signature.rb @@ -623,7 +623,7 @@ def validate!(now = Time.current, force: false, request: nil) end unless signed_token? - attributes[:signed_token] = Authlogic::Random.friendly_token + attributes[:signed_token] = SecureRandom.base58(20) end update_columns(attributes) @@ -820,7 +820,7 @@ def generate_uuid end def generate_and_save_signed_token - token = Authlogic::Random.friendly_token + token = SecureRandom.base58(20) retry_lock do if signed_token? diff --git a/spec/factories.rb b/spec/factories.rb index cbec28196..1b0e2b975 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -223,7 +223,7 @@ location_code { "GB" } ip_address { Faker::Internet.public_ip_v4_address } state { Archived::Signature::VALIDATED_STATE } - unsubscribe_token { Authlogic::Random.friendly_token } + unsubscribe_token { SecureRandom.base58(20) } notify_by_email { true } trait :creator do diff --git a/spec/models/signature_spec.rb b/spec/models/signature_spec.rb index 6df799492..5974dd3d9 100644 --- a/spec/models/signature_spec.rb +++ b/spec/models/signature_spec.rb @@ -2086,7 +2086,7 @@ end context "when another process has updated the column" do - let(:token) { Authlogic::Random.friendly_token } + let(:token) { SecureRandom.base58(20) } before do signature.update_column(:signed_token, nil) From b42028947cb8ba8c7bc16d98b7186b6056863d49 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Sat, 16 Dec 2023 16:14:34 +0000 Subject: [PATCH 06/10] Migrate from Authlogic to Devise for authentication Devise has better support for alternative authentication systems and things like 2FA. --- Gemfile | 3 +- Gemfile.lock | 22 +++- app/controllers/admin/admin_controller.rb | 8 ++ app/controllers/admin/profile_controller.rb | 3 +- app/controllers/admin/sessions_controller.rb | 32 +++++ .../admin/user_sessions_controller.rb | 46 -------- app/controllers/concerns/authentication.rb | 39 +----- app/controllers/concerns/flash_i18n.rb | 4 +- app/models/admin_user.rb | 111 +++++++++++------- app/models/admin_user_session.rb | 33 ------ .../admin/sessions/continue.json.jbuilder | 1 + .../{user_sessions => sessions}/new.html.erb | 2 +- app/views/admin/sessions/status.json.jbuilder | 5 + .../user_sessions/continue.json.jbuilder | 1 - .../admin/user_sessions/status.json.jbuilder | 5 - config/initializers/delayed_web.rb | 32 +++-- config/initializers/devise.rb | 79 +++++++++++++ config/locales/activerecord.en-GB.yml | 3 +- config/locales/devise.en-GB.yml | 16 +++ config/routes.rb | 20 +++- .../20231215151430_authlogic_to_devise.rb | 27 +++++ db/schema.rb | 16 +-- features/admin/admin_users_crud.feature | 8 +- features/admin/all_petitions.feature | 6 +- .../step_definitions/authentication_steps.rb | 22 ++++ features/step_definitions/pickle_steps.rb | 12 +- .../encryptors/authlogic_scrypt.rb | 29 +++++ .../admin/admin_users_controller_spec.rb | 4 +- spec/models/admin_user_session_spec.rb | 51 -------- spec/models/admin_user_spec.rb | 111 +++++++++--------- .../admin_user_persistence_token_spec.rb | 22 ++-- spec/requests/csrf_token_spec.rb | 15 ++- spec/requests/login_timeout_spec.rb | 2 +- spec/routing/domain_constraints_spec.rb | 2 +- spec/support/authentication.rb | 14 ++- 35 files changed, 462 insertions(+), 344 deletions(-) create mode 100644 app/controllers/admin/sessions_controller.rb delete mode 100644 app/controllers/admin/user_sessions_controller.rb delete mode 100644 app/models/admin_user_session.rb create mode 100644 app/views/admin/sessions/continue.json.jbuilder rename app/views/admin/{user_sessions => sessions}/new.html.erb (87%) create mode 100644 app/views/admin/sessions/status.json.jbuilder delete mode 100644 app/views/admin/user_sessions/continue.json.jbuilder delete mode 100644 app/views/admin/user_sessions/status.json.jbuilder create mode 100644 config/initializers/devise.rb create mode 100644 config/locales/devise.en-GB.yml create mode 100644 db/migrate/20231215151430_authlogic_to_devise.rb create mode 100644 lib/devise/encryptable/encryptors/authlogic_scrypt.rb delete mode 100644 spec/models/admin_user_session_spec.rb diff --git a/Gemfile b/Gemfile index 07e4f84c5..346d67df1 100644 --- a/Gemfile +++ b/Gemfile @@ -7,7 +7,8 @@ gem 'rails', '6.1.7.6' gem 'rake' gem 'pg' -gem 'authlogic' +gem 'devise' +gem 'devise-encryptable' gem 'will_paginate' gem 'json' gem 'delayed_job_active_record' diff --git a/Gemfile.lock b/Gemfile.lock index 94cd9a272..5af3b1f35 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -64,11 +64,6 @@ GEM public_suffix (>= 2.0.2, < 6.0) appsignal (3.4.13) rack - authlogic (6.4.1) - activemodel (>= 5.2, < 6.2) - activerecord (>= 5.2, < 6.2) - activesupport (>= 5.2, < 6.2) - request_store (~> 1.0) aws-eventstream (1.1.1) aws-partitions (1.441.0) aws-sdk-cloudwatchlogs (1.6.0) @@ -140,6 +135,14 @@ GEM delayed_job_active_record (4.1.6) activerecord (>= 3.0, < 6.2) delayed_job (>= 3.0, < 5) + devise (4.9.3) + bcrypt (~> 3.0) + orm_adapter (~> 0.1) + railties (>= 4.1.0) + responders + warden (~> 1.2.3) + devise-encryptable (0.2.0) + devise (>= 2.1.0) diff-lcs (1.4.4) docile (1.3.1) dotenv (2.7.6) @@ -235,6 +238,7 @@ GEM racc (~> 1.4) nokogiri (1.15.4-x86_64-linux) racc (~> 1.4) + orm_adapter (0.5.0) pg (1.2.3) pry (0.13.1) coderay (~> 1.1) @@ -286,6 +290,9 @@ GEM regexp_parser (2.8.2) request_store (1.5.0) rack (>= 1.4) + responders (3.1.1) + actionpack (>= 5.2) + railties (>= 5.2) rexml (3.2.6) rspec-core (3.10.1) rspec-support (~> 3.10.0) @@ -348,6 +355,8 @@ GEM concurrent-ruby (~> 1.0) uglifier (4.1.8) execjs (>= 0.3.0, < 3) + warden (1.2.9) + rack (>= 2.0.9) webmock (3.18.1) addressable (>= 2.8.0) crack (>= 0.3.2) @@ -371,7 +380,6 @@ PLATFORMS DEPENDENCIES appsignal - authlogic aws-sdk-cloudwatchlogs aws-sdk-codedeploy aws-sdk-s3 @@ -387,6 +395,8 @@ DEPENDENCIES database_cleaner delayed-web delayed_job_active_record + devise + devise-encryptable dotenv-rails email_spec factory_bot_rails diff --git a/app/controllers/admin/admin_controller.rb b/app/controllers/admin/admin_controller.rb index c92caabf4..555eec293 100644 --- a/app/controllers/admin/admin_controller.rb +++ b/app/controllers/admin/admin_controller.rb @@ -26,4 +26,12 @@ def set_appsignal_namespace def set_current_user Admin::Current.user = current_user end + + def after_sign_in_path_for(resource) + admin_root_url + end + + def after_sign_out_path_for(resource) + admin_login_url + end end diff --git a/app/controllers/admin/profile_controller.rb b/app/controllers/admin/profile_controller.rb index d45f59c13..599c5b6e1 100644 --- a/app/controllers/admin/profile_controller.rb +++ b/app/controllers/admin/profile_controller.rb @@ -5,7 +5,8 @@ def edit end def update - if current_user.update_with_password(admin_user_params) + if current_user.update_password(admin_user_params) + bypass_sign_in(current_user, scope: :user) redirect_to admin_root_url, notice: :password_updated else render :edit diff --git a/app/controllers/admin/sessions_controller.rb b/app/controllers/admin/sessions_controller.rb new file mode 100644 index 000000000..8c956af0f --- /dev/null +++ b/app/controllers/admin/sessions_controller.rb @@ -0,0 +1,32 @@ +class Admin::SessionsController < Devise::SessionsController + skip_before_action :require_admin, except: :continue + skip_before_action :check_for_password_change + + prepend_before_action :skip_timeout, only: :status + + helper_method :last_request_at + + def continue + respond_to do |format| + format.json + end + end + + def status + respond_to do |format| + format.json + end + end + + private + + def skip_timeout + request.env['devise.skip_trackable'] = true + end + + def last_request_at + if user_session && user_session.key?("last_request_at") + Time.at(user_session["last_request_at"]).in_time_zone + end + end +end diff --git a/app/controllers/admin/user_sessions_controller.rb b/app/controllers/admin/user_sessions_controller.rb deleted file mode 100644 index 976f31888..000000000 --- a/app/controllers/admin/user_sessions_controller.rb +++ /dev/null @@ -1,46 +0,0 @@ -class Admin::UserSessionsController < Admin::AdminController - skip_before_action :require_admin, only: [:new, :create, :destroy, :status] - skip_before_action :check_for_password_change - - before_action :reset_session, only: [:create, :destroy] - - def new - @user_session = AdminUserSession.new - end - - def create - @user_session = AdminUserSession.new(user_session_params) - - if @user_session.save - redirect_to_target_or_default - elsif @user_session.last_login_attempt? - render :new, alert: :last_login - elsif @user_session.being_brute_force_protected? - render :new, alert: :disabled_login - else - render :new, alert: :invalid_login - end - end - - def destroy - current_session.destroy if logged_in? - redirect_to admin_login_url, notice: :logged_out - end - - def status - end - - def continue - current_user.touch(:last_request_at) - end - - private - - def user_session_params - params.require(:admin_user_session).permit(:email, :password).to_h - end - - def last_request_update_allowed? - action_name != 'status' - end -end diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index fa6435912..26524117d 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -2,32 +2,11 @@ module Authentication extend ActiveSupport::Concern included do - before_action :set_login_timeout - before_action :logout_stale_session - before_action :require_admin before_action :check_for_password_change - helper_method :current_user, :current_session, :logged_in? - end - - def current_session - return @current_session if defined?(@current_session) - @current_session = AdminUserSession.find - end - - def current_user - return @current_user if defined?(@current_user) - @current_user = current_session && current_session.record - end - - def logged_in? - current_user - end - - def redirect_to_target_or_default - redirect_to(session[:return_to] || admin_root_url) - session[:return_to] = nil + alias_method :logged_in?, :user_signed_in? + helper_method :logged_in? end def require_admin @@ -53,18 +32,4 @@ def require_sysadmin redirect_to admin_root_url, alert: :sysadmin_required end end - - private - - def store_target_location - session[:return_to] = request.fullpath - end - - def set_login_timeout - AdminUser.logged_in_timeout = Site.login_timeout - end - - def logout_stale_session - current_session.destroy if current_session && current_session.stale? - end end diff --git a/app/controllers/concerns/flash_i18n.rb b/app/controllers/concerns/flash_i18n.rb index a74acdc1a..33d6084bf 100644 --- a/app/controllers/concerns/flash_i18n.rb +++ b/app/controllers/concerns/flash_i18n.rb @@ -1,6 +1,4 @@ module FlashI18n - protected - def redirect_to(url, options = {}) self.class._flash_types.each do |flash_type| if options.key?(flash_type) @@ -17,6 +15,8 @@ def redirect_to(url, options = {}) super(url, options) end + protected + def translate_flash(key) if admin_request? scope = :"admin.flash" diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 3ac906f3a..0b9c809bc 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -9,19 +9,7 @@ class AdminUser < ActiveRecord::Base class CannotDeleteCurrentUser < RuntimeError; end class MustBeAtLeastOneAdminUser < RuntimeError; end - acts_as_authentic do |config| - config.crypto_provider = ::Authlogic::CryptoProviders::SCrypt - - config.check_passwords_against_database = true - config.ignore_blank_passwords = true - config.logged_in_timeout = Site.login_timeout - config.require_password_confirmation = true - - config.validates_length_of :password, minimum: 8, unless: ->(u) { u.password.blank? } - config.validates_confirmation_of :password, unless: ->(u) { u.password.blank? } - config.validates :email, email: true, uniqueness: { case_sensitive: false } - config.validates_uniqueness_of :email, uniqueness: { case_sensitive: false } - end + devise :database_authenticatable, :encryptable, :trackable, :timeoutable, :lockable with_options dependent: :restrict_with_exception do with_options foreign_key: :moderated_by_id do @@ -31,22 +19,53 @@ class MustBeAtLeastOneAdminUser < RuntimeError; end end # = Validations = - validates_presence_of :email, :first_name, :last_name - validates_presence_of :password, on: :create - validates_format_of :password, with: PASSWORD_REGEX, allow_blank: true - validates_inclusion_of :role, in: ROLES + validates :first_name, :last_name, presence: true + validates :email, presence: true, email: true + validates :email, uniqueness: { case_sensitive: false } + validates :password, presence: true, on: [:create, :update_password] + validates :password, length: { minimum: 8, allow_blank: true } + validates :password, format: { with: PASSWORD_REGEX, allow_blank: true } + validates :password, confirmation: true, on: :update_password + validates :role, inclusion: { in: ROLES } - # = Callbacks = - before_update if: :crypted_password_changed? do - self.force_password_reset = false - self.password_changed_at = Time.current + validate on: :update_password do + errors.add(:current_password, :blank) if current_password.blank? + errors.add(:current_password, :invalid) unless valid_password?(current_password) + errors.add(:password, :taken) if valid_password?(password) end + # = Callbacks = + before_save :clear_locked_at, if: :enabling_account? + before_save :set_locked_at, if: :disabling_account? + before_save :reset_persistence_token, unless: :persistence_token? + before_update :reset_password_tracking, if: :encrypted_password_changed? + # = Finders = scope :by_name, -> { order(:last_name, :first_name) } scope :by_role, ->(role) { where(role: role).order(:id) } # = Methods = + def self.timeout_in + Site.login_timeout.seconds + end + + def reset_password_tracking + self.force_password_reset = false + self.password_changed_at = Time.current + end + + def reset_persistence_token + self.persistence_token = SecureRandom.hex(64) + end + + def reset_persistence_token! + SecureRandom.hex(64).tap { |token| update_column(:persistence_token, token) } + end + + def valid_persistence_token?(token) + persistence_token == token + end + def current_password defined?(@current_password) ? @current_password : nil end @@ -55,24 +74,18 @@ def current_password=(value) @current_password = value end - def update_with_password(attrs) - if attrs[:password].blank? - attrs.delete(:password) - attrs.delete(:password_confirmation) if attrs[:password_confirmation].blank? - end + def valid_password?(password) + encryptor_class.compare(encrypted_password_in_database, password, nil, password_salt_in_database, nil) + end - self.attributes = attrs - self.valid? + def update_password(params) + assign_attributes(params) - if current_password.blank? - errors.add(:current_password, :blank) - elsif !valid_password?(current_password) - errors.add(:current_password, :invalid) - elsif current_password == password - errors.add(:password, :taken) + save(context: :update_password).tap do + self.current_password = nil + self.password = nil + self.password_confirmation = nil end - - errors.empty? && save(validate: false) end def destroy(current_user: nil) @@ -130,18 +143,34 @@ def can_moderate_petitions? end def account_disabled - self.failed_login_count >= DISABLED_LOGIN_COUNT + self.failed_attempts >= DISABLED_LOGIN_COUNT end def account_disabled=(flag) - self.failed_login_count = (flag == "0" or !flag) ? 0 : DISABLED_LOGIN_COUNT + self.failed_attempts = (flag == "0" or !flag) ? 0 : DISABLED_LOGIN_COUNT + end + + def enabling_account? + failed_attempts_changed? && failed_attempts.zero? + end + + def disabling_account? + failed_attempts_changed? && failed_attempts >= DISABLED_LOGIN_COUNT + end + + def clear_locked_at + self.locked_at = nil + end + + def set_locked_at(now = Time.current) + self.locked_at = now end - def elapsed_time(now = Time.current) + def elapsed_time(last_request_at, now = Time.current) (now - last_request_at).floor end - def time_remaining(now = Time.current) - [Site.login_timeout - elapsed_time(now), 0].max + def time_remaining(last_request_at, now = Time.current) + [Site.login_timeout - elapsed_time(last_request_at, now), 0].max end end diff --git a/app/models/admin_user_session.rb b/app/models/admin_user_session.rb deleted file mode 100644 index e11841576..000000000 --- a/app/models/admin_user_session.rb +++ /dev/null @@ -1,33 +0,0 @@ -class AdminUserSession < Authlogic::Session::Base - allow_http_basic_auth false - consecutive_failed_logins_limit AdminUser::DISABLED_LOGIN_COUNT - logout_on_timeout true - - before_save do - record.reset_persistence_token! - end - - before_destroy do - controller.reset_session - - if stale? - stale_record.reset_persistence_token! - else - record.reset_persistence_token! - end - end - - def last_login_attempt? - failed_login_count == consecutive_failed_logins_limit - 1 - end - - def time_remaining - record ? record.time_remaining : 0 - end - - private - - def failed_login_count - attempted_record.present? ? attempted_record.failed_login_count : 0 - end -end diff --git a/app/views/admin/sessions/continue.json.jbuilder b/app/views/admin/sessions/continue.json.jbuilder new file mode 100644 index 000000000..857102f0b --- /dev/null +++ b/app/views/admin/sessions/continue.json.jbuilder @@ -0,0 +1 @@ +json.last_request_at last_request_at diff --git a/app/views/admin/user_sessions/new.html.erb b/app/views/admin/sessions/new.html.erb similarity index 87% rename from app/views/admin/user_sessions/new.html.erb rename to app/views/admin/sessions/new.html.erb index 4554fe2b3..e8586997f 100644 --- a/app/views/admin/user_sessions/new.html.erb +++ b/app/views/admin/sessions/new.html.erb @@ -1,7 +1,7 @@

Sign in

- <%= form_for @user_session do |f| %> + <%= form_for(resource, as: resource_name, url: admin_login_path) do |f| %>
<%= f.label :email, "Email", class: "form-label" %> <%= f.text_field :email, :class => 'form-control', :type => 'email', :autofocus => 'autofocus' %> diff --git a/app/views/admin/sessions/status.json.jbuilder b/app/views/admin/sessions/status.json.jbuilder new file mode 100644 index 000000000..804614345 --- /dev/null +++ b/app/views/admin/sessions/status.json.jbuilder @@ -0,0 +1,5 @@ +if current_user + json.time_remaining current_user.time_remaining(last_request_at) +else + json.time_remaining 0 +end diff --git a/app/views/admin/user_sessions/continue.json.jbuilder b/app/views/admin/user_sessions/continue.json.jbuilder deleted file mode 100644 index f60d49986..000000000 --- a/app/views/admin/user_sessions/continue.json.jbuilder +++ /dev/null @@ -1 +0,0 @@ -json.last_request_at current_user.last_request_at diff --git a/app/views/admin/user_sessions/status.json.jbuilder b/app/views/admin/user_sessions/status.json.jbuilder deleted file mode 100644 index 618c77b4c..000000000 --- a/app/views/admin/user_sessions/status.json.jbuilder +++ /dev/null @@ -1,5 +0,0 @@ -if current_session - json.time_remaining current_session.time_remaining -else - json.time_remaining 0 -end diff --git a/config/initializers/delayed_web.rb b/config/initializers/delayed_web.rb index 582345de3..d0ace5844 100644 --- a/config/initializers/delayed_web.rb +++ b/config/initializers/delayed_web.rb @@ -1,19 +1,31 @@ # Tell Delayed::Web that we're using ActiveRecord as our backend. Rails.application.config.to_prepare do Delayed::Web::Job.backend = 'active_record' -end -# Authenticate our delayed job web interface -Delayed::Web::ApplicationController.class_eval do - include Authentication, FlashI18n + # Authenticate our delayed job web interface + Delayed::Web::ApplicationController.class_eval do + include FlashI18n - def admin_request? - true - end + before_action :require_admin + + def admin_request? + true + end + + protected + + def admin_login_url + main_app.admin_login_url + end - protected + def current_user + @current_user ||= warden.authenticate(scope: :user) + end - def admin_login_url - main_app.admin_login_url + def require_admin + unless current_user + redirect_to admin_login_url, alert: :admin_required + end + end end end diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb new file mode 100644 index 000000000..738457ff3 --- /dev/null +++ b/config/initializers/devise.rb @@ -0,0 +1,79 @@ +require 'devise/encryptable/encryptors/authlogic_scrypt' + +Devise.setup do |config| + # ==> Controller configuration + config.parent_controller = 'Admin::AdminController' + + # ==> Mailer Configuration + config.mailer_sender = 'petitionscommittee@parliament.uk' + config.mailer = 'Devise::Mailer' + config.parent_mailer = 'ActionMailer::Base' + + # ==> ORM configuration + require 'devise/orm/active_record' + + # ==> Configuration for any authentication mechanism + config.authentication_keys = [:email] + config.case_insensitive_keys = [:email] + config.strip_whitespace_keys = [:email] + + # ==> Configuration for :database_authenticatable + config.stretches = Rails.env.test? ? 1 : 12 + + # ==> Configuration for :validatable + config.password_length = 6..128 + config.email_regexp = /\A[^@\s]+@[^@\s]+\z/ + + # ==> Configuration for :timeoutable + # This is overidden on the AdminUser model to use Site.login_timeout + config.timeout_in = 24.hours + + # ==> Configuration for :lockable + config.lock_strategy = :failed_attempts + config.unlock_strategy = :none + config.maximum_attempts = 5 + config.last_attempt_warning = true + + # ==> Configuration for :encryptable + config.encryptor = :authlogic_scrypt + + # ==> Navigation configuration + config.sign_out_via = :get + + # ==> Warden configuration + # Reset the token after logging in so that other sessions are logged out + Warden::Manager.after_set_user except: :fetch do |user, warden, options| + if warden.authenticated?(:user) + session = warden.session(:user) + session['persistence_token'] = user.reset_persistence_token! + end + end + + # Logout the user if the token doesn't match what's in the session + Warden::Manager.after_set_user only: :fetch do |user, warden, options| + if warden.authenticated?(:user) && options[:store] != false + session = warden.session(:user) + + unless user.valid_persistence_token?(session['persistence_token']) + warden.raw_session.clear + warden.logout(:user) + + throw :warden, scope: :user, message: :other_login + end + end + end + + # The devise failure app redirects to self when a session times out + # which causes issues with flash messages, etc. so change the message + # in the options hash to trigger the alternative path. + Warden::Manager.before_failure do |env, options| + if options[:message] == :timeout + options[:message] = :timedout + end + end + + # Reset the token after logging out + Warden::Manager.before_logout do |user, warden, options| + user && user.reset_persistence_token! + end +end diff --git a/config/locales/activerecord.en-GB.yml b/config/locales/activerecord.en-GB.yml index 18bda7f12..768b30b38 100644 --- a/config/locales/activerecord.en-GB.yml +++ b/config/locales/activerecord.en-GB.yml @@ -37,12 +37,13 @@ en-GB: admin_user: attributes: current_password: + blank: "Current password can’t be blank" invalid: "Current password is incorrect" password: invalid: "Password must contain at least one digit, a lower and upper case letter and a special character" taken: "Password is the same as the current password" password_confirmation: - confirmation: "Password confirmation doesn't match password" + confirmation: "Password confirmation doesn’t match password" role: inclusion: "Role '%{value}' is invalid" diff --git a/config/locales/devise.en-GB.yml b/config/locales/devise.en-GB.yml new file mode 100644 index 000000000..976ce48ff --- /dev/null +++ b/config/locales/devise.en-GB.yml @@ -0,0 +1,16 @@ +# Additional translations at https://github.com/heartcombo/devise/wiki/I18n + +en: + devise: + failure: + already_authenticated: "You are already logged in" + invalid: "Invalid email/password combination" + locked: "Failed login limit exceeded, your account has been temporarily disabled" + last_attempt: "You have one more attempt before your account is temporarily disabled" + other_login: "You have been logged out" + not_found_in_database: "Invalid email/password combination" + timedout: "Your session expired. Please log in again to continue" + sessions: + signed_in: "Logged in successfully" + signed_out: "You have been logged out" + already_signed_out: "You have been logged out" diff --git a/config/routes.rb b/config/routes.rb index 0f226623b..6a4d6288b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -132,7 +132,6 @@ resources :admin_users resources :profile, only: %i[edit update] - resources :user_sessions, only: %i[create] resources :invalidations, except: %i[show] do post :cancel, :count, :start, on: :member @@ -246,14 +245,23 @@ post '/', action: 'create', as: nil end - controller 'user_sessions' do - get '/logout', action: 'destroy' - get '/login', action: 'new' - get '/continue', action: 'continue' - get '/status', action: 'status' + end + + devise_for :users, class_name: 'AdminUser', skip: %i[sessions] + + as :user do + controller 'admin/sessions' do + get '/admin/login', action: 'new' + post '/admin/login', action: 'create', as: nil + get '/admin/logout', action: 'destroy' + get '/admin/continue', action: 'continue' + get '/admin/status', action: 'status' end end end + # Devise needs a `new_user_session_url` helper for its failure app + direct(:new_user_session) { route_for(:admin_login) } + get 'ping', to: 'ping#ping' end diff --git a/db/migrate/20231215151430_authlogic_to_devise.rb b/db/migrate/20231215151430_authlogic_to_devise.rb new file mode 100644 index 000000000..af9d78d10 --- /dev/null +++ b/db/migrate/20231215151430_authlogic_to_devise.rb @@ -0,0 +1,27 @@ +class AuthlogicToDevise < ActiveRecord::Migration[6.1] + def up + rename_column :admin_users, :crypted_password, :encrypted_password + rename_column :admin_users, :login_count, :sign_in_count + rename_column :admin_users, :current_login_at, :current_sign_in_at + rename_column :admin_users, :last_login_at, :last_sign_in_at + rename_column :admin_users, :current_login_ip, :current_sign_in_ip + rename_column :admin_users, :last_login_ip, :last_sign_in_ip + rename_column :admin_users, :failed_login_count, :failed_attempts + + add_column :admin_users, :locked_at, :timestamp + remove_column :admin_users, :last_request_at + end + + def down + rename_column :admin_users, :encrypted_password, :crypted_password + rename_column :admin_users, :sign_in_count, :login_count + rename_column :admin_users, :current_sign_in_at, :current_login_at + rename_column :admin_users, :last_sign_in_at, :last_login_at + rename_column :admin_users, :current_sign_in_ip, :current_login_ip + rename_column :admin_users, :last_sign_in_ip, :last_login_ip + rename_column :admin_users, :failed_attempts, :failed_login_count + + remove_column :admin_users, :locked_at + add_column :admin_users, :last_request_at, :timestamp + end +end diff --git a/db/schema.rb b/db/schema.rb index 7c23f575b..b7caf0033 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -47,14 +47,14 @@ create_table "admin_users", id: :serial, force: :cascade do |t| t.string "email", limit: 255, null: false t.string "persistence_token", limit: 255 - t.string "crypted_password", limit: 255 + t.string "encrypted_password", limit: 255 t.string "password_salt", limit: 255 - t.integer "login_count", default: 0 - t.integer "failed_login_count", default: 0 - t.datetime "current_login_at" - t.datetime "last_login_at" - t.string "current_login_ip", limit: 255 - t.string "last_login_ip", limit: 255 + t.integer "sign_in_count", default: 0 + t.integer "failed_attempts", default: 0 + t.datetime "current_sign_in_at" + t.datetime "last_sign_in_at" + t.string "current_sign_in_ip", limit: 255 + t.string "last_sign_in_ip", limit: 255 t.string "first_name", limit: 255 t.string "last_name", limit: 255 t.string "role", limit: 10, null: false @@ -62,7 +62,7 @@ t.datetime "password_changed_at" t.datetime "created_at" t.datetime "updated_at" - t.datetime "last_request_at" + t.datetime "locked_at" t.index ["email"], name: "index_admin_users_on_email", unique: true t.index ["last_name", "first_name"], name: "index_admin_users_on_last_name_and_first_name" end diff --git a/features/admin/admin_users_crud.feature b/features/admin/admin_users_crud.feature index c3c79ac11..c07b7488d 100644 --- a/features/admin/admin_users_crud.feature +++ b/features/admin/admin_users_crud.feature @@ -15,7 +15,7 @@ Feature: Admin users index and crud Scenario: Ordering of the users index Given a moderator user exists with email: "derek@example.com", first_name: "Derek", last_name: "Jacobi" - And a moderator user exists with email: "helen@example.com", first_name: "Helen", last_name: "Hunt", failed_login_count: 5 + And a moderator user exists with email: "helen@example.com", first_name: "Helen", last_name: "Hunt", failed_attempts: 5 When I go to the admin users index page Then I should see the following admin user table: | Name | Email | Role | Disabled | @@ -76,11 +76,11 @@ Feature: Admin users index and crud When I follow "Campbell, Nolene" And the "Email" field should contain "helen@example.com" And the "Account disabled" checkbox should be checked - And a moderator user should exist with email: "helen@example.com", failed_login_count: "5" + And a moderator user should exist with email: "helen@example.com", failed_attempts: "5" And I should be connected to the server via an ssl connection Scenario: Enabling a user's disabled account - Given a moderator user exists with email: "derek@example.com", first_name: "Derek", last_name: "Jacobi", failed_login_count: "5" + Given a moderator user exists with email: "derek@example.com", first_name: "Derek", last_name: "Jacobi", failed_attempts: "5" When I go to the admin users index page And I follow "Jacobi, Derek" And the "Account disabled" checkbox should be checked @@ -89,7 +89,7 @@ Feature: Admin users index and crud Then I should be on the admin users index page When I follow "Jacobi, Derek" And the "Account disabled" checkbox should not be checked - And a moderator user should exist with email: "derek@example.com", failed_login_count: "0" + And a moderator user should exist with email: "derek@example.com", failed_attempts: "0" Scenario: Deleting a user When I go to the admin users index page diff --git a/features/admin/all_petitions.feature b/features/admin/all_petitions.feature index 65fabca5b..7b9e44ed6 100644 --- a/features/admin/all_petitions.feature +++ b/features/admin/all_petitions.feature @@ -108,19 +108,19 @@ Feature: A moderator user views all petitions | My open petition with scheduled debate date | Scenario: A sysadmin can view all petitions - Given I am logged in as a sysadmin + Given I log out and login back in as a sysadmin And an open petition exists with action: "Simply the best" When I view all petitions Then I should see "Simply the best" Scenario: A moderator user can view all petitions - Given I am logged in as a moderator + Given I log out and login back in as a moderator And an open petition exists with action: "Simply the best" When I view all petitions Then I should see "Simply the best" Scenario: A moderator can download petitions as CSV - Given I am logged in as a moderator + Given I log out and login back in as a moderator And the time is "2015-08-21 06:00:00" When I view all petitions And I follow "Download CSV" diff --git a/features/step_definitions/authentication_steps.rb b/features/step_definitions/authentication_steps.rb index e3cdbd1c5..f5af09768 100644 --- a/features/step_definitions/authentication_steps.rb +++ b/features/step_definitions/authentication_steps.rb @@ -8,6 +8,24 @@ step "the admin user is logged in" end +Given(/^I log out and login back in as a sysadmin$/) do + @user = FactoryBot.create(:sysadmin_user) + + steps %q[ + the admin user is logged out + the admin user is logged in + ] +end + +Given(/^I log out and login back in as a moderator$/) do + @user = FactoryBot.create(:moderator_user) + + steps %q[ + the admin user is logged out + the admin user is logged in + ] +end + Given /^I am logged in as a moderator named "([^\"]*)"$/ do |name| first_name, last_name = name.split @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name) @@ -42,3 +60,7 @@ fill_in("Password", :with => "Letmein1!") click_button("Sign in") end + +Given /^the admin user is logged out$/ do + visit admin_logout_url +end diff --git a/features/step_definitions/pickle_steps.rb b/features/step_definitions/pickle_steps.rb index c7d58b89b..2d7822789 100644 --- a/features/step_definitions/pickle_steps.rb +++ b/features/step_definitions/pickle_steps.rb @@ -18,8 +18,8 @@ @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, email: email) end -Given(/^a moderator user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)", failed_login_count: (\d+)$/) do |email, first_name, last_name, failed_login_count| - @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, email: email, failed_login_count: failed_login_count) +Given(/^a moderator user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)", failed_attempts: (\d+)$/) do |email, first_name, last_name, failed_attempts| + @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, email: email, failed_attempts: failed_attempts) end Given(/^(\d+) moderator users exist$/) do |number| @@ -34,12 +34,12 @@ end end -When(/^a moderator user should exist with email: "([^"]*)", failed_login_count: "([^"]*)"$/) do |email, failed_login_count| - expect(AdminUser.where(email: email, failed_login_count: failed_login_count)).to exist +When(/^a moderator user should exist with email: "([^"]*)", failed_attempts: "([^"]*)"$/) do |email, failed_attempts| + expect(AdminUser.where(email: email, failed_attempts: failed_attempts)).to exist end -Given(/^a moderator user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)", failed_login_count: "([^"]*)"$/) do |email, first_name, last_name, failed_login_count| - @user = FactoryBot.create(:moderator_user, email: email, first_name: first_name, last_name: last_name, failed_login_count: failed_login_count) +Given(/^a moderator user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)", failed_attempts: "([^"]*)"$/) do |email, first_name, last_name, failed_attempts| + @user = FactoryBot.create(:moderator_user, email: email, first_name: first_name, last_name: last_name, failed_attempts: failed_attempts) end Then(/^a admin user should not exist with email: "([^"]*)"$/) do |email| diff --git a/lib/devise/encryptable/encryptors/authlogic_scrypt.rb b/lib/devise/encryptable/encryptors/authlogic_scrypt.rb new file mode 100644 index 000000000..44507b350 --- /dev/null +++ b/lib/devise/encryptable/encryptors/authlogic_scrypt.rb @@ -0,0 +1,29 @@ +require 'scrypt' + +module Devise + module Encryptable + module Encryptors + class AuthlogicScrypt + DEFAULTS = { + key_len: 32, + salt_size: 8, + max_time: 0.2, + max_mem: 1024 * 1024, + max_memfrac: 0.5 + } + + def self.digest(password, stretches, salt, pepper) + ::SCrypt::Password.create([password, salt].join, DEFAULTS) + end + + def self.salt(stretches) + SecureRandom.base58(20) + end + + def self.compare(encrypted_password, password, stretches, salt, pepper) + ::SCrypt::Password.new(encrypted_password) == [password, salt].join + end + end + end + end +end diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index f65cae87a..9febba566 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -151,7 +151,7 @@ def do_get end describe "PUT 'update'" do - let(:edit_user) { FactoryBot.create(:moderator_user, :email => "admin@example.com", :failed_login_count => 5) } + let(:edit_user) { FactoryBot.create(:moderator_user, :email => "admin@example.com", :failed_attempts => 5) } def do_update patch :update, params: { @@ -178,7 +178,7 @@ def do_update it "should reset the failed login count to 0" do do_update edit_user.reload - expect(edit_user.failed_login_count).to eq(0) + expect(edit_user.failed_attempts).to eq(0) end end diff --git a/spec/models/admin_user_session_spec.rb b/spec/models/admin_user_session_spec.rb deleted file mode 100644 index 65117cf53..000000000 --- a/spec/models/admin_user_session_spec.rb +++ /dev/null @@ -1,51 +0,0 @@ -require 'rails_helper' - -RSpec.describe AdminUserSession do - describe "#last_login_attempt?" do - before do - Authlogic::Session::Base.controller = Authlogic::TestCase::MockController.new - FactoryBot.create(:moderator_user, email: email) - end - - let(:email) { "admin@petition.parliament.uk" } - let(:params) { { email: email, password: "password" } } - let(:user_session) { described_class.new(params) } - let(:user) { AdminUser.find_by!(email: email) } - - context "when there are no failed login attempts" do - before do - user.update_columns(failed_login_count: 0) - user_session.save - end - - it "returns false" do - expect(user_session.attempted_record).to be_present - expect(user_session.last_login_attempt?).to be false - end - end - - context "when there are 3 failed login attempts" do - before do - user.update_columns(failed_login_count: 3) - user_session.save - end - - it "returns true" do - expect(user_session.attempted_record).to be_present - expect(user_session.last_login_attempt?).to be true - end - end - - context "when there are 4 failed login attempts" do - before do - user.update_columns(failed_login_count: 4) - user_session.save - end - - it "returns false" do - expect(user_session.attempted_record).to be_present - expect(user_session.last_login_attempt?).to be false - end - end - end -end diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb index d7790b125..ebf829773 100644 --- a/spec/models/admin_user_spec.rb +++ b/spec/models/admin_user_spec.rb @@ -4,20 +4,19 @@ describe "schema" do it { is_expected.to have_db_column(:email).of_type(:string).with_options(null: false) } it { is_expected.to have_db_column(:persistence_token).of_type(:string) } - it { is_expected.to have_db_column(:crypted_password).of_type(:string) } + it { is_expected.to have_db_column(:encrypted_password).of_type(:string) } it { is_expected.to have_db_column(:password_salt).of_type(:string) } - it { is_expected.to have_db_column(:login_count).of_type(:integer).with_options(default: 0) } - it { is_expected.to have_db_column(:failed_login_count).of_type(:integer).with_options(default: 0) } - it { is_expected.to have_db_column(:current_login_at).of_type(:datetime) } - it { is_expected.to have_db_column(:last_login_at).of_type(:datetime) } - it { is_expected.to have_db_column(:current_login_ip).of_type(:string) } - it { is_expected.to have_db_column(:last_login_ip).of_type(:string) } + it { is_expected.to have_db_column(:sign_in_count).of_type(:integer).with_options(default: 0) } + it { is_expected.to have_db_column(:failed_attempts).of_type(:integer).with_options(default: 0) } + it { is_expected.to have_db_column(:current_sign_in_at).of_type(:datetime) } + it { is_expected.to have_db_column(:last_sign_in_at).of_type(:datetime) } + it { is_expected.to have_db_column(:current_sign_in_ip).of_type(:string) } + it { is_expected.to have_db_column(:last_sign_in_ip).of_type(:string) } it { is_expected.to have_db_column(:role).of_type(:string).with_options(limit: 10, null: false) } it { is_expected.to have_db_column(:force_password_reset).of_type(:boolean).with_options(default: true) } it { is_expected.to have_db_column(:password_changed_at).of_type(:datetime) } it { is_expected.to have_db_column(:created_at).of_type(:datetime) } it { is_expected.to have_db_column(:updated_at).of_type(:datetime) } - it { is_expected.to have_db_column(:last_request_at).of_type(:datetime) } end describe "indexes" do @@ -25,10 +24,6 @@ it { is_expected.to have_db_index([:last_name, :first_name]) } end - describe "behaviours" do - it { expect(AdminUser.respond_to?(:acts_as_authentic)).to be_truthy } - end - describe "defaults" do it "force_password_reset should default to true" do u = AdminUser.new @@ -100,7 +95,7 @@ end describe "instance methods" do - describe "#update_with_password" do + describe "#update_password" do let!(:user) do FactoryBot.create( :sysadmin_user, @@ -124,28 +119,28 @@ context "when the new password is valid" do it "returns true" do - expect(user.update_with_password(params)).to be_truthy + expect(user.update_password(params)).to be_truthy end - it "changes the crypted_password field" do + it "changes the encrypted_password field" do expect { - user.update_with_password(params) + user.update_password(params) }.to change { - user.reload.crypted_password + user.reload.encrypted_password } end it "sets the timestamp for when the password was changed" do expect { - user.update_with_password(params) + user.update_password(params) }.to change { user.reload.password_changed_at - }.from(nil).to(be_within(1.second).of(Time.current)) + }.from(nil).to(be_within(2.seconds).of(Time.current)) end it "clears the force password reset flag" do expect { - user.update_with_password(params) + user.update_password(params) }.to change { user.reload.force_password_reset }.from(true).to(false) @@ -156,21 +151,21 @@ let(:current_password) { "" } it "returns false" do - expect(user.update_with_password(params)).to be_falsey + expect(user.update_password(params)).to be_falsey end it "adds an error" do - user.update_with_password(params) - expect(user.errors[:current_password]).to eq(["Current password can't be blank"]) + user.update_password(params) + expect(user.errors[:current_password]).to include("Current password can’t be blank") end it "doesn't clear the force password reset flag" do - user.update_with_password(params) + user.update_password(params) expect(user.force_password_reset).to be true end it "doesn't set the password_changed_at timestamp" do - user.update_with_password(params) + user.update_password(params) expect(user.password_changed_at).to be_nil end end @@ -179,21 +174,21 @@ let(:current_password) { "L3tme!n" } it "returns false" do - expect(user.update_with_password(params)).to be_falsey + expect(user.update_password(params)).to be_falsey end it "adds an error" do - user.update_with_password(params) - expect(user.errors[:current_password]).to eq(["Current password is incorrect"]) + user.update_password(params) + expect(user.errors[:current_password]).to include("Current password is incorrect") end it "doesn't clear the force password reset flag" do - user.update_with_password(params) + user.update_password(params) expect(user.force_password_reset).to be true end it "doesn't set the password_changed_at timestamp" do - user.update_with_password(params) + user.update_password(params) expect(user.password_changed_at).to be_nil end end @@ -203,21 +198,21 @@ let(:password_confirmation) { current_password } it "returns false" do - expect(user.update_with_password(params)).to be_falsey + expect(user.update_password(params)).to be_falsey end it "adds an error" do - user.update_with_password(params) - expect(user.errors[:password]).to eq(["Password is the same as the current password"]) + user.update_password(params) + expect(user.errors[:password]).to include("Password is the same as the current password") end it "doesn't clear the force password reset flag" do - user.update_with_password(params) + user.update_password(params) expect(user.force_password_reset).to be true end it "doesn't set the password_changed_at timestamp" do - user.update_with_password(params) + user.update_password(params) expect(user.password_changed_at).to be_nil end end @@ -227,21 +222,21 @@ let(:password_confirmation) { "password" } it "returns false" do - expect(user.update_with_password(params)).to be_falsey + expect(user.update_password(params)).to be_falsey end it "adds an error" do - user.update_with_password(params) - expect(user.errors[:password]).to eq(["Password must contain at least one digit, a lower and upper case letter and a special character"]) + user.update_password(params) + expect(user.errors[:password]).to include("Password must contain at least one digit, a lower and upper case letter and a special character") end it "doesn't clear the force password reset flag" do - user.update_with_password(params) + user.update_password(params) expect(user.force_password_reset).to be true end it "doesn't set the password_changed_at timestamp" do - user.update_with_password(params) + user.update_password(params) expect(user.password_changed_at).to be_nil end end @@ -251,21 +246,21 @@ let(:password_confirmation) { "L3tme!n2" } it "returns false" do - expect(user.update_with_password(params)).to be_falsey + expect(user.update_password(params)).to be_falsey end it "adds an error" do - user.update_with_password(params) - expect(user.errors[:password_confirmation]).to eq(["Password confirmation doesn't match password"]) + user.update_password(params) + expect(user.errors[:password_confirmation]).to include("Password confirmation doesn’t match password") end it "doesn't clear the force password reset flag" do - user.update_with_password(params) + user.update_password(params) expect(user.force_password_reset).to be true end it "doesn't set the password_changed_at timestamp" do - user.update_with_password(params) + user.update_password(params) expect(user.password_changed_at).to be_nil end end @@ -379,19 +374,19 @@ describe "#account_disabled" do it "should return true when user has tried to login 5 times unsuccessfully" do user = FactoryBot.create(:moderator_user) - user.failed_login_count = 5 + user.failed_attempts = 5 expect(user.account_disabled).to be_truthy end it "should return true when user has tried to login 6 times unsuccessfully" do user = FactoryBot.create(:moderator_user) - user.failed_login_count = 6 + user.failed_attempts = 6 expect(user.account_disabled).to be_truthy end it "should return false when user has tried to login 4 times unsuccessfully" do user = FactoryBot.create(:moderator_user) - user.failed_login_count = 4 + user.failed_attempts = 4 expect(user.account_disabled).to be_falsey end end @@ -400,28 +395,32 @@ it "should set the failed login count to 5 when true" do u = FactoryBot.create(:moderator_user) u.account_disabled = true - expect(u.failed_login_count).to eq(5) + expect(u.failed_attempts).to eq(5) end it "should set the failed login count to 0 when false" do u = FactoryBot.create(:moderator_user) - u.failed_login_count = 5 + u.failed_attempts = 5 u.account_disabled = false - expect(u.failed_login_count).to eq(0) + expect(u.failed_attempts).to eq(0) end end describe "#elapsed_time" do it "returns the number of seconds since the last request" do - user = FactoryBot.build(:admin_user, last_request_at: 60.seconds.ago) - expect(user.elapsed_time).to eq(60) + user = FactoryBot.build(:admin_user) + expect(user.elapsed_time(60.seconds.ago)).to eq(60) end end describe "#time_remaining" do - it "returns the number of seconds since the last request" do - user = FactoryBot.build(:admin_user, last_request_at: 60.seconds.ago) - expect(user.elapsed_time).to eq(60) + before do + allow(Site).to receive(:login_timeout).and_return(300) + end + + it "returns the number of seconds remaining until the user is logged out" do + user = FactoryBot.build(:admin_user) + expect(user.time_remaining(60.seconds.ago)).to eq(240) end end end diff --git a/spec/requests/admin_user_persistence_token_spec.rb b/spec/requests/admin_user_persistence_token_spec.rb index 9327ab238..673fbc5ca 100644 --- a/spec/requests/admin_user_persistence_token_spec.rb +++ b/spec/requests/admin_user_persistence_token_spec.rb @@ -30,44 +30,44 @@ def new_browser context "when a new session is created" do it "logs out existing sessions" do s1 = new_browser - s1.post "/admin/user_sessions", params: { admin_user_session: login_params } + s1.post "/admin/login", params: { user: login_params } expect(s1.response.status).to eq(302) - expect(s1.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin") + expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin") s2 = new_browser - s2.post "/admin/user_sessions", params: { admin_user_session: login_params } + s2.post "/admin/login", params: { user: login_params } expect(s2.response.status).to eq(302) - expect(s2.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin") + expect(s2.response.location).to eq("https://moderate.petition.parliament.uk/admin") s1.get("/admin") expect(s1.response.status).to eq(302) - expect(s1.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin/login") + expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin/login") end end context "when a session is destroyed" do it "resets the persistence token" do s1 = new_browser - s1.post "/admin/user_sessions", params: { admin_user_session: login_params } + s1.post "/admin/login", params: { user: login_params } expect(s1.response.status).to eq(302) - expect(s1.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin") + expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin") s2 = new_browser - s2.cookies["admin_user_credentials"] = s1.cookies["admin_user_credentials"] + s2.cookies["_epets_session"] = s1.cookies["_epets_session"] s1.get("/admin/logout") expect(s1.response.status).to eq(302) - expect(s1.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin/login") + expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin/login") s2.get("/admin") expect(s2.response.status).to eq(302) - expect(s2.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin/login") + expect(s2.response.location).to eq("https://moderate.petition.parliament.uk/admin/login") end end @@ -81,7 +81,7 @@ def new_browser Site.instance.update(login_timeout: 600) travel_to 5.minutes.ago do - post "/admin/user_sessions", params: { admin_user_session: login_params } + post "/admin/login", params: { user: login_params } expect(response).to redirect_to("/admin") end diff --git a/spec/requests/csrf_token_spec.rb b/spec/requests/csrf_token_spec.rb index 5ed37b7f6..7402162d9 100644 --- a/spec/requests/csrf_token_spec.rb +++ b/spec/requests/csrf_token_spec.rb @@ -36,20 +36,25 @@ context "when a new session is created" do it "resets the csrf token" do expect { - post "/admin/user_sessions", params: { admin_user_session: login_params, authenticity_token: authenticity_token } + post "/admin/login", params: { user: login_params, authenticity_token: authenticity_token } }.to change { session[:_csrf_token] - } + }.from(be_present).to(be_nil) end end context "when a session is destroyed" do + before do + post "/admin/login", params: { user: login_params, authenticity_token: authenticity_token } + follow_redirect! + end + it "resets the csrf token" do expect { get "/admin/logout" }.to change { session[:_csrf_token] - } + }.from(be_present).to(be_nil) end end @@ -58,7 +63,7 @@ Site.instance.update(login_timeout: 600) travel_to 5.minutes.ago do - post "/admin/user_sessions", params: { admin_user_session: login_params, authenticity_token: authenticity_token } + post "/admin/login", params: { user: login_params, authenticity_token: authenticity_token } expect(response).to redirect_to("/admin") end @@ -71,7 +76,7 @@ expect(response).to redirect_to("/admin/login") }.to change { session[:_csrf_token] - } + }.from(be_present).to(be_nil) end Site.instance.update(login_timeout: 1800) diff --git a/spec/requests/login_timeout_spec.rb b/spec/requests/login_timeout_spec.rb index d39ae6c64..af3a3f1a7 100644 --- a/spec/requests/login_timeout_spec.rb +++ b/spec/requests/login_timeout_spec.rb @@ -26,7 +26,7 @@ Site.instance.update(login_timeout: 600) travel_to 2.minutes.ago do - post "/admin/user_sessions", params: { admin_user_session: login_params } + post "/admin/login", params: { user: login_params } expect(response).to redirect_to("/admin") end diff --git a/spec/routing/domain_constraints_spec.rb b/spec/routing/domain_constraints_spec.rb index b3386554a..be1cce0fb 100644 --- a/spec/routing/domain_constraints_spec.rb +++ b/spec/routing/domain_constraints_spec.rb @@ -30,7 +30,7 @@ context "and making a request for an admin path" do it "is routable" do - expect(get("/admin/login")).to route_to("admin/user_sessions#new") + expect(get("/admin/login")).to route_to("admin/sessions#new") end end diff --git a/spec/support/authentication.rb b/spec/support/authentication.rb index 3f737e20f..83ee60779 100644 --- a/spec/support/authentication.rb +++ b/spec/support/authentication.rb @@ -1,10 +1,14 @@ -require 'authlogic/test_case' - RSpec.configure do |config| mod = Module.new do def login_as(user) - activate_authlogic - AdminUserSession.create(user) + sign_in(user, scope: :user) + + # The devise sign_in test helper bypasses the hooks that set + # the token in the session so we have to set them manually. + session["warden.user.user.session"] = { + "persistence_token" => user.persistence_token, + "last_request_at" => Time.current.to_i + } end def http_authentication(username, password) @@ -12,6 +16,6 @@ def http_authentication(username, password) end end + config.include(Devise::Test::ControllerHelpers, type: :controller) config.include(mod, type: :controller) - config.include(Authlogic::TestCase, type: :controller) end From 664c006a424e609b47b8c6f7f5f87446e16ae9d7 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 15 Dec 2023 09:17:58 +0000 Subject: [PATCH 07/10] Remove password reset feature Once migrated there will be no passwords to reset. --- .../stylesheets/petitions/admin/_header.scss | 1 + app/controllers/admin/profile_controller.rb | 21 ---- app/controllers/admin/sessions_controller.rb | 2 - app/controllers/concerns/authentication.rb | 7 -- app/views/admin/profile/edit.html.erb | 26 ---- .../shared/_header_user_actions.html.erb | 2 +- features/admin/admin_access.feature | 28 ----- features/admin/profile.feature | 42 ------- features/support/paths.rb | 6 - .../admin/admin_users_controller_spec.rb | 13 -- .../debate_outcomes_controller_spec.rb | 19 --- .../government_response_controller_spec.rb | 19 --- .../admin/archived/locks_controller_spec.rb | 22 ---- .../admin/archived/notes_controller_spec.rb | 19 --- .../petition_departments_controller_spec.rb | 19 --- .../petition_details_controller_spec.rb | 19 --- .../petition_emails_controller_spec.rb | 42 ------- .../archived/petition_tags_controller_spec.rb | 19 --- .../petition_topics_controller_spec.rb | 19 --- .../archived/petitions_controller_spec.rb | 19 --- .../schedule_debate_controller_spec.rb | 19 --- .../archived/signatures_controller_spec.rb | 19 --- .../admin/debate_outcomes_controller_spec.rb | 19 --- .../government_response_controller_spec.rb | 19 --- .../admin/locks_controller_spec.rb | 22 ---- .../moderation_delays_controller_spec.rb | 20 --- .../admin/notes_controller_spec.rb | 19 --- .../petition_departments_controller_spec.rb | 19 --- .../admin/petition_details_controller_spec.rb | 19 --- .../admin/petition_emails_controller_spec.rb | 42 ------- .../admin/petition_tags_controller_spec.rb | 19 --- .../admin/petition_topics_controller_spec.rb | 19 --- .../admin/petitions_controller_spec.rb | 19 --- .../admin/profile_controller_spec.rb | 116 ------------------ .../admin/schedule_debate_controller_spec.rb | 19 --- .../admin/searches_controller_spec.rb | 12 -- .../admin/signatures_controller_spec.rb | 19 --- .../admin/take_down_controller_spec.rb | 19 --- .../admin/trending_domains_controller_spec.rb | 18 --- .../admin/trending_ips_controller_spec.rb | 18 --- 40 files changed, 2 insertions(+), 857 deletions(-) delete mode 100644 app/controllers/admin/profile_controller.rb delete mode 100644 app/views/admin/profile/edit.html.erb delete mode 100644 features/admin/profile.feature delete mode 100644 spec/controllers/admin/profile_controller_spec.rb diff --git a/app/assets/stylesheets/petitions/admin/_header.scss b/app/assets/stylesheets/petitions/admin/_header.scss index 25441550f..32d6d888e 100644 --- a/app/assets/stylesheets/petitions/admin/_header.scss +++ b/app/assets/stylesheets/petitions/admin/_header.scss @@ -9,6 +9,7 @@ float: right; } li { + color: $white; display: inline; margin-left: $gutter-one-third; a { diff --git a/app/controllers/admin/profile_controller.rb b/app/controllers/admin/profile_controller.rb deleted file mode 100644 index 599c5b6e1..000000000 --- a/app/controllers/admin/profile_controller.rb +++ /dev/null @@ -1,21 +0,0 @@ -class Admin::ProfileController < Admin::AdminController - skip_before_action :check_for_password_change - - def edit - end - - def update - if current_user.update_password(admin_user_params) - bypass_sign_in(current_user, scope: :user) - redirect_to admin_root_url, notice: :password_updated - else - render :edit - end - end - - def admin_user_params - params.require(:admin_user).permit( - :current_password, :password, :password_confirmation - ) - end -end diff --git a/app/controllers/admin/sessions_controller.rb b/app/controllers/admin/sessions_controller.rb index 8c956af0f..31284179c 100644 --- a/app/controllers/admin/sessions_controller.rb +++ b/app/controllers/admin/sessions_controller.rb @@ -1,7 +1,5 @@ class Admin::SessionsController < Devise::SessionsController skip_before_action :require_admin, except: :continue - skip_before_action :check_for_password_change - prepend_before_action :skip_timeout, only: :status helper_method :last_request_at diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 26524117d..2ae551c8b 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -3,7 +3,6 @@ module Authentication included do before_action :require_admin - before_action :check_for_password_change alias_method :logged_in?, :user_signed_in? helper_method :logged_in? @@ -15,12 +14,6 @@ def require_admin end end - def check_for_password_change - if current_user.has_to_change_password? - redirect_to edit_admin_profile_url(current_user), alert: :change_password - end - end - def require_moderator unless current_user.is_a_moderator? || current_user.is_a_sysadmin? redirect_to admin_root_url, alert: :moderator_required diff --git a/app/views/admin/profile/edit.html.erb b/app/views/admin/profile/edit.html.erb deleted file mode 100644 index 0182b5982..000000000 --- a/app/views/admin/profile/edit.html.erb +++ /dev/null @@ -1,26 +0,0 @@ -

Change your password

-
-
- <%= form_for [:admin, current_user], url: admin_profile_path(current_user) do |f| -%> - <%= form_row for: [f.object, :current_password] do %> - <%= f.label :current_password, class: 'form-label' %> - <%= error_messages_for_field f.object, :current_password %> - <%= f.password_field :current_password, tabindex: increment, class: 'form-control', autofocus: 'autofocus', autocomplete: 'off' %> - <% end %> - - <%= form_row for: [f.object, :password] do %> - <%= f.label :password, 'New password', class: 'form-label' %> - <%= error_messages_for_field f.object, :password %> - <%= f.password_field :password, tabindex: increment, class: 'form-control', autocomplete: 'off' %> - <% end %> - - <%= form_row for: [f.object, :password_confirmation] do %> - <%= f.label :password_confirmation, class: 'form-label' %> - <%= error_messages_for_field f.object, :password_confirmation %> - <%= f.password_field :password_confirmation, tabindex: increment, class: 'form-control', autocomplete: 'off' %> - <% end %> - - <%= f.submit 'Save', class: "button" %> - <% end -%> -
-
diff --git a/app/views/admin/shared/_header_user_actions.html.erb b/app/views/admin/shared/_header_user_actions.html.erb index 41cbe6bd8..d4024db58 100644 --- a/app/views/admin/shared/_header_user_actions.html.erb +++ b/app/views/admin/shared/_header_user_actions.html.erb @@ -1,6 +1,6 @@ <% if current_user %>
    -
  • <%= link_to current_user.pretty_name, edit_admin_profile_path(current_user) %>
  • +
  • <%= current_user.pretty_name %>
  • <% if current_user.is_a_sysadmin? %>
  • <%= link_to "Site", edit_admin_site_path %>
  • <%= link_to "Parliament", admin_parliament_path %>
  • diff --git a/features/admin/admin_access.feature b/features/admin/admin_access.feature index 3a7c1d88f..9c4c49d27 100644 --- a/features/admin/admin_access.feature +++ b/features/admin/admin_access.feature @@ -72,31 +72,3 @@ Feature: Restricted access to the admin console And I fill in "Password" with "Letmein1!" And I press "Sign in" Then I should be on the admin home page - - Scenario: Login as a user who hasn't changed their password for over 9 months - Given a moderator user exists with email: "admin@example.com", password: "Letmein1!", password_confirmation: "Letmein1!" - And the password was changed 10 months ago - When I go to the admin login page - And I fill in "Email" with "admin@example.com" - And I fill in "Password" with "Letmein1!" - And I press "Sign in" - Then I should be on the admin edit profile page for "admin@example.com" - And I should be connected to the server via an ssl connection - And I fill in "Current password" with "Letmein1!" - And I fill in "New password" with "Letmeout1!" - And I fill in "Password confirmation" with "Letmeout1!" - And I press "Save" - Then I should be on the admin home page - - Scenario: Login as a user who is logging in for the first time - Given a moderator user exists with email: "admin@example.com", password: "Letmein1!", password_confirmation: "Letmein1!", force_password_reset: true - When I go to the admin login page - And I fill in "Email" with "admin@example.com" - And I fill in "Password" with "Letmein1!" - And I press "Sign in" - Then I should be on the admin edit profile page for "admin@example.com" - And I fill in "Current password" with "Letmein1!" - And I fill in "New password" with "Letmeout1!" - And I fill in "Password confirmation" with "Letmeout1!" - And I press "Save" - Then I should be on the admin home page diff --git a/features/admin/profile.feature b/features/admin/profile.feature deleted file mode 100644 index b9430d02c..000000000 --- a/features/admin/profile.feature +++ /dev/null @@ -1,42 +0,0 @@ -@admin -Feature: Moderator users password change - As a an moderator - I can change my password - - Background: - Given I am logged in as a moderator named "John Moderator" with the password "Letmein1!" - - Scenario: Accessing the Profile page - When I go to the admin home page - And I follow "John Moderator" - Then I should be on the admin edit profile page - And I should see a "Current password" password field - And I should see a "New password" password field - And I should see a "Password confirmation" password field - And I should be connected to the server via an ssl connection - And the markup should be valid - - Scenario: Changing password successfully - When I go to the admin edit profile page - And I fill in "Current password" with "Letmein1!" - And I fill in "New password" with "Letmeout1!" - And I fill in "Password confirmation" with "Letmeout1!" - And I press "Save" - Then I should be on the admin home page - And I should see "Password was successfully updated" - - Scenario: Incorrect current password - When I go to the admin edit profile page - And I fill in "Current password" with "wrong password" - And I fill in "New password" with "Letmeout1!" - And I fill in "Password confirmation" with "Letmeout1!" - And I press "Save" - Then I should see "Current password is incorrect" - - Scenario: Invalid new password - When I go to the admin edit profile page - And I fill in "Current password" with "Letmein1!" - And I fill in "New password" with "12345678" - And I fill in "Password confirmation" with "12345678" - And I press "Save" - Then I should see "Password must contain at least one digit, a lower and upper case letter and a special character" diff --git a/features/support/paths.rb b/features/support/paths.rb index 250e0f80c..10e44ef57 100644 --- a/features/support/paths.rb +++ b/features/support/paths.rb @@ -110,12 +110,6 @@ def admin_url(admin_page) when /^new user page$/ new_admin_admin_user_url - when /^edit profile page$/ - edit_admin_profile_url(@user) - - when /^edit profile page for "([^\"]*)"$/ - edit_admin_profile_url(AdminUser.find_by(email: $1)) - when /^debate outcomes form page for "([^\"]*)"$/ admin_petition_debate_outcome_url(Petition.find_by(action: $1)) diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index 9febba566..02f23a659 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -31,19 +31,6 @@ end end - context "logged in as sysadmin but need to reset password" do - let(:user) { FactoryBot.create(:sysadmin_user, :force_password_reset => true) } - before :each do - login_as(user) - end - - it "should redirect to edit profile page" do - expect(user.has_to_change_password?).to be_truthy - get :index - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - describe "logged in as sysadmin user" do let(:user) { FactoryBot.create(:sysadmin_user, :first_name => 'Sys', :last_name => 'Admin') } before :each do diff --git a/spec/controllers/admin/archived/debate_outcomes_controller_spec.rb b/spec/controllers/admin/archived/debate_outcomes_controller_spec.rb index 5fa0ec367..01eeb1583 100644 --- a/spec/controllers/admin/archived/debate_outcomes_controller_spec.rb +++ b/spec/controllers/admin/archived/debate_outcomes_controller_spec.rb @@ -41,25 +41,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET /show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/government_response_controller_spec.rb b/spec/controllers/admin/archived/government_response_controller_spec.rb index fac180081..decfeca3a 100644 --- a/spec/controllers/admin/archived/government_response_controller_spec.rb +++ b/spec/controllers/admin/archived/government_response_controller_spec.rb @@ -42,25 +42,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET /show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/locks_controller_spec.rb b/spec/controllers/admin/archived/locks_controller_spec.rb index 06c630e4c..54500efd7 100644 --- a/spec/controllers/admin/archived/locks_controller_spec.rb +++ b/spec/controllers/admin/archived/locks_controller_spec.rb @@ -20,28 +20,6 @@ end end - context "when logged in as a moderator requiring a password reset" do - let(:moderator) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(moderator) } - - [ - ["GET", "/admin/archived/petitions/:petition_id/lock.json", :show, { petition_id: "100000" }], - ["POST", "/admin/archived/petitions/:petition_id/lock.json", :create, { petition_id: "100000" }], - ["PATCH", "/admin/archived/petitions/:petition_id/lock.json", :update, { petition_id: "100000" }], - ["DELETE", "/admin/archived/petitions/:petition_id/lock.json", :destroy, { petition_id: "100000" }] - ].each do |method, path, action, params| - - describe "#{method} #{path}" do - before { process action, method: method, params: params, format: :json } - - it "redirects to the admin profile page" do - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{moderator.id}/edit") - end - end - - end - end - context "when logged in as a moderator" do let(:moderator) { FactoryBot.create(:moderator_user) } let(:petition) { FactoryBot.create(:archived_petition) } diff --git a/spec/controllers/admin/archived/notes_controller_spec.rb b/spec/controllers/admin/archived/notes_controller_spec.rb index c6cbb0057..3a9210851 100644 --- a/spec/controllers/admin/archived/notes_controller_spec.rb +++ b/spec/controllers/admin/archived/notes_controller_spec.rb @@ -20,25 +20,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET /show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/petition_departments_controller_spec.rb b/spec/controllers/admin/archived/petition_departments_controller_spec.rb index d846b1df7..f0fc1f629 100644 --- a/spec/controllers/admin/archived/petition_departments_controller_spec.rb +++ b/spec/controllers/admin/archived/petition_departments_controller_spec.rb @@ -20,25 +20,6 @@ end end - context "when logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/archived/petitions/:petition_id/departments" do - it "redirects to the edit profile page" do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "PATCH /admin/archived/petitions/:petition_id/departments" do - it "redirects to the edit profile page" do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/petition_details_controller_spec.rb b/spec/controllers/admin/archived/petition_details_controller_spec.rb index d25704269..62fbd9243 100644 --- a/spec/controllers/admin/archived/petition_details_controller_spec.rb +++ b/spec/controllers/admin/archived/petition_details_controller_spec.rb @@ -20,25 +20,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET #show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH #update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context 'logged in as moderator user' do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/petition_emails_controller_spec.rb b/spec/controllers/admin/archived/petition_emails_controller_spec.rb index 18f1b8830..ffb892f1e 100644 --- a/spec/controllers/admin/archived/petition_emails_controller_spec.rb +++ b/spec/controllers/admin/archived/petition_emails_controller_spec.rb @@ -90,48 +90,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:email) { FactoryBot.create(:archived_petition_email, petition: petition) } - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - - before { login_as(user) } - - describe 'GET /new' do - it 'redirects to edit profile page' do - get :new, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'POST /' do - it 'redirects to edit profile page' do - post :create, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'GET /:id/edit' do - it 'redirects to the login page' do - get :edit, params: { petition_id: petition.id, id: email.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /:id' do - it 'redirects to the login page' do - patch :update, params: { petition_id: petition.id, id: email.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'DELETE /:id' do - it 'redirects to the login page' do - patch :destroy, params: { petition_id: petition.id, id: email.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/petition_tags_controller_spec.rb b/spec/controllers/admin/archived/petition_tags_controller_spec.rb index 052df7696..bb7f9f17c 100644 --- a/spec/controllers/admin/archived/petition_tags_controller_spec.rb +++ b/spec/controllers/admin/archived/petition_tags_controller_spec.rb @@ -20,25 +20,6 @@ end end - context "when logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/archived/petitions/:petition_id/tags" do - it "redirects to the edit profile page" do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "PATCH /admin/archived/petitions/:petition_id/tags" do - it "redirects to the edit profile page" do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/petition_topics_controller_spec.rb b/spec/controllers/admin/archived/petition_topics_controller_spec.rb index ef1763f61..dd04af12f 100644 --- a/spec/controllers/admin/archived/petition_topics_controller_spec.rb +++ b/spec/controllers/admin/archived/petition_topics_controller_spec.rb @@ -20,25 +20,6 @@ end end - context "when logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/archived/petitions/:petition_id/topics" do - it "redirects to the edit profile page" do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "PATCH /admin/archived/petitions/:petition_id/topics" do - it "redirects to the edit profile page" do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/petitions_controller_spec.rb b/spec/controllers/admin/archived/petitions_controller_spec.rb index bf3f7a527..44727edaf 100644 --- a/spec/controllers/admin/archived/petitions_controller_spec.rb +++ b/spec/controllers/admin/archived/petitions_controller_spec.rb @@ -21,25 +21,6 @@ end end - context "when logged in as a moderator but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/archived/petitions" do - it "redirects to the edit profile page" do - get :index - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "GET /admin/archived/petitions/:id" do - it "redirects to the edit profile page" do - get :show, params: { id: "100000" } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as a moderator" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/schedule_debate_controller_spec.rb b/spec/controllers/admin/archived/schedule_debate_controller_spec.rb index 03599b847..a7b9ad8fa 100644 --- a/spec/controllers/admin/archived/schedule_debate_controller_spec.rb +++ b/spec/controllers/admin/archived/schedule_debate_controller_spec.rb @@ -41,25 +41,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET /show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/archived/signatures_controller_spec.rb b/spec/controllers/admin/archived/signatures_controller_spec.rb index f0ced78f7..0543c445b 100644 --- a/spec/controllers/admin/archived/signatures_controller_spec.rb +++ b/spec/controllers/admin/archived/signatures_controller_spec.rb @@ -20,25 +20,6 @@ end end - context "logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/archived/signatures" do - it "redirects to the login page" do - get :index, params: { q: "Alice" } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "DELETE /admin/archived/signatures/:id" do - it "redirects to edit profile page" do - delete :destroy, params: { id: signature.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } let(:token) { SecureRandom.base64(32) } diff --git a/spec/controllers/admin/debate_outcomes_controller_spec.rb b/spec/controllers/admin/debate_outcomes_controller_spec.rb index 985e988f2..aa1f8d518 100644 --- a/spec/controllers/admin/debate_outcomes_controller_spec.rb +++ b/spec/controllers/admin/debate_outcomes_controller_spec.rb @@ -40,25 +40,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET /show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/government_response_controller_spec.rb b/spec/controllers/admin/government_response_controller_spec.rb index 17f7522ea..344ee3f58 100644 --- a/spec/controllers/admin/government_response_controller_spec.rb +++ b/spec/controllers/admin/government_response_controller_spec.rb @@ -41,25 +41,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET /show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/locks_controller_spec.rb b/spec/controllers/admin/locks_controller_spec.rb index f6f12ac79..44289725c 100644 --- a/spec/controllers/admin/locks_controller_spec.rb +++ b/spec/controllers/admin/locks_controller_spec.rb @@ -20,28 +20,6 @@ end end - context "when logged in as a moderator requiring a password reset" do - let(:moderator) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(moderator) } - - [ - ["GET", "/admin/petitions/:petition_id/lock.json", :show, { petition_id: "100000" }], - ["POST", "/admin/petitions/:petition_id/lock.json", :create, { petition_id: "100000" }], - ["PATCH", "/admin/petitions/:petition_id/lock.json", :update, { petition_id: "100000" }], - ["DELETE", "/admin/petitions/:petition_id/lock.json", :destroy, { petition_id: "100000" }] - ].each do |method, path, action, params| - - describe "#{method} #{path}" do - before { process action, method: method, params: params, format: :json } - - it "redirects to the admin profile page" do - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{moderator.id}/edit") - end - end - - end - end - context "when logged in as a moderator" do let(:moderator) { FactoryBot.create(:moderator_user) } let(:petition) { FactoryBot.create(:petition) } diff --git a/spec/controllers/admin/moderation_delays_controller_spec.rb b/spec/controllers/admin/moderation_delays_controller_spec.rb index a4adb43b7..94157bc54 100644 --- a/spec/controllers/admin/moderation_delays_controller_spec.rb +++ b/spec/controllers/admin/moderation_delays_controller_spec.rb @@ -18,26 +18,6 @@ end end - context "when logged in as a moderator requiring a password reset" do - let(:moderator) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(moderator) } - - [ - ["GET", "/admin/moderation-delay/new", :new, {}], - ["POST", "/admin/moderation-delay", :create, {}] - ].each do |method, path, action, params| - - describe "#{method} #{path}" do - before { process action, method: method, params: params } - - it "redirects to the admin profile page" do - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{moderator.id}/edit") - end - end - - end - end - context "when logged in as a moderator" do let(:moderator) { FactoryBot.create(:moderator_user) } before { login_as(moderator) } diff --git a/spec/controllers/admin/notes_controller_spec.rb b/spec/controllers/admin/notes_controller_spec.rb index d15492bde..0000fce9a 100644 --- a/spec/controllers/admin/notes_controller_spec.rb +++ b/spec/controllers/admin/notes_controller_spec.rb @@ -20,25 +20,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET /show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/petition_departments_controller_spec.rb b/spec/controllers/admin/petition_departments_controller_spec.rb index c85b0ac3d..bd6ddfd81 100644 --- a/spec/controllers/admin/petition_departments_controller_spec.rb +++ b/spec/controllers/admin/petition_departments_controller_spec.rb @@ -19,25 +19,6 @@ end end - context "when logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/petitions/:petition_id/departments" do - it "redirects to the edit profile page" do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "PATCH /admin/petitions/:petition_id/departments" do - it "redirects to the edit profile page" do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/petition_details_controller_spec.rb b/spec/controllers/admin/petition_details_controller_spec.rb index d6578ff47..9444b088c 100644 --- a/spec/controllers/admin/petition_details_controller_spec.rb +++ b/spec/controllers/admin/petition_details_controller_spec.rb @@ -20,25 +20,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET #show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH #update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context 'logged in as moderator user' do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/petition_emails_controller_spec.rb b/spec/controllers/admin/petition_emails_controller_spec.rb index e171426af..4335d2d76 100644 --- a/spec/controllers/admin/petition_emails_controller_spec.rb +++ b/spec/controllers/admin/petition_emails_controller_spec.rb @@ -89,48 +89,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:email) { FactoryBot.create(:petition_email, petition: petition) } - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - - before { login_as(user) } - - describe 'GET /new' do - it 'redirects to edit profile page' do - get :new, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'POST /' do - it 'redirects to edit profile page' do - post :create, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'GET /:id/edit' do - it 'redirects to the login page' do - get :edit, params: { petition_id: petition.id, id: email.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /:id' do - it 'redirects to the login page' do - patch :update, params: { petition_id: petition.id, id: email.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'DELETE /:id' do - it 'redirects to the login page' do - patch :destroy, params: { petition_id: petition.id, id: email.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/petition_tags_controller_spec.rb b/spec/controllers/admin/petition_tags_controller_spec.rb index 709a78741..46f8acbd9 100644 --- a/spec/controllers/admin/petition_tags_controller_spec.rb +++ b/spec/controllers/admin/petition_tags_controller_spec.rb @@ -19,25 +19,6 @@ end end - context "when logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/petitions/:petition_id/tags" do - it "redirects to the edit profile page" do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "PATCH /admin/petitions/:petition_id/tags" do - it "redirects to the edit profile page" do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/petition_topics_controller_spec.rb b/spec/controllers/admin/petition_topics_controller_spec.rb index 9c5c39cb0..cedc49a75 100644 --- a/spec/controllers/admin/petition_topics_controller_spec.rb +++ b/spec/controllers/admin/petition_topics_controller_spec.rb @@ -19,25 +19,6 @@ end end - context "when logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/petitions/:petition_id/topics" do - it "redirects to the edit profile page" do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "PATCH /admin/petitions/:petition_id/topics" do - it "redirects to the edit profile page" do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/petitions_controller_spec.rb b/spec/controllers/admin/petitions_controller_spec.rb index 0e56c7249..8fd4ba438 100644 --- a/spec/controllers/admin/petitions_controller_spec.rb +++ b/spec/controllers/admin/petitions_controller_spec.rb @@ -17,25 +17,6 @@ end end - context "when logged in as a moderator but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/petitions" do - it "redirects to the edit profile page" do - get :index - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "GET /admin/petitions/:id" do - it "redirects to the edit profile page" do - get :show, params: { id: "100000" } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as a moderator" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/profile_controller_spec.rb b/spec/controllers/admin/profile_controller_spec.rb deleted file mode 100644 index 2cdd7d488..000000000 --- a/spec/controllers/admin/profile_controller_spec.rb +++ /dev/null @@ -1,116 +0,0 @@ -require 'rails_helper' - -RSpec.describe Admin::ProfileController, type: :controller, admin: true do - before :each do - @user = FactoryBot.create(:sysadmin_user, :password => 'Letmein1!', - :password_confirmation => 'Letmein1!', :force_password_reset => true) - end - - context "not logged in" do - describe "GET 'edit'" do - it "should redirect to the login page" do - get 'edit', params: { id: @user.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/login") - end - end - end - - context "logged in but need to reset password" do - before :each do - login_as(@user) - end - - it "should render successfully" do - expect(@user.has_to_change_password?).to be_truthy - get :edit, params: { id: 50000 } - expect(response).to be_successful - end - end - - context "logged in" do - before :each do - login_as(@user) - end - - describe "GET 'edit'" do - it "should render successfully" do - get :edit, params: { id: 50000 } - expect(response).to be_successful - end - end - - describe "GET 'update'" do - def do_patch(current_password, new_password, password_confirmation) - admin_user_attributes = { - current_password: current_password, - password: new_password, - password_confirmation: password_confirmation - } - patch :update, params: { id: 50000, admin_user: admin_user_attributes } - end - - context "successful password change" do - it "should update password" do - do_patch('Letmein1!', 'Letmeout1!', 'Letmeout1!') - @user.reload - expect(@user.valid_password?('Letmeout1!')).to be_truthy - end - - it "should update password_changed_at to current time" do - do_patch('Letmein1!', 'Letmeout1!', 'Letmeout1!') - @user.reload - expect(@user.password_changed_at).to be_within(1.second).of(Time.current) - end - - it "should set force_password_reset to false" do - expect(@user.force_password_reset).to be_truthy - do_patch('Letmein1!', 'Letmeout1!', 'Letmeout1!') - @user.reload - expect(@user.force_password_reset).to be_falsey - end - - it "should redirect" do - do_patch('Letmein1!', 'Letmeout1!', 'Letmeout1!') - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin") - end - end - - context "unsuccessful password change" do - it "should have current password incorrect" do - do_patch('Letmeout1!', 'Letmein1!', 'Letmein1!') - expect(@user.valid_password?('Letmeout1!')).to be_falsey - expect(@user.valid_password?('Letmein1!')).to be_truthy - expect(assigns[:current_user].errors[:current_password]).not_to be_blank - end - - it "should not update password_changed_at" do - do_patch('Letmeout1!', 'Letmein1!', 'Letmein1!') - @user.reload - expect(@user.password_changed_at).not_to be_within(1.second).of(Time.current) - expect(@user.valid_password?('Letmein1!')).to be_truthy - end - - it "should have current password and new password are the same" do - do_patch('Letmein1!', 'Letmein1!', 'Letmein1!') - expect(assigns[:current_user].errors[:password]).not_to be_blank - @user.reload - expect(@user.valid_password?('Letmein1!')).to be_truthy - end - - it "should have new password as invalid" do - do_patch('Letmein1!', 'aB1defgh', 'aB1defgh') - expect(assigns[:current_user].errors[:password]).not_to be_blank - @user.reload - expect(@user.valid_password?('Letmein1!')).to be_truthy - end - - it "should have password as invalid when confirmation is different" do - do_patch('Letmein1!', 'aB1!efgh', 'aB1defgh') - expect(assigns[:current_user].errors[:password_confirmation]).not_to be_blank - @user.reload - expect(@user.valid_password?('Letmein1!')).to be_truthy - end - end - end - end -end diff --git a/spec/controllers/admin/schedule_debate_controller_spec.rb b/spec/controllers/admin/schedule_debate_controller_spec.rb index 2f191f2b9..9252e5ba2 100644 --- a/spec/controllers/admin/schedule_debate_controller_spec.rb +++ b/spec/controllers/admin/schedule_debate_controller_spec.rb @@ -41,25 +41,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET /show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/searches_controller_spec.rb b/spec/controllers/admin/searches_controller_spec.rb index 6c1ba092f..a3ddde258 100644 --- a/spec/controllers/admin/searches_controller_spec.rb +++ b/spec/controllers/admin/searches_controller_spec.rb @@ -10,18 +10,6 @@ end end - context "when logged in as a moderator but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/search" do - it "redirects to the edit profile page" do - get :show, params: { type: "petition", q: "foo" } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as a moderator" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/signatures_controller_spec.rb b/spec/controllers/admin/signatures_controller_spec.rb index 7f1342f75..1faa8d651 100644 --- a/spec/controllers/admin/signatures_controller_spec.rb +++ b/spec/controllers/admin/signatures_controller_spec.rb @@ -20,25 +20,6 @@ end end - context "logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe "GET /admin/signatures" do - it "redirects to the login page" do - get :index, params: { q: "Alice" } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe "DELETE /admin/signatures/:id" do - it "redirects to edit profile page" do - delete :destroy, params: { id: signature.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } let(:token) { SecureRandom.base64(32) } diff --git a/spec/controllers/admin/take_down_controller_spec.rb b/spec/controllers/admin/take_down_controller_spec.rb index c073a5a03..e323d89b4 100644 --- a/spec/controllers/admin/take_down_controller_spec.rb +++ b/spec/controllers/admin/take_down_controller_spec.rb @@ -48,25 +48,6 @@ end end - context 'logged in as moderator user but need to reset password' do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - before { login_as(user) } - - describe 'GET /show' do - it 'redirects to edit profile page' do - get :show, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - - describe 'PATCH /update' do - it 'redirects to edit profile page' do - patch :update, params: { petition_id: petition.id } - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } before { login_as(user) } diff --git a/spec/controllers/admin/trending_domains_controller_spec.rb b/spec/controllers/admin/trending_domains_controller_spec.rb index 80a341ad7..16f01cdad 100644 --- a/spec/controllers/admin/trending_domains_controller_spec.rb +++ b/spec/controllers/admin/trending_domains_controller_spec.rb @@ -13,24 +13,6 @@ end end - context "when logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - - before do - login_as(user) - end - - describe "GET /admin/petitions/200000/trending-domains" do - before do - get :index, params: { petition_id: "200000" } - end - - it "redirects to edit profile page" do - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } diff --git a/spec/controllers/admin/trending_ips_controller_spec.rb b/spec/controllers/admin/trending_ips_controller_spec.rb index b9c3c0479..25c1712e8 100644 --- a/spec/controllers/admin/trending_ips_controller_spec.rb +++ b/spec/controllers/admin/trending_ips_controller_spec.rb @@ -13,24 +13,6 @@ end end - context "when logged in as moderator user but need to reset password" do - let(:user) { FactoryBot.create(:moderator_user, force_password_reset: true) } - - before do - login_as(user) - end - - describe "GET /admin/petitions/200000/trending-ips" do - before do - get :index, params: { petition_id: "200000" } - end - - it "redirects to edit profile page" do - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/profile/#{user.id}/edit") - end - end - end - context "when logged in as moderator user" do let(:user) { FactoryBot.create(:moderator_user) } From 902c078f99f8146e613aa333737a73661cb61938 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 15 Dec 2023 11:17:02 +0000 Subject: [PATCH 08/10] Remove the admin user CRUD feature --- .../admin/admin_users_controller.rb | 60 ------- app/views/admin/admin_users/_form.html.erb | 65 ------- app/views/admin/admin_users/edit.html.erb | 6 - app/views/admin/admin_users/index.html.erb | 16 +- app/views/admin/admin_users/new.html.erb | 4 - config/brakeman.ignore | 20 --- config/routes.rb | 2 +- features/admin/admin_users.feature | 34 ++++ features/admin/admin_users_crud.feature | 98 ----------- features/step_definitions/pickle_steps.rb | 4 + .../admin/admin_users_controller_spec.rb | 162 ------------------ 11 files changed, 42 insertions(+), 429 deletions(-) delete mode 100644 app/views/admin/admin_users/_form.html.erb delete mode 100644 app/views/admin/admin_users/edit.html.erb delete mode 100644 app/views/admin/admin_users/new.html.erb create mode 100644 features/admin/admin_users.feature delete mode 100644 features/admin/admin_users_crud.feature diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 3af497500..28ee372a3 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -1,67 +1,7 @@ class Admin::AdminUsersController < Admin::AdminController before_action :require_sysadmin - before_action :find_user, only: %i[edit update destroy] - - rescue_from AdminUser::CannotDeleteCurrentUser do - redirect_to admin_admin_users_url, alert: :user_is_current_user - end - - rescue_from AdminUser::MustBeAtLeastOneAdminUser do - redirect_to admin_admin_users_url, alert: :user_count_is_too_low - end - - rescue_from ActiveRecord::DeleteRestrictionError do - redirect_to admin_admin_users_url, alert: :user_has_moderated_petitions - end def index @users = AdminUser.by_name.paginate(page: params[:page], per_page: 50) end - - def new - @user = AdminUser.new - end - - def create - @user = AdminUser.new(admin_user_params) - - if @user.save - redirect_to admin_admin_users_url, notice: :user_created - else - render :new - end - end - - def edit - end - - def update - if @user.update(admin_user_params) - redirect_to admin_admin_users_url, notice: :user_updated - else - render :edit - end - end - - def destroy - if @user.destroy(current_user: current_user) - redirect_to admin_admin_users_url, notice: :user_deleted - else - redirect_to admin_admin_users_url, alert: :user_not_deleted - end - end - - protected - - def find_user - @user = AdminUser.find(params[:id]) - end - - def admin_user_params - params. - require(:admin_user). - permit(:password, :password_confirmation, :first_name, - :last_name, :role, :email, :force_password_reset, - :account_disabled) - end end diff --git a/app/views/admin/admin_users/_form.html.erb b/app/views/admin/admin_users/_form.html.erb deleted file mode 100644 index 458be9fab..000000000 --- a/app/views/admin/admin_users/_form.html.erb +++ /dev/null @@ -1,65 +0,0 @@ -<%= form_for [:admin, @user] do |f| -%> - <%= form_row for: [f.object, :first_name] do %> - <%= f.label :first_name, class: 'form-label form-label-inline' %><%= mandatory_field %> - <%= error_messages_for_field f.object, :first_name %> - <%= f.text_field :first_name, tabindex: increment, class: 'form-control', autofocus: 'autofocus' %> - <% end %> - - <%= form_row for: [f.object, :last_name] do %> - <%= f.label :last_name, class: 'form-label form-label-inline' %><%= mandatory_field %> - <%= error_messages_for_field f.object, :last_name %> - <%= f.text_field :last_name, tabindex: increment, class: 'form-control' %> - <% end %> - - <%= form_row for: [f.object, :email] do %> - <%= f.label :email, class: 'form-label form-label-inline' %><%= mandatory_field %> - <%= error_messages_for_field f.object, :email %> - <%= f.email_field :email, tabindex: increment, class: 'form-control' %> - <% end %> - - <%= form_row for: [f.object, :email], class: 'inline' do %> - <%= f.label :role, class: 'form-label' %> - <%= error_messages_for_field f.object, :role %> - <% AdminUser::ROLES.each do |role| %> -
    - <%= f.radio_button :role, role %> - <%= f.label :role, role, for: "admin_user_role_#{role}" %> -
    - <% end %> - <% end %> - - <%= form_row for: [f.object, :force_password_reset] do %> - <%= error_messages_for_field f.object, :force_password_reset %> -
    - <%= f.check_box :force_password_reset, tabindex: increment %> - <%= f.label :force_password_reset %> -
    - <% end %> - - <%= form_row for: [f.object, :account_disabled] do %> - <%= error_messages_for_field f.object, :account_disabled %> -
    - <%= f.check_box :account_disabled, tabindex: increment %> - <%= f.label :account_disabled %> -
    - <% end %> - - <%= field_set_tag do %> - <%= content_tag(:legend, 'Passwords', class: 'form-label-bold') %> - <%= content_tag(:p, 'Note, no password needs to be entered unless you want to change it') unless f.object.new_record? %> - <%= form_row for: [f.object, :password] do %> - <%= f.label :password, class: 'form-label form-label-inline' %><%= mandatory_field if f.object.new_record? %> - <%= error_messages_for_field f.object, :password %> - <%= f.password_field :password, tabindex: increment, class: 'form-control', autocomplete: 'off' %> - <% end %> - - <%= form_row for: [f.object, :password_confirmation] do %> - <%= f.label :password_confirmation, class: 'form-label form-label-inline' %><%= mandatory_field if f.object.new_record? %> - <%= error_messages_for_field f.object, :password_confirmation %> - <%= f.password_field :password_confirmation, tabindex: increment, class: 'form-control', autocomplete: 'off' %> - <% end %> - <% end %> - - - <%= f.submit 'Save', class: "button" %> -<% end -%> diff --git a/app/views/admin/admin_users/edit.html.erb b/app/views/admin/admin_users/edit.html.erb deleted file mode 100644 index 3b6470a30..000000000 --- a/app/views/admin/admin_users/edit.html.erb +++ /dev/null @@ -1,6 +0,0 @@ -

    Edit user

    -
    -
    - <%= render :partial => 'form' %> -
    -
    diff --git a/app/views/admin/admin_users/index.html.erb b/app/views/admin/admin_users/index.html.erb index cff2fb66e..906fbe783 100644 --- a/app/views/admin/admin_users/index.html.erb +++ b/app/views/admin/admin_users/index.html.erb @@ -1,26 +1,16 @@ -<%= link_to 'New user', new_admin_admin_user_path, class: "button" %> - - - + <%- @users.each do |user| -%> - + - - + <%- end -%> <%= will_paginate(@users) if @users.any? %> diff --git a/app/views/admin/admin_users/new.html.erb b/app/views/admin/admin_users/new.html.erb deleted file mode 100644 index 85463f352..000000000 --- a/app/views/admin/admin_users/new.html.erb +++ /dev/null @@ -1,4 +0,0 @@ -

    New user

    -
    - <%= render :partial => 'form' %> -
    \ No newline at end of file diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 3c5131577..59e71e92b 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -128,26 +128,6 @@ "user_input": null, "confidence": "Medium", "note": "" - }, - { - "warning_type": "Mass Assignment", - "warning_code": 105, - "fingerprint": "f523941f55e48c9af1dda1b71dd47c35e8cd6bce092c90e04bc940d257bf44e8", - "check_name": "PermitAttributes", - "message": "Potentially dangerous key allowed for mass assignment", - "file": "app/controllers/admin/admin_users_controller.rb", - "line": 64, - "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/", - "code": "params.require(:admin_user).permit(:password, :password_confirmation, :first_name, :last_name, :role, :email, :force_password_reset, :account_disabled)", - "render_path": null, - "location": { - "type": "method", - "class": "Admin::AdminUsersController", - "method": "admin_user_params" - }, - "user_input": ":role", - "confidence": "Medium", - "note": "" } ], "updated": "2023-03-30 00:48:36 +0100", diff --git a/config/routes.rb b/config/routes.rb index 6a4d6288b..5ff85c8f8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -130,7 +130,7 @@ resource :parliament, only: %i[show update] resource :search, only: %i[show] - resources :admin_users + resources :admin_users, only: %[index] resources :profile, only: %i[edit update] resources :invalidations, except: %i[show] do diff --git a/features/admin/admin_users.feature b/features/admin/admin_users.feature new file mode 100644 index 000000000..261357fac --- /dev/null +++ b/features/admin/admin_users.feature @@ -0,0 +1,34 @@ +@admin +Feature: Admin users index and crud + As a sysadmin + I can see the admin users index and crud admin users + + Background: + Given I am logged in as a sysadmin with the email "muddy@fox.com", first_name "Sys", last_name "Admin" + And a moderator user exists with email: "naomi@example.com", first_name: "Naomi", last_name: "Campbell" + + Scenario: Accessing the admin users index + When I go to the admin home page + And I follow "Users" + Then I should be on the admin users index page + And I should be connected to the server via an ssl connection + + Scenario: Ordering of the users index + Given a moderator user exists with email: "derek@example.com", first_name: "Derek", last_name: "Jacobi" + And a moderator user exists with email: "helen@example.com", first_name: "Helen", last_name: "Hunt", current_sign_in_at: "2023-12-15 10:09:31" + When I go to the admin users index page + Then I should see the following admin user table: + | Name | Email | Role | Last login | + | Admin, Sys | muddy@fox.com | sysadmin | | + | Campbell, Naomi | naomi@example.com | moderator | | + | Hunt, Helen | helen@example.com | moderator | 10:09am on 15 December 2023 | + | Jacobi, Derek | derek@example.com | moderator | | + And the markup should be valid + + Scenario: Pagination of the users index + Given 50 moderator users exist + When I go to the admin users index page + And I follow "Next" + Then I should see 2 rows in the admin user table + And I follow "Previous" + Then I should see 50 rows in the admin user table diff --git a/features/admin/admin_users_crud.feature b/features/admin/admin_users_crud.feature deleted file mode 100644 index c07b7488d..000000000 --- a/features/admin/admin_users_crud.feature +++ /dev/null @@ -1,98 +0,0 @@ -@admin -Feature: Admin users index and crud - As a sysadmin - I can see the admin users index and crud admin users - - Background: - Given I am logged in as a sysadmin with the email "muddy@fox.com", first_name "Sys", last_name "Admin" - And a moderator user exists with email: "naomi@example.com", first_name: "Naomi", last_name: "Campbell" - - Scenario: Accessing the admin users index - When I go to the admin home page - And I follow "Users" - Then I should be on the admin users index page - And I should be connected to the server via an ssl connection - - Scenario: Ordering of the users index - Given a moderator user exists with email: "derek@example.com", first_name: "Derek", last_name: "Jacobi" - And a moderator user exists with email: "helen@example.com", first_name: "Helen", last_name: "Hunt", failed_attempts: 5 - When I go to the admin users index page - Then I should see the following admin user table: - | Name | Email | Role | Disabled | - | Admin, Sys | muddy@fox.com | sysadmin | | - | Campbell, Naomi | naomi@example.com | moderator | | - | Hunt, Helen | helen@example.com | moderator | Yes | - | Jacobi, Derek | derek@example.com | moderator | | - And the markup should be valid - - Scenario: Pagination of the users index - Given 50 moderator users exist - When I go to the admin users index page - And I follow "Next" - Then I should see 2 rows in the admin user table - And I follow "Previous" - Then I should see 50 rows in the admin user table - - Scenario: Inspecting the new user form - When I go to the admin users index page - And I follow "New user" - Then I should be on the admin new user page - And I should be connected to the server via an ssl connection - And I should see a "First name" text field - And I should see a "Last name" text field - And I should see a "Email" email field - And I should see a "sysadmin" radio field - And I should see a "moderator" radio field - And I should see a "Force password reset" checkbox field - And I should see a "Account disabled" checkbox field - And I should see a "Password" password field - And I should see a "Password confirmation" password field - - Scenario: Creating a new user - When I go to the admin users index page - And I follow "New user" - And I fill in "First name" with "Derek" - And I fill in "Last name" with "Jacobi" - And I fill in "Email" with "derek@example.com" - And I choose "sysadmin" - And I fill in "Password" with "Letmein1!" - And I fill in "Password confirmation" with "Letmein1!" - And I press "Save" - Then I should be on the admin users index page - And I should see "derek@example.com" - When I follow "Jacobi, Derek" - Then the "sysadmin" radio button should be chosen - And the "Account disabled" checkbox should not be checked - - Scenario: Updating a user - When I go to the admin users index page - And I follow "Campbell, Naomi" - And I fill in "First name" with "Nolene" - And I fill in "Email" with "helen@example.com" - And I check "Account disabled" - And I press "Save" - Then I should be on the admin users index page - And I should see "helen@example.com" - When I follow "Campbell, Nolene" - And the "Email" field should contain "helen@example.com" - And the "Account disabled" checkbox should be checked - And a moderator user should exist with email: "helen@example.com", failed_attempts: "5" - And I should be connected to the server via an ssl connection - - Scenario: Enabling a user's disabled account - Given a moderator user exists with email: "derek@example.com", first_name: "Derek", last_name: "Jacobi", failed_attempts: "5" - When I go to the admin users index page - And I follow "Jacobi, Derek" - And the "Account disabled" checkbox should be checked - And I uncheck "Account disabled" - And I press "Save" - Then I should be on the admin users index page - When I follow "Jacobi, Derek" - And the "Account disabled" checkbox should not be checked - And a moderator user should exist with email: "derek@example.com", failed_attempts: "0" - - Scenario: Deleting a user - When I go to the admin users index page - And I follow "Delete" for "naomi@example.com" - Then a admin user should not exist with email: "naomi@example.com" - And the row with the name "naomi@example.com" is not listed diff --git a/features/step_definitions/pickle_steps.rb b/features/step_definitions/pickle_steps.rb index 2d7822789..9a804e693 100644 --- a/features/step_definitions/pickle_steps.rb +++ b/features/step_definitions/pickle_steps.rb @@ -18,6 +18,10 @@ @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, email: email) end +Given(/^a moderator user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)", current_sign_in_at: "([^"]*)"$/) do |email, first_name, last_name, current_sign_in_at| + @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, email: email, current_sign_in_at: current_sign_in_at) +end + Given(/^a moderator user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)", failed_attempts: (\d+)$/) do |email, first_name, last_name, failed_attempts| @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, email: email, failed_attempts: failed_attempts) end diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index 02f23a659..de055aec5 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -8,13 +8,6 @@ expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/login") end end - - describe "GET 'new'" do - it "should redirect to the login page" do - get :new - expect(response).to redirect_to("https://moderate.petition.parliament.uk/admin/login") - end - end end context "logged in as moderator user" do @@ -55,160 +48,5 @@ expect(assigns[:users]).to eq([user, @user4, @user2, @user1, @user3]) end end - - describe "GET 'new'" do - it "should be successful" do - get :new - expect(response).to be_successful - end - - it "should assign a new user" do - get :new - expect(assigns[:user]).to be_a(AdminUser) - expect(assigns[:user]).to be_new_record - end - end - - describe "POST 'create'" do - def do_create - post :create, params: { admin_user: admin_user_attributes } - end - - describe "with valid params" do - let(:admin_user_attributes) do - { - :first_name => 'John', - :last_name => 'Kennedy', - :role => 'moderator', - :email => 'admin@example.com', - :password => 'Letmein1!', - :password_confirmation => 'Letmein1!' - } - end - - it "should create a new user" do - expect do - do_create - end.to change(AdminUser, :count).by(1) - end - - it "should redirect to the index" do - do_create - expect(response).to redirect_to(:action => :index) - end - end - - describe "with invalid params" do - let(:admin_user_attributes) do - { - :email => 'admin@example.com' - } - end - - it "should not create a new user" do - expect { - do_create - }.not_to change(AdminUser, :count) - end - - it "should re-render the new template" do - do_create - expect(response).to render_template('new') - end - end - end - - describe "GET 'edit'" do - let(:edit_user) { FactoryBot.create(:moderator_user) } - - def do_get - get :edit, params: { id: edit_user.to_param } - end - - it "should be successful" do - do_get - expect(response).to be_successful - end - - it "should assign the user" do - do_get - expect(assigns[:user]).to be_a(AdminUser) - expect(assigns[:user]).to eq edit_user - end - end - - describe "PUT 'update'" do - let(:edit_user) { FactoryBot.create(:moderator_user, :email => "admin@example.com", :failed_attempts => 5) } - - def do_update - patch :update, params: { - id: edit_user.to_param, - admin_user: admin_user_attributes - } - end - - describe "with valid params" do - let(:admin_user_attributes) do - { - :email => "another_admin@example.com", - :account_disabled => '0' - } - end - - it "should update the user and redirect to the index" do - do_update - edit_user.reload - expect(edit_user.email).to eq("another_admin@example.com") - expect(response).to redirect_to(:action => :index) - end - - it "should reset the failed login count to 0" do - do_update - edit_user.reload - expect(edit_user.failed_attempts).to eq(0) - end - end - - describe "with invalid params" do - let(:admin_user_attributes) do - { - :email => "bademailaddress" - } - end - - it "should not update the user" do - do_update - edit_user.reload - expect(edit_user.email).to eq("admin@example.com") - expect(response).to render_template('edit') - end - end - end - - describe "DELETE 'destroy'" do - let(:delete_user) { FactoryBot.create(:moderator_user, :email => 'admin@example.com') } - - it "deletes the requested user" do - delete :destroy, params: { id: delete_user.to_param } - expect(AdminUser.exists?(delete_user.id)).to be_falsey - expect(response).to redirect_to(:action => :index) - end - - it "will not let you delete yourself" do - delete :destroy, params: { id: user.to_param } - expect(AdminUser.exists?(user.id)).to be_truthy - expect(response).to redirect_to(:action => :index) - expect(flash[:alert]).to eq "You are not allowed to delete yourself!" - end - - it "will not let you delete users that have moderated petitions" do - petition = FactoryBot.create(:open_petition, moderated_by: delete_user) - - delete :destroy, params: { id: delete_user.to_param } - expect(AdminUser.exists?(user.id)).to be_truthy - expect(response).to redirect_to(:action => :index) - expect(flash[:alert]).to eq "The user has moderated petitions so you can only deactivate their account" - end - end end end From 7fd91b5ed0b45ac0e3ce1eef38b80cdba81f5780 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Sun, 17 Dec 2023 08:36:10 +0000 Subject: [PATCH 09/10] Remove database authentication and replace with omniauth --- Gemfile | 3 +- Gemfile.lock | 15 +- .../admin/omniauth_callbacks_controller.rb | 25 ++ app/models/admin_user.rb | 79 +----- app/views/admin/sessions/new.html.erb | 12 +- config/initializers/devise.rb | 22 +- config/initializers/omniauth.rb | 3 + config/locales/activerecord.en-GB.yml | 8 - config/locales/admin.en-GB.yml | 4 +- config/locales/devise.en-GB.yml | 2 + config/routes.rb | 7 +- features/admin/admin_access.feature | 42 +-- features/admin/admin_users.feature | 9 +- .../step_definitions/authentication_steps.rb | 9 +- features/step_definitions/pickle_steps.rb | 28 +- .../encryptors/authlogic_scrypt.rb | 29 -- lib/tasks/epets.rake | 1 - spec/factories.rb | 3 - spec/models/admin_user_spec.rb | 263 +----------------- .../admin_user_persistence_token_spec.rb | 14 +- spec/requests/bad_requests_spec.rb | 4 +- spec/requests/csrf_token_spec.rb | 12 +- spec/requests/login_timeout_spec.rb | 8 +- 23 files changed, 104 insertions(+), 498 deletions(-) create mode 100644 app/controllers/admin/omniauth_callbacks_controller.rb create mode 100644 config/initializers/omniauth.rb delete mode 100644 lib/devise/encryptable/encryptors/authlogic_scrypt.rb diff --git a/Gemfile b/Gemfile index 346d67df1..6d348bca0 100644 --- a/Gemfile +++ b/Gemfile @@ -8,7 +8,6 @@ gem 'rails', '6.1.7.6' gem 'rake' gem 'pg' gem 'devise' -gem 'devise-encryptable' gem 'will_paginate' gem 'json' gem 'delayed_job_active_record' @@ -35,6 +34,8 @@ gem 'image_processing' gem 'maxminddb' gem 'redcarpet' gem 'scrypt' +gem 'omniauth' +gem 'omniauth-rails_csrf_protection' gem 'aws-sdk-codedeploy' gem 'aws-sdk-cloudwatchlogs' diff --git a/Gemfile.lock b/Gemfile.lock index 5af3b1f35..5d6531bb2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -141,8 +141,6 @@ GEM railties (>= 4.1.0) responders warden (~> 1.2.3) - devise-encryptable (0.2.0) - devise (>= 2.1.0) diff-lcs (1.4.4) docile (1.3.1) dotenv (2.7.6) @@ -174,6 +172,7 @@ GEM globalid (1.2.1) activesupport (>= 6.1) hashdiff (1.0.1) + hashie (5.0.0) htmlentities (4.3.4) i18n (1.14.1) concurrent-ruby (~> 1.0) @@ -238,6 +237,13 @@ GEM racc (~> 1.4) nokogiri (1.15.4-x86_64-linux) racc (~> 1.4) + omniauth (2.1.1) + hashie (>= 3.4.6) + rack (>= 2.2.3) + rack-protection + omniauth-rails_csrf_protection (1.0.1) + actionpack (>= 4.2) + omniauth (~> 2.0) orm_adapter (0.5.0) pg (1.2.3) pry (0.13.1) @@ -248,6 +254,8 @@ GEM nio4r (~> 2.0) racc (1.7.1) rack (2.2.8) + rack-protection (3.1.0) + rack (~> 2.2, >= 2.2.4) rack-test (2.1.0) rack (>= 1.3) rails (6.1.7.6) @@ -396,7 +404,6 @@ DEPENDENCIES delayed-web delayed_job_active_record devise - devise-encryptable dotenv-rails email_spec factory_bot_rails @@ -413,6 +420,8 @@ DEPENDENCIES maxminddb net-http-persistent nokogiri + omniauth + omniauth-rails_csrf_protection pg pry puma (< 6) diff --git a/app/controllers/admin/omniauth_callbacks_controller.rb b/app/controllers/admin/omniauth_callbacks_controller.rb new file mode 100644 index 000000000..95ac35bbd --- /dev/null +++ b/app/controllers/admin/omniauth_callbacks_controller.rb @@ -0,0 +1,25 @@ +class Admin::OmniauthCallbacksController < Devise::OmniauthCallbacksController + skip_before_action :require_admin + skip_before_action :verify_authenticity_token, only: %i[developer] + + def developer + @user = AdminUser.find_by(email: omniauth_hash["uid"]) + + if @user.present? + sign_in_and_redirect @user, event: :authentication + set_flash_message(:notice, :signed_in) if is_navigational_format? + else + redirect_to admin_login_url, alert: :invalid_login + end + end + + private + + def after_omniauth_failure_path_for(scope) + admin_login_url + end + + def omniauth_hash + request.env["omniauth.auth"] + end +end diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 0b9c809bc..f408fa6ea 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -1,15 +1,20 @@ class AdminUser < ActiveRecord::Base - DISABLED_LOGIN_COUNT = 5 SYSADMIN_ROLE = 'sysadmin' MODERATOR_ROLE = 'moderator' REVIEWER_ROLE = 'reviewer' ROLES = [SYSADMIN_ROLE, MODERATOR_ROLE, REVIEWER_ROLE] - PASSWORD_REGEX = /\A.*(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[\W_]).*\z/ class CannotDeleteCurrentUser < RuntimeError; end class MustBeAtLeastOneAdminUser < RuntimeError; end - devise :database_authenticatable, :encryptable, :trackable, :timeoutable, :lockable + devise :trackable, :timeoutable, :omniauthable, omniauth_providers: %i[developer] + + # TODO: Drop these columns once rollout of SSO has been completed + self.ignored_columns = %i[ + encrypted_password password_salt + force_password_reset password_changed_at + failed_attempts locked_at + ] with_options dependent: :restrict_with_exception do with_options foreign_key: :moderated_by_id do @@ -22,23 +27,10 @@ class MustBeAtLeastOneAdminUser < RuntimeError; end validates :first_name, :last_name, presence: true validates :email, presence: true, email: true validates :email, uniqueness: { case_sensitive: false } - validates :password, presence: true, on: [:create, :update_password] - validates :password, length: { minimum: 8, allow_blank: true } - validates :password, format: { with: PASSWORD_REGEX, allow_blank: true } - validates :password, confirmation: true, on: :update_password validates :role, inclusion: { in: ROLES } - validate on: :update_password do - errors.add(:current_password, :blank) if current_password.blank? - errors.add(:current_password, :invalid) unless valid_password?(current_password) - errors.add(:password, :taken) if valid_password?(password) - end - # = Callbacks = - before_save :clear_locked_at, if: :enabling_account? - before_save :set_locked_at, if: :disabling_account? before_save :reset_persistence_token, unless: :persistence_token? - before_update :reset_password_tracking, if: :encrypted_password_changed? # = Finders = scope :by_name, -> { order(:last_name, :first_name) } @@ -49,11 +41,6 @@ def self.timeout_in Site.login_timeout.seconds end - def reset_password_tracking - self.force_password_reset = false - self.password_changed_at = Time.current - end - def reset_persistence_token self.persistence_token = SecureRandom.hex(64) end @@ -66,28 +53,6 @@ def valid_persistence_token?(token) persistence_token == token end - def current_password - defined?(@current_password) ? @current_password : nil - end - - def current_password=(value) - @current_password = value - end - - def valid_password?(password) - encryptor_class.compare(encrypted_password_in_database, password, nil, password_salt_in_database, nil) - end - - def update_password(params) - assign_attributes(params) - - save(context: :update_password).tap do - self.current_password = nil - self.password = nil - self.password_confirmation = nil - end - end - def destroy(current_user: nil) if self == current_user raise CannotDeleteCurrentUser, "Cannot delete current user" @@ -122,10 +87,6 @@ def is_a_reviewer? self.role == 'reviewer' end - def has_to_change_password? - self.force_password_reset or (self.password_changed_at and self.password_changed_at < 9.months.ago) - end - def can_take_petitions_down? is_a_sysadmin? || is_a_moderator? end @@ -142,30 +103,6 @@ def can_moderate_petitions? is_a_sysadmin? || is_a_moderator? end - def account_disabled - self.failed_attempts >= DISABLED_LOGIN_COUNT - end - - def account_disabled=(flag) - self.failed_attempts = (flag == "0" or !flag) ? 0 : DISABLED_LOGIN_COUNT - end - - def enabling_account? - failed_attempts_changed? && failed_attempts.zero? - end - - def disabling_account? - failed_attempts_changed? && failed_attempts >= DISABLED_LOGIN_COUNT - end - - def clear_locked_at - self.locked_at = nil - end - - def set_locked_at(now = Time.current) - self.locked_at = now - end - def elapsed_time(last_request_at, now = Time.current) (now - last_request_at).floor end diff --git a/app/views/admin/sessions/new.html.erb b/app/views/admin/sessions/new.html.erb index e8586997f..cc337b429 100644 --- a/app/views/admin/sessions/new.html.erb +++ b/app/views/admin/sessions/new.html.erb @@ -1,16 +1,8 @@

    Sign in

    - <%= form_for(resource, as: resource_name, url: admin_login_path) do |f| %> -
    - <%= f.label :email, "Email", class: "form-label" %> - <%= f.text_field :email, :class => 'form-control', :type => 'email', :autofocus => 'autofocus' %> -
    -
    - <%= f.label :password, "Password", class: "form-label" %> - <%= f.password_field :password, :class => 'form-control' %> -
    - <%= f.submit 'Sign in', :class => 'button' %> + <%= form_tag(admin_auth_developer_path, method: :post) do %> + <% end %>
    diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 738457ff3..a3014e056 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -1,5 +1,3 @@ -require 'devise/encryptable/encryptors/authlogic_scrypt' - Devise.setup do |config| # ==> Controller configuration config.parent_controller = 'Admin::AdminController' @@ -17,29 +15,17 @@ config.case_insensitive_keys = [:email] config.strip_whitespace_keys = [:email] - # ==> Configuration for :database_authenticatable - config.stretches = Rails.env.test? ? 1 : 12 - - # ==> Configuration for :validatable - config.password_length = 6..128 - config.email_regexp = /\A[^@\s]+@[^@\s]+\z/ - # ==> Configuration for :timeoutable # This is overidden on the AdminUser model to use Site.login_timeout config.timeout_in = 24.hours - # ==> Configuration for :lockable - config.lock_strategy = :failed_attempts - config.unlock_strategy = :none - config.maximum_attempts = 5 - config.last_attempt_warning = true - - # ==> Configuration for :encryptable - config.encryptor = :authlogic_scrypt - # ==> Navigation configuration config.sign_out_via = :get + # ==> Omniauth configuration + config.omniauth_path_prefix = '/admin/auth' + config.omniauth :developer, fields: %i[email] + # ==> Warden configuration # Reset the token after logging in so that other sessions are logged out Warden::Manager.after_set_user except: :fetch do |user, warden, options| diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb new file mode 100644 index 000000000..ed8872266 --- /dev/null +++ b/config/initializers/omniauth.rb @@ -0,0 +1,3 @@ +OmniAuth.configure do |config| + config.logger = Rails.logger +end diff --git a/config/locales/activerecord.en-GB.yml b/config/locales/activerecord.en-GB.yml index 768b30b38..30fb9e9c2 100644 --- a/config/locales/activerecord.en-GB.yml +++ b/config/locales/activerecord.en-GB.yml @@ -36,14 +36,6 @@ en-GB: models: admin_user: attributes: - current_password: - blank: "Current password can’t be blank" - invalid: "Current password is incorrect" - password: - invalid: "Password must contain at least one digit, a lower and upper case letter and a special character" - taken: "Password is the same as the current password" - password_confirmation: - confirmation: "Password confirmation doesn’t match password" role: inclusion: "Role '%{value}' is invalid" diff --git a/config/locales/admin.en-GB.yml b/config/locales/admin.en-GB.yml index 55993e2f8..501f50d66 100644 --- a/config/locales/admin.en-GB.yml +++ b/config/locales/admin.en-GB.yml @@ -73,9 +73,7 @@ en-GB: invalidation_removed: "Invalidation removed successfully" invalidation_started: "Enqueued the invalidation %{summary}" invalidation_updated: "Invalidation updated successfully" - last_login: "You have one more attempt before your account is temporarily disabled" - disabled_login: "Failed login limit exceeded, your account has been temporarily disabled" - invalid_login: "Invalid email/password combination" + invalid_login: "Invalid login details" logged_out: "You have been logged out" moderator_required: "You must be logged in as a moderator or system administrator to view this page" moderation_delay_sent: "An email has been sent to creators that moderation has been delayed" diff --git a/config/locales/devise.en-GB.yml b/config/locales/devise.en-GB.yml index 976ce48ff..cd2e5af47 100644 --- a/config/locales/devise.en-GB.yml +++ b/config/locales/devise.en-GB.yml @@ -10,6 +10,8 @@ en: other_login: "You have been logged out" not_found_in_database: "Invalid email/password combination" timedout: "Your session expired. Please log in again to continue" + omniauth_callbacks: + signed_in: "Logged in successfully" sessions: signed_in: "Logged in successfully" signed_out: "You have been logged out" diff --git a/config/routes.rb b/config/routes.rb index 5ff85c8f8..3a7af4515 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -247,12 +247,11 @@ end - devise_for :users, class_name: 'AdminUser', skip: %i[sessions] + devise_for :users, class_name: 'AdminUser', module: 'admin', skip: %i[sessions] as :user do controller 'admin/sessions' do get '/admin/login', action: 'new' - post '/admin/login', action: 'create', as: nil get '/admin/logout', action: 'destroy' get '/admin/continue', action: 'continue' get '/admin/status', action: 'status' @@ -263,5 +262,9 @@ # Devise needs a `new_user_session_url` helper for its failure app direct(:new_user_session) { route_for(:admin_login) } + # Friendly url helpers for Omniauth + direct(:admin_auth_developer) { route_for(:user_developer_omniauth_authorize) } + direct(:admin_auth_developer_callback) { route_for(:user_developer_omniauth_callback) } + get 'ping', to: 'ping#ping' end diff --git a/features/admin/admin_access.feature b/features/admin/admin_access.feature index 9c4c49d27..7bbafbb2a 100644 --- a/features/admin/admin_access.feature +++ b/features/admin/admin_access.feature @@ -16,11 +16,11 @@ Feature: Restricted access to the admin console And the markup should be valid Scenario: Login and logout to the admin console as a sysadmin - Given a sysadmin user exists with first_name: "John", last_name: "Admin", email: "admin@example.com", password: "Letmein1!", password_confirmation: "Letmein1!" + Given a sysadmin user exists with email: "admin@example.com", first_name: "John", last_name: "Admin" When I go to the admin login page + And I press "Login with developer strategy" And I fill in "Email" with "admin@example.com" - And I fill in "Password" with "Letmein1!" - And I press "Sign in" + And I press "Sign In" Then I should be on the admin home page And I should be connected to the server via an ssl connection And the markup should be valid @@ -30,11 +30,11 @@ Feature: Restricted access to the admin console And I should be on the admin login page Scenario: Login and logout to the admin console as a moderator user - Given a moderator user exists with first_name: "John", last_name: "Moderator", email: "admin@example.com", password: "Letmein1!", password_confirmation: "Letmein1!" + Given a moderator user exists with email: "admin@example.com", first_name: "John", last_name: "Moderator" When I go to the admin login page + And I press "Login with developer strategy" And I fill in "Email" with "admin@example.com" - And I fill in "Password" with "Letmein1!" - And I press "Sign in" + And I press "Sign In" Then I should be on the admin home page And I should see "John Moderator" And I should not see "Users" @@ -43,32 +43,8 @@ Feature: Restricted access to the admin console Scenario: Invalid login Given I go to the admin login page + And I press "Login with developer strategy" And I fill in "Email" with "admin@example.com" - And I fill in "Password" with "letmein1" - And I press "Sign in" - Then I should see "Invalid email/password combination" + And I press "Sign In" + Then I should see "Invalid login details" And should not see "Logout" - - Scenario: 5 failed logins disables an account - Given a moderator user exists with email: "admin@example.com", password: "Letmein1!", password_confirmation: "Letmein1!" - And I go to the admin login page - And I try the password "wrong trousers" 3 times in a row - And I fill in "Email" with "admin@example.com" - And I fill in "Password" with "wrong trousers" - And I press "Sign in" - Then I should see "You have one more attempt before your account is temporarily disabled" - And should not see "Logout" - And I fill in "Email" with "admin@example.com" - And I fill in "Password" with "wrong trousers" - And I press "Sign in" - Then I should see "Failed login limit exceeded, your account has been temporarily disabled" - And should not see "Logout" - - Scenario: 5 failed logins with an email address containing a wildcard does not disable an account - Given a moderator user exists with email: "admin@example.com", password: "Letmein1!", password_confirmation: "Letmein1!" - And I go to the admin login page - And I try the password "wrong trousers" 5 times in a row for the email "admin%" - And I fill in "Email" with "admin@example.com" - And I fill in "Password" with "Letmein1!" - And I press "Sign in" - Then I should be on the admin home page diff --git a/features/admin/admin_users.feature b/features/admin/admin_users.feature index 261357fac..4f9ce4b22 100644 --- a/features/admin/admin_users.feature +++ b/features/admin/admin_users.feature @@ -4,7 +4,8 @@ Feature: Admin users index and crud I can see the admin users index and crud admin users Background: - Given I am logged in as a sysadmin with the email "muddy@fox.com", first_name "Sys", last_name "Admin" + Given the time is "Sun, 17 Dec 2023 08:28:41 +0000" + And I am logged in as a sysadmin with the email "muddy@fox.com", first_name "Sys", last_name "Admin" And a moderator user exists with email: "naomi@example.com", first_name: "Naomi", last_name: "Campbell" Scenario: Accessing the admin users index @@ -14,15 +15,15 @@ Feature: Admin users index and crud And I should be connected to the server via an ssl connection Scenario: Ordering of the users index - Given a moderator user exists with email: "derek@example.com", first_name: "Derek", last_name: "Jacobi" + Given a moderator user exists with email: "derek@example.com", first_name: "Derek", last_name: "Jacobi", current_sign_in_at: "2023-12-11 14:27:14" And a moderator user exists with email: "helen@example.com", first_name: "Helen", last_name: "Hunt", current_sign_in_at: "2023-12-15 10:09:31" When I go to the admin users index page Then I should see the following admin user table: | Name | Email | Role | Last login | - | Admin, Sys | muddy@fox.com | sysadmin | | + | Admin, Sys | muddy@fox.com | sysadmin | 08:28am on 17 December 2023 | | Campbell, Naomi | naomi@example.com | moderator | | | Hunt, Helen | helen@example.com | moderator | 10:09am on 15 December 2023 | - | Jacobi, Derek | derek@example.com | moderator | | + | Jacobi, Derek | derek@example.com | moderator | 14:27pm on 11 December 2023 | And the markup should be valid Scenario: Pagination of the users index diff --git a/features/step_definitions/authentication_steps.rb b/features/step_definitions/authentication_steps.rb index f5af09768..f8c2fcd60 100644 --- a/features/step_definitions/authentication_steps.rb +++ b/features/step_definitions/authentication_steps.rb @@ -32,11 +32,6 @@ step "the admin user is logged in" end -Given /^I am logged in as a moderator with the password "([^\"]*)"$/ do |password| - @user = FactoryBot.create(:moderator_user, :password => password, :password_confirmation => password) - step "the admin user is logged in" -end - Given /^I am logged in as a moderator named "([^\"]*)" with the password "([^\"]*)"$/ do |name, password| first_name, last_name = name.split @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, :password => password, :password_confirmation => password) @@ -56,9 +51,9 @@ Given /^the admin user is logged in$/ do visit admin_login_url + click_button("Login with developer strategy") fill_in("Email", :with => @user.email) - fill_in("Password", :with => "Letmein1!") - click_button("Sign in") + click_button("Sign In") end Given /^the admin user is logged out$/ do diff --git a/features/step_definitions/pickle_steps.rb b/features/step_definitions/pickle_steps.rb index 9a804e693..f8ee725a9 100644 --- a/features/step_definitions/pickle_steps.rb +++ b/features/step_definitions/pickle_steps.rb @@ -1,17 +1,5 @@ -Given(/^a sysadmin user exists with first_name: "([^"]*)", last_name: "([^"]*)", email: "([^"]*)", password: "([^"]*)", password_confirmation: "([^"]*)"$/) do |first_name, last_name, email, password, password_confirmation| - @user = FactoryBot.create(:sysadmin_user, first_name: first_name, last_name: last_name, email: email, password: password, password_confirmation: password_confirmation) -end - -Given(/^a moderator user exists with first_name: "([^"]*)", last_name: "([^"]*)", email: "([^"]*)", password: "([^"]*)", password_confirmation: "([^"]*)"$/) do |first_name, last_name, email, password, password_confirmation| - @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, email: email, password: password, password_confirmation: password_confirmation) -end - -Given(/^a moderator user exists with email: "([^"]*)", password: "([^"]*)", password_confirmation: "([^"]*)"$/) do |email, password, password_confirmation| - @user = FactoryBot.create(:moderator_user, email: email, password: password, password_confirmation: password_confirmation) -end - -Given(/^a moderator user exists with email: "([^"]*)", password: "([^"]*)", password_confirmation: "([^"]*)", force_password_reset: true$/) do |email, password, password_confirmation| - @user = FactoryBot.create(:moderator_user, email: email, password: password, password_confirmation: password_confirmation, force_password_reset: true) +Given(/^a sysadmin user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)"$/) do |email, first_name, last_name| + @user = FactoryBot.create(:sysadmin_user, first_name: first_name, last_name: last_name, email: email) end Given(/^a moderator user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)"$/) do |email, first_name, last_name| @@ -22,10 +10,6 @@ @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, email: email, current_sign_in_at: current_sign_in_at) end -Given(/^a moderator user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)", failed_attempts: (\d+)$/) do |email, first_name, last_name, failed_attempts| - @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, email: email, failed_attempts: failed_attempts) -end - Given(/^(\d+) moderator users exist$/) do |number| number.times do |count| FactoryBot.create(:moderator_user) @@ -38,14 +22,6 @@ end end -When(/^a moderator user should exist with email: "([^"]*)", failed_attempts: "([^"]*)"$/) do |email, failed_attempts| - expect(AdminUser.where(email: email, failed_attempts: failed_attempts)).to exist -end - -Given(/^a moderator user exists with email: "([^"]*)", first_name: "([^"]*)", last_name: "([^"]*)", failed_attempts: "([^"]*)"$/) do |email, first_name, last_name, failed_attempts| - @user = FactoryBot.create(:moderator_user, email: email, first_name: first_name, last_name: last_name, failed_attempts: failed_attempts) -end - Then(/^a admin user should not exist with email: "([^"]*)"$/) do |email| expect(AdminUser.where(email: email)).not_to exist end diff --git a/lib/devise/encryptable/encryptors/authlogic_scrypt.rb b/lib/devise/encryptable/encryptors/authlogic_scrypt.rb deleted file mode 100644 index 44507b350..000000000 --- a/lib/devise/encryptable/encryptors/authlogic_scrypt.rb +++ /dev/null @@ -1,29 +0,0 @@ -require 'scrypt' - -module Devise - module Encryptable - module Encryptors - class AuthlogicScrypt - DEFAULTS = { - key_len: 32, - salt_size: 8, - max_time: 0.2, - max_mem: 1024 * 1024, - max_memfrac: 0.5 - } - - def self.digest(password, stretches, salt, pepper) - ::SCrypt::Password.create([password, salt].join, DEFAULTS) - end - - def self.salt(stretches) - SecureRandom.base58(20) - end - - def self.compare(encrypted_password, password, stretches, salt, pepper) - ::SCrypt::Password.new(encrypted_password) == [password, salt].join - end - end - end - end -end diff --git a/lib/tasks/epets.rake b/lib/tasks/epets.rake index 79ae580b6..c0457e1be 100644 --- a/lib/tasks/epets.rake +++ b/lib/tasks/epets.rake @@ -6,7 +6,6 @@ namespace :epets do if AdminUser.find_by(email: 'admin@example.com').nil? admin = AdminUser.new(:first_name => 'Cool', :last_name => 'Admin', :email => 'admin@example.com') admin.role = 'sysadmin' - admin.password = admin.password_confirmation = 'Letmein1!' admin.save! end end diff --git a/spec/factories.rb b/spec/factories.rb index 1b0e2b975..1fe7cc448 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -4,11 +4,8 @@ FactoryBot.define do factory :admin_user do sequence(:email) { |n| "admin#{n}@example.com" } - password { "Letmein1!" } - password_confirmation { "Letmein1!" } sequence(:first_name) { |n| "AdminUser#{n}" } sequence(:last_name) { |n| "AdminUser#{n}" } - force_password_reset { false } end factory :sysadmin_user, :parent => :admin_user do diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb index ebf829773..62c4a9ff4 100644 --- a/spec/models/admin_user_spec.rb +++ b/spec/models/admin_user_spec.rb @@ -4,19 +4,21 @@ describe "schema" do it { is_expected.to have_db_column(:email).of_type(:string).with_options(null: false) } it { is_expected.to have_db_column(:persistence_token).of_type(:string) } - it { is_expected.to have_db_column(:encrypted_password).of_type(:string) } - it { is_expected.to have_db_column(:password_salt).of_type(:string) } it { is_expected.to have_db_column(:sign_in_count).of_type(:integer).with_options(default: 0) } - it { is_expected.to have_db_column(:failed_attempts).of_type(:integer).with_options(default: 0) } it { is_expected.to have_db_column(:current_sign_in_at).of_type(:datetime) } it { is_expected.to have_db_column(:last_sign_in_at).of_type(:datetime) } it { is_expected.to have_db_column(:current_sign_in_ip).of_type(:string) } it { is_expected.to have_db_column(:last_sign_in_ip).of_type(:string) } it { is_expected.to have_db_column(:role).of_type(:string).with_options(limit: 10, null: false) } - it { is_expected.to have_db_column(:force_password_reset).of_type(:boolean).with_options(default: true) } - it { is_expected.to have_db_column(:password_changed_at).of_type(:datetime) } it { is_expected.to have_db_column(:created_at).of_type(:datetime) } it { is_expected.to have_db_column(:updated_at).of_type(:datetime) } + + it { is_expected.not_to have_db_column(:encrypted_password).of_type(:string) } + it { is_expected.not_to have_db_column(:password_salt).of_type(:string) } + it { is_expected.not_to have_db_column(:failed_attempts).of_type(:integer).with_options(default: 0) } + it { is_expected.not_to have_db_column(:force_password_reset).of_type(:boolean).with_options(default: true) } + it { is_expected.not_to have_db_column(:password_changed_at).of_type(:datetime) } + it { is_expected.not_to have_db_column(:locked_at).of_type(:datetime) } end describe "indexes" do @@ -24,19 +26,10 @@ it { is_expected.to have_db_index([:last_name, :first_name]) } end - describe "defaults" do - it "force_password_reset should default to true" do - u = AdminUser.new - expect(u.force_password_reset).to be_truthy - end - end - describe "validations" do - it { is_expected.to validate_presence_of(:password) } it { is_expected.to validate_presence_of(:email) } it { is_expected.to validate_presence_of(:first_name) } it { is_expected.to validate_presence_of(:last_name) } - it { is_expected.to validate_length_of(:password).is_at_least(8) } it { is_expected.to allow_value("oliver@opsb.co.uk").for(:email)} it { is_expected.not_to allow_value("jimbo").for(:email) } @@ -45,20 +38,6 @@ is_expected.to validate_uniqueness_of(:email).case_insensitive end - it "should only allow passwords with a digit, lower and upper case alpha and a special char" do - ['Letmein1!', 'Letmein1_', '1Ab*aaaa'].each do |email| - u = FactoryBot.build(:moderator_user, :password => email, :password_confirmation => email) - expect(u).to be_valid - end - end - - it "should not allow passwords without a digit, lower and upper case alpha and a special char" do - ['Letmein1', 'hell$0123', '^%ttttFFFFF', 'KJDL_3444'].each do |email| - u = FactoryBot.build(:moderator_user, :password => email, :password_confirmation => email) - expect(u).not_to be_valid - end - end - it "should allow sysadmin role" do u = FactoryBot.build(:admin_user, :role => 'sysadmin') expect(u).to be_valid @@ -95,177 +74,6 @@ end describe "instance methods" do - describe "#update_password" do - let!(:user) do - FactoryBot.create( - :sysadmin_user, - password: "Testing!23", - password_confirmation: "Testing!23", - password_changed_at: nil, - force_password_reset: true - ) - end - - let(:params) do - { - current_password: current_password, password: password, - password_confirmation: password_confirmation - } - end - - let(:current_password) { "Testing!23" } - let(:password) { "NewP4ssword!" } - let(:password_confirmation) { "NewP4ssword!" } - - context "when the new password is valid" do - it "returns true" do - expect(user.update_password(params)).to be_truthy - end - - it "changes the encrypted_password field" do - expect { - user.update_password(params) - }.to change { - user.reload.encrypted_password - } - end - - it "sets the timestamp for when the password was changed" do - expect { - user.update_password(params) - }.to change { - user.reload.password_changed_at - }.from(nil).to(be_within(2.seconds).of(Time.current)) - end - - it "clears the force password reset flag" do - expect { - user.update_password(params) - }.to change { - user.reload.force_password_reset - }.from(true).to(false) - end - end - - context "when the current password is missing" do - let(:current_password) { "" } - - it "returns false" do - expect(user.update_password(params)).to be_falsey - end - - it "adds an error" do - user.update_password(params) - expect(user.errors[:current_password]).to include("Current password can’t be blank") - end - - it "doesn't clear the force password reset flag" do - user.update_password(params) - expect(user.force_password_reset).to be true - end - - it "doesn't set the password_changed_at timestamp" do - user.update_password(params) - expect(user.password_changed_at).to be_nil - end - end - - context "when the current password is incorrect" do - let(:current_password) { "L3tme!n" } - - it "returns false" do - expect(user.update_password(params)).to be_falsey - end - - it "adds an error" do - user.update_password(params) - expect(user.errors[:current_password]).to include("Current password is incorrect") - end - - it "doesn't clear the force password reset flag" do - user.update_password(params) - expect(user.force_password_reset).to be true - end - - it "doesn't set the password_changed_at timestamp" do - user.update_password(params) - expect(user.password_changed_at).to be_nil - end - end - - context "when the new password is the same as the old password" do - let(:password) { current_password } - let(:password_confirmation) { current_password } - - it "returns false" do - expect(user.update_password(params)).to be_falsey - end - - it "adds an error" do - user.update_password(params) - expect(user.errors[:password]).to include("Password is the same as the current password") - end - - it "doesn't clear the force password reset flag" do - user.update_password(params) - expect(user.force_password_reset).to be true - end - - it "doesn't set the password_changed_at timestamp" do - user.update_password(params) - expect(user.password_changed_at).to be_nil - end - end - - context "when the new password is invalid" do - let(:password) { "password" } - let(:password_confirmation) { "password" } - - it "returns false" do - expect(user.update_password(params)).to be_falsey - end - - it "adds an error" do - user.update_password(params) - expect(user.errors[:password]).to include("Password must contain at least one digit, a lower and upper case letter and a special character") - end - - it "doesn't clear the force password reset flag" do - user.update_password(params) - expect(user.force_password_reset).to be true - end - - it "doesn't set the password_changed_at timestamp" do - user.update_password(params) - expect(user.password_changed_at).to be_nil - end - end - - context "when the new password doesn't match the confirmation" do - let(:password) { "L3tme!n1" } - let(:password_confirmation) { "L3tme!n2" } - - it "returns false" do - expect(user.update_password(params)).to be_falsey - end - - it "adds an error" do - user.update_password(params) - expect(user.errors[:password_confirmation]).to include("Password confirmation doesn’t match password") - end - - it "doesn't clear the force password reset flag" do - user.update_password(params) - expect(user.force_password_reset).to be true - end - - it "doesn't set the password_changed_at timestamp" do - user.update_password(params) - expect(user.password_changed_at).to be_nil - end - end - end - describe "#destroy" do context "when there is no current user and there is more than one" do let!(:user_1) { FactoryBot.create(:sysadmin_user) } @@ -337,28 +145,6 @@ end end - describe "#has_to_change_password?" do - it "should be true when force_reset_password is true" do - user = FactoryBot.create(:moderator_user, :force_password_reset => true) - expect(user.has_to_change_password?).to be_truthy - end - - it "should be false when force_reset_password is false" do - user = FactoryBot.create(:moderator_user, :force_password_reset => false) - expect(user.has_to_change_password?).to be_falsey - end - - it "should be true when password was last changed over 9 months ago" do - user = FactoryBot.create(:moderator_user, :force_password_reset => false, :password_changed_at => 9.months.ago - 1.minute) - expect(user.has_to_change_password?).to be_truthy - end - - it "should be false when password was last changed less than 9 months ago" do - user = FactoryBot.create(:moderator_user, :force_password_reset => false, :password_changed_at => 9.months.ago + 1.minute) - expect(user.has_to_change_password?).to be_falsey - end - end - describe "#can_take_petitions_down?" do it "is true if the user is a sysadmin" do user = FactoryBot.create(:admin_user, :role => 'sysadmin') @@ -371,41 +157,6 @@ end end - describe "#account_disabled" do - it "should return true when user has tried to login 5 times unsuccessfully" do - user = FactoryBot.create(:moderator_user) - user.failed_attempts = 5 - expect(user.account_disabled).to be_truthy - end - - it "should return true when user has tried to login 6 times unsuccessfully" do - user = FactoryBot.create(:moderator_user) - user.failed_attempts = 6 - expect(user.account_disabled).to be_truthy - end - - it "should return false when user has tried to login 4 times unsuccessfully" do - user = FactoryBot.create(:moderator_user) - user.failed_attempts = 4 - expect(user.account_disabled).to be_falsey - end - end - - describe "#account_disabled=" do - it "should set the failed login count to 5 when true" do - u = FactoryBot.create(:moderator_user) - u.account_disabled = true - expect(u.failed_attempts).to eq(5) - end - - it "should set the failed login count to 0 when false" do - u = FactoryBot.create(:moderator_user) - u.failed_attempts = 5 - u.account_disabled = false - expect(u.failed_attempts).to eq(0) - end - end - describe "#elapsed_time" do it "returns the number of seconds since the last request" do user = FactoryBot.build(:admin_user) diff --git a/spec/requests/admin_user_persistence_token_spec.rb b/spec/requests/admin_user_persistence_token_spec.rb index 673fbc5ca..8c3679133 100644 --- a/spec/requests/admin_user_persistence_token_spec.rb +++ b/spec/requests/admin_user_persistence_token_spec.rb @@ -5,14 +5,12 @@ { first_name: "System", last_name: "Administrator", - email: "admin@petition.parliament.uk", - password: "L3tme1n!", - password_confirmation: "L3tme1n!" + email: "admin@petition.parliament.uk" } end let(:login_params) do - { email: "admin@petition.parliament.uk", password: "L3tme1n!" } + { email: "admin@petition.parliament.uk" } end before do @@ -30,13 +28,13 @@ def new_browser context "when a new session is created" do it "logs out existing sessions" do s1 = new_browser - s1.post "/admin/login", params: { user: login_params } + s1.post "/admin/auth/developer/callback", params: login_params expect(s1.response.status).to eq(302) expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin") s2 = new_browser - s2.post "/admin/login", params: { user: login_params } + s2.post "/admin/auth/developer/callback", params: login_params expect(s2.response.status).to eq(302) expect(s2.response.location).to eq("https://moderate.petition.parliament.uk/admin") @@ -51,7 +49,7 @@ def new_browser context "when a session is destroyed" do it "resets the persistence token" do s1 = new_browser - s1.post "/admin/login", params: { user: login_params } + s1.post "/admin/auth/developer/callback", params: login_params expect(s1.response.status).to eq(302) expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin") @@ -81,7 +79,7 @@ def new_browser Site.instance.update(login_timeout: 600) travel_to 5.minutes.ago do - post "/admin/login", params: { user: login_params } + post "/admin/auth/developer/callback", params: login_params expect(response).to redirect_to("/admin") end diff --git a/spec/requests/bad_requests_spec.rb b/spec/requests/bad_requests_spec.rb index 67ec515d7..df982a87e 100644 --- a/spec/requests/bad_requests_spec.rb +++ b/spec/requests/bad_requests_spec.rb @@ -26,9 +26,7 @@ { first_name: "System", last_name: "Administrator", - email: "admin@petition.parliament.uk", - password: "L3tme1n!", - password_confirmation: "L3tme1n!" + email: "admin@petition.parliament.uk" } end diff --git a/spec/requests/csrf_token_spec.rb b/spec/requests/csrf_token_spec.rb index 7402162d9..b2b3e308d 100644 --- a/spec/requests/csrf_token_spec.rb +++ b/spec/requests/csrf_token_spec.rb @@ -5,14 +5,12 @@ { first_name: "System", last_name: "Administrator", - email: "admin@petition.parliament.uk", - password: "L3tme1n!", - password_confirmation: "L3tme1n!" + email: "admin@petition.parliament.uk" } end let(:login_params) do - { email: "admin@petition.parliament.uk", password: "L3tme1n!" } + { email: "admin@petition.parliament.uk" } end let(:encrypted_csrf_token) do @@ -36,7 +34,7 @@ context "when a new session is created" do it "resets the csrf token" do expect { - post "/admin/login", params: { user: login_params, authenticity_token: authenticity_token } + post "/admin/auth/developer/callback", params: login_params }.to change { session[:_csrf_token] }.from(be_present).to(be_nil) @@ -45,7 +43,7 @@ context "when a session is destroyed" do before do - post "/admin/login", params: { user: login_params, authenticity_token: authenticity_token } + post "/admin/auth/developer/callback", params: login_params follow_redirect! end @@ -63,7 +61,7 @@ Site.instance.update(login_timeout: 600) travel_to 5.minutes.ago do - post "/admin/login", params: { user: login_params, authenticity_token: authenticity_token } + post "/admin/auth/developer/callback", params: login_params expect(response).to redirect_to("/admin") end diff --git a/spec/requests/login_timeout_spec.rb b/spec/requests/login_timeout_spec.rb index af3a3f1a7..8973298f9 100644 --- a/spec/requests/login_timeout_spec.rb +++ b/spec/requests/login_timeout_spec.rb @@ -5,14 +5,12 @@ { first_name: "System", last_name: "Administrator", - email: "admin@petition.parliament.uk", - password: "L3tme1n!", - password_confirmation: "L3tme1n!" + email: "admin@petition.parliament.uk" } end let(:login_params) do - { email: "admin@petition.parliament.uk", password: "L3tme1n!" } + { email: "admin@petition.parliament.uk" } end let!(:user) { FactoryBot.create(:sysadmin_user, user_attributes) } @@ -26,7 +24,7 @@ Site.instance.update(login_timeout: 600) travel_to 2.minutes.ago do - post "/admin/login", params: { user: login_params } + post "/admin/auth/developer/callback", params: login_params expect(response).to redirect_to("/admin") end From 1a0293e52d97cbbfecc6d86403277168b202c1eb Mon Sep 17 00:00:00 2001 From: Andrew White Date: Wed, 20 Dec 2023 09:33:04 +0000 Subject: [PATCH 10/10] Replace developer strategy with SAML --- Gemfile | 1 + Gemfile.lock | 7 ++ README.md | 6 -- .../admin/omniauth_callbacks_controller.rb | 32 ++++-- app/controllers/admin/sessions_controller.rb | 22 ++++ app/models/admin_user.rb | 24 ++++- app/models/identity_provider.rb | 100 ++++++++++++++++++ app/models/site.rb | 8 ++ app/views/admin/sessions/new.html.erb | 9 +- config/initializers/devise.rb | 4 - config/initializers/omniauth.rb | 11 +- config/locales/admin.en-GB.yml | 1 + config/routes.rb | 15 ++- config/sso.yml | 13 +++ features/admin/admin_access.feature | 56 +++++++--- features/admin/admin_users.feature | 4 +- features/admin/anonymize_petitions.feature | 2 +- .../step_definitions/authentication_steps.rb | 47 +++++--- features/support/hooks.rb | 13 +++ lib/package_builder/scripts/after_install.sh | 1 + spec/factories.rb | 78 ++++++++++++++ .../admin_user_persistence_token_spec.rb | 65 ++++++++++-- spec/requests/csrf_token_spec.rb | 36 +++++-- spec/requests/login_timeout_spec.rb | 25 ++++- spec/support/omniauth.rb | 16 +++ 25 files changed, 514 insertions(+), 82 deletions(-) create mode 100644 app/models/identity_provider.rb create mode 100644 config/sso.yml create mode 100644 spec/support/omniauth.rb diff --git a/Gemfile b/Gemfile index 6d348bca0..a8f26cd6f 100644 --- a/Gemfile +++ b/Gemfile @@ -36,6 +36,7 @@ gem 'redcarpet' gem 'scrypt' gem 'omniauth' gem 'omniauth-rails_csrf_protection' +gem 'omniauth-saml' gem 'aws-sdk-codedeploy' gem 'aws-sdk-cloudwatchlogs' diff --git a/Gemfile.lock b/Gemfile.lock index 5d6531bb2..6091084b1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -244,6 +244,9 @@ GEM omniauth-rails_csrf_protection (1.0.1) actionpack (>= 4.2) omniauth (~> 2.0) + omniauth-saml (2.1.0) + omniauth (~> 2.0) + ruby-saml (~> 1.12) orm_adapter (0.5.0) pg (1.2.3) pry (0.13.1) @@ -319,6 +322,9 @@ GEM rspec-mocks (~> 3.10) rspec-support (~> 3.10) rspec-support (3.10.2) + ruby-saml (1.16.0) + nokogiri (>= 1.13.10) + rexml ruby-vips (2.1.4) ffi (~> 1.12) rubyzip (2.3.2) @@ -422,6 +428,7 @@ DEPENDENCIES nokogiri omniauth omniauth-rails_csrf_protection + omniauth-saml pg pry puma (< 6) diff --git a/README.md b/README.md index 34c3f5efc..acc8392d0 100644 --- a/README.md +++ b/README.md @@ -12,12 +12,6 @@ We recommend using [Docker Desktop][2] to get setup quickly. If you'd prefer not docker compose run --rm web rake db:setup ``` -### Create an admin user - -``` -docker compose run --rm web rake epets:add_sysadmin_user -``` - ### Load the country list ``` diff --git a/app/controllers/admin/omniauth_callbacks_controller.rb b/app/controllers/admin/omniauth_callbacks_controller.rb index 95ac35bbd..78a4fb705 100644 --- a/app/controllers/admin/omniauth_callbacks_controller.rb +++ b/app/controllers/admin/omniauth_callbacks_controller.rb @@ -1,25 +1,45 @@ class Admin::OmniauthCallbacksController < Devise::OmniauthCallbacksController skip_before_action :require_admin - skip_before_action :verify_authenticity_token, only: %i[developer] + skip_before_action :verify_authenticity_token, only: %i[saml] - def developer - @user = AdminUser.find_by(email: omniauth_hash["uid"]) + rescue_from ActiveRecord::RecordNotFound do + redirect_to admin_login_url, alert: :login_failed + end + + def saml + @user = AdminUser.find_or_create_from!(provider, auth_data) if @user.present? - sign_in_and_redirect @user, event: :authentication - set_flash_message(:notice, :signed_in) if is_navigational_format? + sign_in @user, event: :authentication + + set_flash_message(:notice, :signed_in) + set_refresh_header + + render "admin/admin/index" else redirect_to admin_login_url, alert: :invalid_login end end + def failure + redirect_to admin_login_url, alert: :login_failed + end + private def after_omniauth_failure_path_for(scope) admin_login_url end - def omniauth_hash + def auth_data request.env["omniauth.auth"] end + + def provider + IdentityProvider.find_by!(name: auth_data.provider) + end + + def set_refresh_header + headers['Refresh'] = "0; url=#{admin_root_url}" + end end diff --git a/app/controllers/admin/sessions_controller.rb b/app/controllers/admin/sessions_controller.rb index 31284179c..1df24f9e2 100644 --- a/app/controllers/admin/sessions_controller.rb +++ b/app/controllers/admin/sessions_controller.rb @@ -4,6 +4,14 @@ class Admin::SessionsController < Devise::SessionsController helper_method :last_request_at + def create + if provider? + redirect_to sso_provider_url(provider), status: :temporary_redirect + else + redirect_to admin_login_url, alert: :invalid_login + end + end + def continue respond_to do |format| format.json @@ -18,6 +26,20 @@ def status private + def email_domain + Mail::Address.new(sign_in_params[:email]).domain + rescue Mail::Field::ParseError + nil + end + + def provider + @provider ||= IdentityProvider.find_by(domain: email_domain) + end + + def provider? + provider.present? + end + def skip_timeout request.env['devise.skip_trackable'] = true end diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index f408fa6ea..e95e38c22 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -7,7 +7,7 @@ class AdminUser < ActiveRecord::Base class CannotDeleteCurrentUser < RuntimeError; end class MustBeAtLeastOneAdminUser < RuntimeError; end - devise :trackable, :timeoutable, :omniauthable, omniauth_providers: %i[developer] + devise :trackable, :timeoutable # TODO: Drop these columns once rollout of SSO has been completed self.ignored_columns = %i[ @@ -27,7 +27,7 @@ class MustBeAtLeastOneAdminUser < RuntimeError; end validates :first_name, :last_name, presence: true validates :email, presence: true, email: true validates :email, uniqueness: { case_sensitive: false } - validates :role, inclusion: { in: ROLES } + validates :role, presence: true, inclusion: { in: ROLES } # = Callbacks = before_save :reset_persistence_token, unless: :persistence_token? @@ -41,6 +41,26 @@ def self.timeout_in Site.login_timeout.seconds end + def self.find_or_create_from!(provider, auth_data) + find_or_create_by!(email: auth_data.fetch(:uid)) do |user| + user.first_name = auth_data.info.fetch(:first_name) + user.last_name = auth_data.info.fetch(:last_name) + groups = Array.wrap(auth_data.info.fetch(:groups)) + + if (groups & provider.sysadmins).any? + user.role = SYSADMIN_ROLE + elsif (groups & provider.moderators).any? + user.role = MODERATOR_ROLE + elsif (groups & provider.reviewers).any? + user.role = REVIEWER_ROLE + end + end + rescue ActiveRecord::RecordNotUnique => e + find_by!(email: auth_data.fetch(:uid)) + rescue ActiveRecord::RecordInvalid => e + Appsignal.send_exception(e) and return nil + end + def reset_persistence_token self.persistence_token = SecureRandom.hex(64) end diff --git a/app/models/identity_provider.rb b/app/models/identity_provider.rb new file mode 100644 index 000000000..67025cfb4 --- /dev/null +++ b/app/models/identity_provider.rb @@ -0,0 +1,100 @@ +class IdentityProvider + class ProviderNotFound < ArgumentError; end + + class << self + delegate :each, to: :providers + + def providers + @providers ||= load_providers + end + + def names + providers.map(&:name) + end + + def find_by(domain:) + providers.detect { |provider| provider.domains.include?(domain) } + end + + def find_by!(name:) + providers.detect { |provider| provider.name.to_s == name } || raise_provider_not_found(name) + end + + private + + def load_providers + Rails.application.config_for(:sso).map { |options| IdentityProvider.new(options) } + end + + def raise_provider_not_found(name) + raise ProviderNotFound, "Couldn't find the provider '#{name}'" + end + end + + attr_reader :name, :attribute_statements + attr_reader :assertion_consumer_service_url, :sp_entity_id + attr_reader :idp_sso_service_url, :idp_cert, :domains + attr_reader :sysadmins, :moderators, :reviewers + + def initialize(options) + @name = options.fetch(:name).to_sym + @attribute_statements = options.fetch(:attributes, default_attributes) + @assertion_consumer_service_url = "#{Site.moderate_url}/admin/auth/#{name}/callback" + @sp_entity_id = "#{Site.moderate_url}/admin/auth/#{name}" + @idp_sso_service_url = options.fetch(:idp_sso_service_url) + @idp_cert = options.fetch(:idp_cert, "") + @domains = options.fetch(:domains) + @sysadmins = options.fetch(:sysadmins, []) + @moderators = options.fetch(:moderators, []) + @reviewers = options.fetch(:reviewers, []) + + unless klass_defined? + strategies.const_set(klass, new_klass) + end + end + + def to_param + name.to_s + end + + def config + { + attribute_statements: attribute_statements, + assertion_consumer_service_url: assertion_consumer_service_url, + sp_entity_id: sp_entity_id, + idp_sso_service_url: idp_sso_service_url, + idp_cert: idp_cert + } + end + + private + + def default_attributes + { + email: ["email"], + first_name: ["first_name"], + last_name: ["last_name"], + groups: ["groups"] + } + end + + def strategies + OmniAuth::Strategies + end + + def parent_klass + OmniAuth::Strategies::SAML + end + + def new_klass + Class.new(parent_klass) + end + + def klass + @klass ||= name.to_s.camelize.to_sym + end + + def klass_defined? + strategies.const_defined?(klass, false) + end +end diff --git a/app/models/site.rb b/app/models/site.rb index 615d400e2..e56e4084b 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -86,6 +86,14 @@ def moderate_host_with_port instance.moderate_host_with_port end + def moderate_url + if table_exists? + instance.moderate_url + else + default_moderate_url + end + end + def constraints_for_moderation if table_exists? instance.constraints_for_moderation diff --git a/app/views/admin/sessions/new.html.erb b/app/views/admin/sessions/new.html.erb index cc337b429..3f80f9479 100644 --- a/app/views/admin/sessions/new.html.erb +++ b/app/views/admin/sessions/new.html.erb @@ -1,8 +1,13 @@

    Sign in

    - <%= form_tag(admin_auth_developer_path, method: :post) do %> - + <%= form_for(@user, as: :user, url: admin_login_path, authenticity_token: form_authenticity_token) do |form| %> +
    + <%= form.label :email, "Email", class: "form-label" %> + <%= form.text_field :email, class: "form-control", type: "email", autofocus: "autofocus" %> +
    + + <%= form.submit "Sign in", class: "button" %> <% end %>
    diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index a3014e056..204e689f1 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -22,10 +22,6 @@ # ==> Navigation configuration config.sign_out_via = :get - # ==> Omniauth configuration - config.omniauth_path_prefix = '/admin/auth' - config.omniauth :developer, fields: %i[email] - # ==> Warden configuration # Reset the token after logging in so that other sessions are logged out Warden::Manager.after_set_user except: :fetch do |user, warden, options| diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index ed8872266..22bf0a143 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -1,3 +1,10 @@ -OmniAuth.configure do |config| - config.logger = Rails.logger +Rails.application.config.middleware.use OmniAuth::Builder do + configure do |config| + config.path_prefix = '/admin/auth' + config.logger = Rails.logger + end + + IdentityProvider.each do |idp| + provider idp.name, idp.config + end end diff --git a/config/locales/admin.en-GB.yml b/config/locales/admin.en-GB.yml index 501f50d66..abbaf34a5 100644 --- a/config/locales/admin.en-GB.yml +++ b/config/locales/admin.en-GB.yml @@ -74,6 +74,7 @@ en-GB: invalidation_started: "Enqueued the invalidation %{summary}" invalidation_updated: "Invalidation updated successfully" invalid_login: "Invalid login details" + login_failed: "There was a problem logging in - please contact support" logged_out: "You have been logged out" moderator_required: "You must be logged in as a moderator or system administrator to view this page" moderation_delay_sent: "An email has been sent to creators that moderation has been delayed" diff --git a/config/routes.rb b/config/routes.rb index 3a7af4515..e50ae9697 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -244,7 +244,6 @@ get '/', action: 'index', as: :stats post '/', action: 'create', as: nil end - end devise_for :users, class_name: 'AdminUser', module: 'admin', skip: %i[sessions] @@ -252,19 +251,25 @@ as :user do controller 'admin/sessions' do get '/admin/login', action: 'new' + post '/admin/login', action: 'create', as: nil get '/admin/logout', action: 'destroy' get '/admin/continue', action: 'continue' get '/admin/status', action: 'status' end + + controller 'admin/omniauth_callbacks' do + get '/admin/auth/failure', action: 'failure' + + scope '/admin/auth/:provider', via: %i[get post] do + match '/', action: 'passthru', as: :sso_provider + match '/callback', action: 'saml', as: :sso_provider_callback + end + end end end # Devise needs a `new_user_session_url` helper for its failure app direct(:new_user_session) { route_for(:admin_login) } - # Friendly url helpers for Omniauth - direct(:admin_auth_developer) { route_for(:user_developer_omniauth_authorize) } - direct(:admin_auth_developer_callback) { route_for(:user_developer_omniauth_callback) } - get 'ping', to: 'ping#ping' end diff --git a/config/sso.yml b/config/sso.yml new file mode 100644 index 000000000..e774e8c3e --- /dev/null +++ b/config/sso.yml @@ -0,0 +1,13 @@ +development: [] + +test: + - name: "example" + idp_sso_service_url: "http://localhost:3000/sso" + domains: + - "example.com" + sysadmins: + - "sysadmins" + moderators: + - "moderators" + reviewers: + - "reviewers" diff --git a/features/admin/admin_access.feature b/features/admin/admin_access.feature index 7bbafbb2a..1c44c96c0 100644 --- a/features/admin/admin_access.feature +++ b/features/admin/admin_access.feature @@ -15,12 +15,12 @@ Feature: Restricted access to the admin console And I should be connected to the server via an ssl connection And the markup should be valid + @javascript Scenario: Login and logout to the admin console as a sysadmin - Given a sysadmin user exists with email: "admin@example.com", first_name: "John", last_name: "Admin" + Given a sysadmin SSO user exists When I go to the admin login page - And I press "Login with developer strategy" - And I fill in "Email" with "admin@example.com" - And I press "Sign In" + And I fill in "Email" with "sysadmin@example.com" + And I press "Sign in" Then I should be on the admin home page And I should be connected to the server via an ssl connection And the markup should be valid @@ -29,22 +29,54 @@ Feature: Restricted access to the admin console And I follow "Logout" And I should be on the admin login page - Scenario: Login and logout to the admin console as a moderator user - Given a moderator user exists with email: "admin@example.com", first_name: "John", last_name: "Moderator" + @javascript + Scenario: Login and logout to the admin console as a moderator + Given a moderator SSO user exists When I go to the admin login page - And I press "Login with developer strategy" - And I fill in "Email" with "admin@example.com" - And I press "Sign In" + And I fill in "Email" with "moderator@example.com" + And I press "Sign in" Then I should be on the admin home page + And I should be connected to the server via an ssl connection + And the markup should be valid + And I should not see "Users" And I should see "John Moderator" + And I follow "Logout" + And I should be on the admin login page + + @javascript + Scenario: Login and logout to the admin console as a reviewer + Given a reviewer SSO user exists + When I go to the admin login page + And I fill in "Email" with "reviewer@example.com" + And I press "Sign in" + Then I should be on the admin home page + And I should be connected to the server via an ssl connection + And the markup should be valid And I should not see "Users" + And I should see "John Reviewer" And I follow "Logout" And I should be on the admin login page + Scenario: Invalid domain + When I go to the admin login page + And I fill in "Email" with "admin@example.org" + And I press "Sign in" + Then I should see "Invalid login details" + And should not see "Logout" + Scenario: Invalid login - Given I go to the admin login page - And I press "Login with developer strategy" + Given an invalid SSO login + When I go to the admin login page And I fill in "Email" with "admin@example.com" - And I press "Sign In" + And I press "Sign in" + Then I should see "There was a problem logging in - please contact support" + And should not see "Logout" + + Scenario: Valid login but no role + Given a valid SSO login with no roles + When I go to the admin login page + And I fill in "Email" with "norole@example.com" + And I press "Sign in" Then I should see "Invalid login details" And should not see "Logout" + diff --git a/features/admin/admin_users.feature b/features/admin/admin_users.feature index 4f9ce4b22..de406a8b8 100644 --- a/features/admin/admin_users.feature +++ b/features/admin/admin_users.feature @@ -5,7 +5,7 @@ Feature: Admin users index and crud Background: Given the time is "Sun, 17 Dec 2023 08:28:41 +0000" - And I am logged in as a sysadmin with the email "muddy@fox.com", first_name "Sys", last_name "Admin" + And I am logged in as a sysadmin with the email "muddy@example.com", first_name "Sys", last_name "Admin" And a moderator user exists with email: "naomi@example.com", first_name: "Naomi", last_name: "Campbell" Scenario: Accessing the admin users index @@ -20,7 +20,7 @@ Feature: Admin users index and crud When I go to the admin users index page Then I should see the following admin user table: | Name | Email | Role | Last login | - | Admin, Sys | muddy@fox.com | sysadmin | 08:28am on 17 December 2023 | + | Admin, Sys | muddy@example.com | sysadmin | 08:28am on 17 December 2023 | | Campbell, Naomi | naomi@example.com | moderator | | | Hunt, Helen | helen@example.com | moderator | 10:09am on 15 December 2023 | | Jacobi, Derek | derek@example.com | moderator | 14:27pm on 11 December 2023 | diff --git a/features/admin/anonymize_petitions.feature b/features/admin/anonymize_petitions.feature index 46fc4ad31..5457e8f16 100644 --- a/features/admin/anonymize_petitions.feature +++ b/features/admin/anonymize_petitions.feature @@ -4,7 +4,7 @@ Feature: An admin anonymizes petitions I want to anonymize all petitions 6 months after parliament closes in accordance with our privacy policy Background: - Given I am logged in as a sysadmin with the email "muddy@fox.com", first_name "Sys", last_name "Admin" + Given I am logged in as a sysadmin with the email "muddy@example.com", first_name "Sys", last_name "Admin" @javascript Scenario: Admin anonymizes petitions 6 months after parliament closes diff --git a/features/step_definitions/authentication_steps.rb b/features/step_definitions/authentication_steps.rb index f8c2fcd60..fc0cf7299 100644 --- a/features/step_definitions/authentication_steps.rb +++ b/features/step_definitions/authentication_steps.rb @@ -1,15 +1,15 @@ Given /^I am logged in as a sysadmin$/ do - @user = FactoryBot.create(:sysadmin_user) + @user = FactoryBot.build(:sysadmin_sso_user) step "the admin user is logged in" end Given /^I am logged in as a moderator$/ do - @user = FactoryBot.create(:moderator_user) + @user = FactoryBot.build(:moderator_sso_user) step "the admin user is logged in" end Given(/^I log out and login back in as a sysadmin$/) do - @user = FactoryBot.create(:sysadmin_user) + @user = FactoryBot.build(:sysadmin_sso_user) steps %q[ the admin user is logged out @@ -18,7 +18,7 @@ end Given(/^I log out and login back in as a moderator$/) do - @user = FactoryBot.create(:moderator_user) + @user = FactoryBot.build(:moderator_sso_user) steps %q[ the admin user is logged out @@ -28,34 +28,49 @@ Given /^I am logged in as a moderator named "([^\"]*)"$/ do |name| first_name, last_name = name.split - @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name) - step "the admin user is logged in" -end - -Given /^I am logged in as a moderator named "([^\"]*)" with the password "([^\"]*)"$/ do |name, password| - first_name, last_name = name.split - @user = FactoryBot.create(:moderator_user, first_name: first_name, last_name: last_name, :password => password, :password_confirmation => password) + @user = FactoryBot.build(:moderator_sso_user, first_name: first_name, last_name: last_name) step "the admin user is logged in" end Given /^I am logged in as a sysadmin named "([^\"]*)"$/ do |name| first_name, last_name = name.split - @user = FactoryBot.create(:sysadmin_user, first_name: first_name, last_name: last_name) + @user = FactoryBot.build(:sysadmin_sso_user, first_name: first_name, last_name: last_name) step "the admin user is logged in" end Given /^I am logged in as a sysadmin with the email "([^\"]*)", first_name "([^\"]*)", last_name "([^\"]*)"$/ do |email, first_name, last_name| - @user = FactoryBot.create(:sysadmin_user, :email => email, :first_name => first_name, :last_name => last_name) + @user = FactoryBot.build(:sysadmin_sso_user, email: email, first_name: first_name, last_name: last_name) step "the admin user is logged in" end Given /^the admin user is logged in$/ do + OmniAuth.config.mock_auth[:example] = @user + visit admin_login_url - click_button("Login with developer strategy") - fill_in("Email", :with => @user.email) - click_button("Sign In") + fill_in("Email", with: @user.uid) + click_button("Sign in") end Given /^the admin user is logged out$/ do visit admin_logout_url end + +Given /^a sysadmin SSO user exists$/ do + OmniAuth.config.mock_auth[:example] = FactoryBot.build(:sysadmin_sso_user) +end + +Given /^a moderator SSO user exists$/ do + OmniAuth.config.mock_auth[:example] = FactoryBot.build(:moderator_sso_user) +end + +Given /^a reviewer SSO user exists$/ do + OmniAuth.config.mock_auth[:example] = FactoryBot.build(:reviewer_sso_user) +end + +Given(/^a valid SSO login with no roles$/) do + OmniAuth.config.mock_auth[:example] = FactoryBot.build(:norole_sso_user) +end + +Given /^an invalid SSO login$/ do + OmniAuth.config.mock_auth[:example] = :invalid_credentials +end diff --git a/features/support/hooks.rb b/features/support/hooks.rb index 103d1aadf..1d3405adc 100644 --- a/features/support/hooks.rb +++ b/features/support/hooks.rb @@ -83,3 +83,16 @@ Before do Rails.application.env_config['action_dispatch.show_detailed_exceptions'] = false end + +Before do + OmniAuth.config.test_mode = true + + OmniAuth.config.on_failure = Proc.new { |env| + OmniAuth::FailureEndpoint.new(env).redirect_to_failure + } +end + +After do + OmniAuth.config.mock_auth[:example] = nil + OmniAuth.config.test_mode = false +end diff --git a/lib/package_builder/scripts/after_install.sh b/lib/package_builder/scripts/after_install.sh index edf20d66e..2e3d18ebc 100644 --- a/lib/package_builder/scripts/after_install.sh +++ b/lib/package_builder/scripts/after_install.sh @@ -5,6 +5,7 @@ set -o pipefail chown -R deploy:deploy /home/deploy/epetitions/releases/<%= release %> su - deploy <<'EOF' +ln -nfs /home/deploy/epetitions/shared/config/sso.yml /home/deploy/epetitions/releases/<%= release %>/config/sso.yml ln -nfs /home/deploy/epetitions/shared/tmp /home/deploy/epetitions/releases/<%= release %>/tmp ln -nfs /home/deploy/epetitions/shared/log /home/deploy/epetitions/releases/<%= release %>/log ln -nfs /home/deploy/epetitions/shared/bundle /home/deploy/epetitions/releases/<%= release %>/vendor/bundle diff --git a/spec/factories.rb b/spec/factories.rb index 1fe7cc448..7ba8c74db 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -982,4 +982,82 @@ ) end end + + factory :sso_user, class: Hash do + transient do + email { "admin@example.com" } + first_name { "Sys" } + last_name { "Admin" } + groups { %w[sysadmins] } + end + + provider { "example" } + uid { email } + + info do + { + name: "#{first_name} #{last_name}", + email: email, + first_name: first_name, + last_name: last_name, + groups: groups + } + end + + credentials do + {} + end + + extra do + { + raw_info: { + name: "#{first_name} #{last_name}", + email: email, + first_name: first_name, + last_name: last_name, + groups: groups + } + } + end + + skip_create + + initialize_with { OmniAuth::AuthHash.new(attributes) } + end + + factory :sysadmin_sso_user, parent: :sso_user do + transient do + email { "sysadmin@example.com" } + first_name { "John" } + last_name { "Admin" } + groups { %w[sysadmins] } + end + end + + factory :moderator_sso_user, parent: :sso_user do + transient do + email { "moderator@example.com" } + first_name { "John" } + last_name { "Moderator" } + groups { %w[moderators] } + end + end + + factory :reviewer_sso_user, parent: :sso_user do + transient do + email { "reviewer@example.com" } + first_name { "John" } + last_name { "Reviewer" } + groups { %w[reviewers] } + end + end + + factory :norole_sso_user, parent: :sso_user do + transient do + email { "norole@example.com" } + first_name { "No" } + last_name { "Role" } + groups { %w[] } + end + end end diff --git a/spec/requests/admin_user_persistence_token_spec.rb b/spec/requests/admin_user_persistence_token_spec.rb index 8c3679133..5dac1d99e 100644 --- a/spec/requests/admin_user_persistence_token_spec.rb +++ b/spec/requests/admin_user_persistence_token_spec.rb @@ -5,16 +5,17 @@ { first_name: "System", last_name: "Administrator", - email: "admin@petition.parliament.uk" + email: "admin@example.com" } end let(:login_params) do - { email: "admin@petition.parliament.uk" } + { email: "admin@example.com" } end before do - FactoryBot.create(:sysadmin_user, user_attributes) + sso_user = FactoryBot.create(:sysadmin_sso_user, **user_attributes) + OmniAuth.config.mock_auth[:example] = sso_user end def new_browser @@ -28,16 +29,36 @@ def new_browser context "when a new session is created" do it "logs out existing sessions" do s1 = new_browser - s1.post "/admin/auth/developer/callback", params: login_params + s1.post "/admin/login", params: { user: login_params } + + expect(s1.response.status).to eq(307) + expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example") + + s1.follow_redirect!(params: s1.request.POST) expect(s1.response.status).to eq(302) - expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin") + expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example/callback") + + s1.follow_redirect! + + expect(s1.response.status).to eq(200) + expect(s1.response).to have_header("Refresh", "0; url=https://moderate.petition.parliament.uk/admin") s2 = new_browser - s2.post "/admin/auth/developer/callback", params: login_params + s2.post "/admin/login", params: { user: login_params } + + expect(s2.response.status).to eq(307) + expect(s2.response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example") + + s2.follow_redirect!(params: s2.request.POST) expect(s2.response.status).to eq(302) - expect(s2.response.location).to eq("https://moderate.petition.parliament.uk/admin") + expect(s2.response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example/callback") + + s2.follow_redirect! + + expect(s2.response.status).to eq(200) + expect(s2.response).to have_header("Refresh", "0; url=https://moderate.petition.parliament.uk/admin") s1.get("/admin") @@ -49,10 +70,20 @@ def new_browser context "when a session is destroyed" do it "resets the persistence token" do s1 = new_browser - s1.post "/admin/auth/developer/callback", params: login_params + s1.post "/admin/login", params: { user: login_params } + + expect(s1.response.status).to eq(307) + expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example") + + s1.follow_redirect!(params: s1.request.POST) expect(s1.response.status).to eq(302) - expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin") + expect(s1.response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example/callback") + + s1.follow_redirect! + + expect(s1.response.status).to eq(200) + expect(s1.response).to have_header("Refresh", "0; url=https://moderate.petition.parliament.uk/admin") s2 = new_browser s2.cookies["_epets_session"] = s1.cookies["_epets_session"] @@ -79,8 +110,20 @@ def new_browser Site.instance.update(login_timeout: 600) travel_to 5.minutes.ago do - post "/admin/auth/developer/callback", params: login_params - expect(response).to redirect_to("/admin") + post "/admin/login", params: { user: login_params } + + expect(response.status).to eq(307) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example") + + follow_redirect!(params: request.POST) + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example/callback") + + follow_redirect! + + expect(response.status).to eq(200) + expect(response).to have_header("Refresh", "0; url=https://moderate.petition.parliament.uk/admin") end get "/admin" diff --git a/spec/requests/csrf_token_spec.rb b/spec/requests/csrf_token_spec.rb index b2b3e308d..4e94edbab 100644 --- a/spec/requests/csrf_token_spec.rb +++ b/spec/requests/csrf_token_spec.rb @@ -5,12 +5,17 @@ { first_name: "System", last_name: "Administrator", - email: "admin@petition.parliament.uk" + email: "admin@example.com" } end let(:login_params) do - { email: "admin@petition.parliament.uk" } + { email: "admin@example.com" } + end + + before do + sso_user = FactoryBot.create(:sysadmin_sso_user, **user_attributes) + OmniAuth.config.mock_auth[:example] = sso_user end let(:encrypted_csrf_token) do @@ -31,20 +36,36 @@ get "/admin/login" end + def do_login + post "/admin/login", params: { user: login_params, authenticity_token: authenticity_token } + + expect(response.status).to eq(307) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example") + + follow_redirect!(params: request.POST) + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example/callback") + + follow_redirect! + + expect(response.status).to eq(200) + expect(response).to have_header("Refresh", "0; url=https://moderate.petition.parliament.uk/admin") + end + context "when a new session is created" do it "resets the csrf token" do expect { - post "/admin/auth/developer/callback", params: login_params + do_login }.to change { session[:_csrf_token] - }.from(be_present).to(be_nil) + } end end context "when a session is destroyed" do before do - post "/admin/auth/developer/callback", params: login_params - follow_redirect! + do_login end it "resets the csrf token" do @@ -61,8 +82,7 @@ Site.instance.update(login_timeout: 600) travel_to 5.minutes.ago do - post "/admin/auth/developer/callback", params: login_params - expect(response).to redirect_to("/admin") + do_login end get "/admin" diff --git a/spec/requests/login_timeout_spec.rb b/spec/requests/login_timeout_spec.rb index 8973298f9..89ff55fec 100644 --- a/spec/requests/login_timeout_spec.rb +++ b/spec/requests/login_timeout_spec.rb @@ -5,15 +5,18 @@ { first_name: "System", last_name: "Administrator", - email: "admin@petition.parliament.uk" + email: "admin@example.com" } end let(:login_params) do - { email: "admin@petition.parliament.uk" } + { email: "admin@example.com" } end - let!(:user) { FactoryBot.create(:sysadmin_user, user_attributes) } + before do + sso_user = FactoryBot.create(:sysadmin_sso_user, **user_attributes) + OmniAuth.config.mock_auth[:example] = sso_user + end before do host! "moderate.petition.parliament.uk" @@ -24,8 +27,20 @@ Site.instance.update(login_timeout: 600) travel_to 2.minutes.ago do - post "/admin/auth/developer/callback", params: login_params - expect(response).to redirect_to("/admin") + post "/admin/login", params: { user: login_params } + + expect(response.status).to eq(307) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example") + + follow_redirect!(params: request.POST) + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example/callback") + + follow_redirect! + + expect(response.status).to eq(200) + expect(response).to have_header("Refresh", "0; url=https://moderate.petition.parliament.uk/admin") end get "/admin" diff --git a/spec/support/omniauth.rb b/spec/support/omniauth.rb new file mode 100644 index 000000000..841ce48f0 --- /dev/null +++ b/spec/support/omniauth.rb @@ -0,0 +1,16 @@ +RSpec.configure do |config| + config.around(:each, type: :request) do |example| + OmniAuth.config.test_mode = true + existing_failure_proc = OmniAuth.config.on_failure + + OmniAuth.config.on_failure = Proc.new { |env| + OmniAuth::FailureEndpoint.new(env).redirect_to_failure + } + + example.run + ensure + OmniAuth.config.mock_auth[:example] = nil + OmniAuth.config.on_failure = existing_failure_proc + OmniAuth.config.test_mode = false + end +end
    Name Email RoleDisabledLast login
    <%= link_to user.name, edit_admin_admin_user_path(user) %><%= user.name%> <%= mail_to user.email %> <%= user.role %><%= user.account_disabled ? 'Yes' : '' %> - <% if user != current_user %> - <%= cms_delete_link user, :url => admin_admin_user_path(user) %> - <% else %> -   - <% end %> - <%= short_date_time_format(user.current_sign_in_at) %>