Merge pull request alphagov#2105 from alphagov/organisation-2sv-logic

Require 2SV for users from organisations where 2SV is mandatory

app/controllers/invitations_controller.rb

@@ -18,14 +18,17 @@ def create
     if (self.resource = User.find_by(email: params[:user][:email]))
       authorize resource
       flash[:alert] = "User already invited. If you want to, you can click 'Resend signup email'."
-      respond_with resource, location: after_invite_path_for(resource)
+      respond_with resource, location: users_path
     else
       # workaround for invitatable not providing a build_invitation which could be authorised before saving
-      user = User.new(resource_params)
-      user.organisation_id = resource_params[:organisation_id]
+      all_params = resource_params
+      all_params[:require_2sv] = new_user_requires_2sv(all_params.symbolize_keys)
+
+      user = User.new(all_params)
+      user.organisation_id = all_params[:organisation_id]
       authorize user
 
-      self.resource = resource_class.invite!(resource_params, current_inviter)
+      self.resource = resource_class.invite!(all_params, current_inviter)
       if resource.errors.empty?
         grant_default_permissions(resource)
         set_flash_message :notice, :send_instructions, email: resource.email
@@ -50,7 +53,11 @@ def resend
 private
 
   def after_invite_path_for(_resource)
-    users_path
+    if new_user_requires_2sv(resource)
+      users_path
+    else
+      require_2sv_user_path(resource)
+    end
   end
 
   # TODO: remove this method when we're on a version of devise_invitable which
@@ -122,4 +129,8 @@ def grant_default_permissions(user)
       user.grant_permission(default_permission)
     end
   end
+
+  def new_user_requires_2sv(params)
+    (params[:organisation_id].present? && Organisation.find(params[:organisation_id]).require_2sv?) || %w[superadmin admin].include?(params[:role])
+  end
 end

app/controllers/users_controller.rb

@@ -123,6 +123,8 @@ def reset_two_step_verification
     redirect_to :root, notice: "Reset 2-step verification for #{@user.email}"
   end
 
+  def require_2sv; end
+
 private
 
   def load_and_authorize_user
@@ -200,14 +202,18 @@ def allow_no_application_access
   end
 
   def user_params
+    if permitted_user_params[:skip_update_user_permissions]
+      permitted_user_params[:supported_permission_ids] = @user.supported_permission_ids
+    end
+
     UserParameterSanitiser.new(
       user_params: permitted_user_params,
       current_user_role: current_user.role.to_sym,
     ).sanitise
   end
 
   def permitted_user_params
-    params.require(:user).permit(:user, :name, :email, :organisation_id, :require_2sv, :role, supported_permission_ids: []).to_h
+    @permitted_user_params ||= params.require(:user).permit(:user, :name, :email, :organisation_id, :require_2sv, :role, :skip_update_user_permissions, supported_permission_ids: []).to_h
   end
 
   def password_params

app/models/user.rb

@@ -40,6 +40,7 @@ class User < ApplicationRecord
   validate :organisation_admin_belongs_to_organisation
   validate :email_is_ascii_only
   validate :exemption_from_2sv_data_is_complete
+  validate :organisation_has_mandatory_2sv, on: :create
 
   has_many :authorisations, class_name: "Doorkeeper::AccessToken", foreign_key: :resource_owner_id
   has_many :application_permissions, class_name: "UserApplicationPermission", inverse_of: :user
@@ -96,6 +97,12 @@ class User < ApplicationRecord
           end
         }
 
+  def require_2sv?
+    return require_2sv unless organisation
+
+    (organisation.require_2sv? && !exempt_from_2sv?) || require_2sv
+  end
+
   def prompt_for_2sv?
     return false if has_2sv?
 
@@ -362,6 +369,10 @@ def exemption_from_2sv_data_is_complete
     errors.add(:reason_for_2sv_exemption, "must be present if exemption expiry date is present") if expiry_date_for_2sv_exemption.present? && reason_for_2sv_exemption.nil?
   end
 
+  def organisation_has_mandatory_2sv
+    errors.add(:require_2sv, "2-step verification is mandatory for all users from this organisation") if organisation && organisation.require_2sv? && !require_2sv
+  end
+
   def fix_apostrophe_in_email
     email.tr!("’", "'") if email.present? && email_changed?
   end

app/policies/user_policy.rb

@@ -41,6 +41,7 @@ def edit_email_or_password?
   alias_method :update_email?, :edit_email_or_password?
   alias_method :update_password?, :edit_email_or_password?
   alias_method :mandate_2sv?, :edit?
+  alias_method :require_2sv?, :edit?
   alias_method :reset_2sv?, :edit?
   alias_method :reset_two_step_verification?, :edit?
 

app/views/users/_form_fields.html.erb

@@ -40,7 +40,7 @@
   <p>This user's role is set to <%= @user.role %>. They are currently exempted from 2sv, meaning that their role cannot be changed as admins are required to have 2sv.</p>
 <% end %>
 
-<% if policy(current_user).mandate_2sv? %>
+<% if policy(current_user).mandate_2sv? && @user.persisted? %>
   <dl>
     <dt>Account security</dt>
     <dd>

app/views/users/require_2sv.html.erb

@@ -0,0 +1,16 @@
+<% content_for :title, "Create new user" %>
+
+<h1>2-step verification settings for new user</h1>
+
+<%= form_for @user, :html => {:class => 'well'} do |f| %>
+  <%= f.hidden_field :skip_update_user_permissions, value: "true" %>
+  <p class="checkbox">
+    <%= f.label :require_2sv do %>
+      <%= f.check_box :require_2sv %> Mandate 2-step verification for this user <%= "(this will remove their exemption)" if @user.exempt_from_2sv? %>
+    <% end %>
+    <br/>
+    User will be prompted to set up 2-step verification again the next time they sign in.
+  </p>
+
+  <%= f.submit "Update user", :class => 'btn btn-success' %>
+<% end %>

config/routes.rb

@@ -42,6 +42,7 @@
       delete :cancel_email_change
       get :event_logs
       patch :reset_two_step_verification
+      get :require_2sv
     end
   end
   resource :user, only: [:show]

db/seeds.rb

@@ -16,11 +16,20 @@
   organisation: gds,
 )
 
-test_organisation = Organisation.create!(
-  name: "Test Organisation",
+test_organisation_without_2sv = Organisation.create!(
+  name: "Test Organisation without mandatory 2SV",
   content_id: SecureRandom.uuid,
   organisation_type: :ministerial_department,
   slug: "test-organisation",
+  require_2sv: false,
+)
+
+test_organisation_with_2sv = Organisation.create!(
+  name: "Test Organisation with mandatory 2SV",
+  content_id: SecureRandom.uuid,
+  organisation_type: :ministerial_department,
+  slug: "test-organisation-with-2sv",
+  require_2sv: true,
 )
 
 User.create!(
@@ -29,7 +38,7 @@
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
   role: :normal,
   confirmed_at: Time.zone.now,
-  organisation: test_organisation,
+  organisation: test_organisation_without_2sv,
 )
 
 # The following user has 2SV enabled by default. Scan the QR code with your authenticator app to generate a code to login.
@@ -65,7 +74,18 @@
   password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
   role: :normal,
   confirmed_at: Time.zone.now,
-  organisation: test_organisation,
+  organisation: test_organisation_without_2sv,
+  require_2sv: true,
+  otp_secret_key: "I5X6Y3VN3CAATYQRBPAZ7KMFLK2RWYJ5",
+)
+
+User.create!(
+  name: "Test User from organisation with mandatory 2SV",
+  email: "[email protected]",
+  password: "6fe552ca-d406-4c54-b7a6-041ed1ade6cd",
+  role: :normal,
+  confirmed_at: Time.zone.now,
+  organisation: test_organisation_with_2sv,
   require_2sv: true,
   otp_secret_key: "I5X6Y3VN3CAATYQRBPAZ7KMFLK2RWYJ5",
 )
@@ -79,3 +99,13 @@
   confirmed_at: Time.zone.now,
   require_2sv: false,
 )
+
+application = Doorkeeper::Application.create!(
+  name: "Test Application 1",
+  redirect_uri: "https://www.gov.uk",
+)
+
+SupportedPermission.create!(
+  name: "Editor",
+  application:,
+)

test/controllers/invitations_controller_test.rb

@@ -3,7 +3,7 @@
 class InvitationsControllerTest < ActionController::TestCase
   setup do
     request.env["devise.mapping"] = Devise.mappings[:user]
-    @user = create(:admin_user)
+    @user = create(:superadmin_user)
     sign_in @user
   end
 
@@ -60,6 +60,42 @@ class InvitationsControllerTest < ActionController::TestCase
       post :create, params: { user: { name: "Testing Org Admins", email: "[email protected]" } }
       assert_redirected_to root_path
     end
+
+    should "save user and render 2SV form when user assigned to organisation that does not require 2SV" do
+      organisation = create(:organisation, require_2sv: false)
+
+      post :create, params: { user: { name: "User Name", email: "[email protected]", organisation_id: organisation.id } }
+
+      assert_redirected_to require_2sv_user_path(User.last)
+      assert_equal "User Name", User.last.name
+    end
+
+    should "save user and not render 2SV form when user assigned to organisation that requires 2SV" do
+      organisation = create(:organisation, require_2sv: true)
+
+      post :create, params: { user: { name: "User Name", email: "[email protected]", organisation_id: organisation.id } }
+
+      assert_redirected_to users_path
+      assert_equal "User Name", User.last.name
+    end
+
+    should "not render 2SV form and saves user when user is a superadmin" do
+      organisation = create(:organisation, require_2sv: false)
+
+      post :create, params: { user: { name: "User Name", email: "[email protected]", organisation_id: organisation.id, role: "superadmin" } }
+
+      assert_redirected_to users_path
+      assert_equal "User Name", User.last.name
+    end
+
+    should "not render 2SV form and saves user when user is an admin" do
+      organisation = create(:organisation, require_2sv: false)
+
+      post :create, params: { user: { name: "User Name", email: "[email protected]", organisation_id: organisation.id, role: "admin" } }
+
+      assert_redirected_to users_path
+      assert_equal "User Name", User.last.name
+    end
   end
 
   context "POST resend" do