Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -407,4 +407,16 @@ def add_sp_cost(token)
def mobile?
BrowserCache.parse(request.user_agent).mobile?
end

def user_is_banned?
return false unless user_signed_in?
BannedUserResolver.new(current_user).banned_for_sp?(issuer: current_sp&.issuer)
end

def handle_banned_user
return unless user_is_banned?
analytics.track_event(Analytics::BANNED_USER_REDIRECT)
sign_out
redirect_to banned_user_url
end
end
5 changes: 5 additions & 0 deletions app/controllers/banned_user_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class BannedUserController < ApplicationController
def show
analytics.track_event(Analytics::BANNED_USER_VISITED)
end
end
1 change: 1 addition & 0 deletions app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class AuthorizationController < ApplicationController
before_action :store_request, only: [:index]
before_action :check_sp_active, only: [:index]
before_action :apply_secure_headers_override, only: [:index]
before_action :handle_banned_user
before_action :confirm_user_is_authenticated_with_fresh_mfa, only: :index
before_action :prompt_for_password_if_ial2_request_and_pii_locked, only: [:index]
before_action :bump_auth_count, only: [:index]
Expand Down
1 change: 1 addition & 0 deletions app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class SamlIdpController < ApplicationController
prepend_before_action :skip_session_expiration, only: :metadata

skip_before_action :verify_authenticity_token
before_action :handle_banned_user
before_action :confirm_user_is_authenticated_with_fresh_mfa, only: :auth
before_action :bump_auth_count, only: [:auth]

Expand Down
6 changes: 5 additions & 1 deletion app/controllers/users/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,11 @@ def request_id
end

def next_url_after_valid_authentication
if pending_account_reset_request.present?
if user_is_banned?
analytics.track_event(Analytics::BANNED_USER_REDIRECT)
sign_out
banned_user_url
elsif pending_account_reset_request.present?
account_reset_pending_url
elsif current_user.accepted_rules_of_use_still_valid?
user_two_factor_authentication_url
Expand Down
3 changes: 3 additions & 0 deletions app/models/sign_in_restriction.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class SignInRestriction < ApplicationRecord
belongs_to :user
end
1 change: 1 addition & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class User < ApplicationRecord
has_many :service_providers,
through: :identities,
source: :service_provider_record
has_many :sign_in_restrictions, dependent: :destroy

attr_accessor :asserted_attributes

Expand Down
2 changes: 2 additions & 0 deletions app/services/analytics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ def browser_attributes
AUTHENTICATION_CONFIRMATION = 'Authentication Confirmation'
AUTHENTICATION_CONFIRMATION_CONTINUE = 'Authentication Confirmation: Continue selected'
AUTHENTICATION_CONFIRMATION_RESET = 'Authentication Confirmation: Reset selected'
BANNED_USER_REDIRECT = 'Banned User redirected'
BANNED_USER_VISITED = 'Banned User visited'
DOC_AUTH = 'Doc Auth' # visited or submitted is appended
DOC_AUTH_ASYNC = 'Doc Auth Async'
DOC_AUTH_WARNING = 'Doc Auth Warning'
Expand Down
11 changes: 11 additions & 0 deletions app/services/banned_user_resolver.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class BannedUserResolver
attr_reader :user

def initialize(user)
@user = user
end

def banned_for_sp?(issuer:)
user.sign_in_restrictions.where(service_provider: [nil, issuer]).any?
end
end
7 changes: 7 additions & 0 deletions app/views/banned_user/show.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<% title t('banned_user.title') %>

<%= image_tag('alert/fail-x.svg', width: 54, alt: '', class: 'display-block margin-bottom-4') %>
<%= render PageHeadingComponent.new.with_content(t('banned_user.title')) %>
<p>
<%= t('banned_user.details') %>
</p>
4 changes: 4 additions & 0 deletions config/locales/banned_user/en.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
en:
banned_user:
details: We are unable to authenticate you at this time.
title: Access is restricted
4 changes: 4 additions & 0 deletions config/locales/banned_user/es.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
es:
banned_user:
details: No podemos autenticarlo en este momento.
title: El acceso está restringido
4 changes: 4 additions & 0 deletions config/locales/banned_user/fr.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
fr:
banned_user:
details: Nous ne sommes pas en mesure de vous authentifier pour le moment.
title: L’accès est restreint
2 changes: 2 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,8 @@

delete '/users' => 'users#destroy', as: :destroy_user

get '/restricted' => 'banned_user#show', as: :banned_user

scope '/verify', as: 'idv' do
get '/' => 'idv#index'
get '/activated' => 'idv#activated'
Expand Down
17 changes: 17 additions & 0 deletions db/primary_migrate/20220129181752_create_sign_in_restrictions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
class CreateSignInRestrictions < ActiveRecord::Migration[6.1]
def change
create_table :sign_in_restrictions do |t|
t.integer :user_id, null: false
t.string :service_provider

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jan 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe for the future we can add a followup for with a reason (internal only, just for record keeping)

Suggested change
t.string :reason

t.timestamps
end

add_index(
:sign_in_restrictions,
[:user_id, :service_provider],
unique: true,
name: :index_sign_in_restrictions_on_user_id_and_service_provider,
)
end
end
10 changes: 9 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2022_01_14_183203) do
ActiveRecord::Schema.define(version: 2022_01_29_181752) do

# These are extensions that must be enabled in order to support this database
enable_extension "pg_stat_statements"
Expand Down Expand Up @@ -534,6 +534,14 @@
t.index ["issuer"], name: "index_service_providers_on_issuer", unique: true
end

create_table "sign_in_restrictions", force: :cascade do |t|
t.integer "user_id", null: false
t.string "service_provider"
t.datetime "created_at", precision: 6, null: false
t.datetime "updated_at", precision: 6, null: false
t.index ["user_id", "service_provider"], name: "index_sign_in_restrictions_on_user_id_and_service_provider", unique: true
end

create_table "sp_costs", force: :cascade do |t|
t.string "issuer", null: false
t.integer "agency_id", null: false
Expand Down
70 changes: 70 additions & 0 deletions spec/features/sign_in/banned_users_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
require 'rails_helper'

feature 'Banning users for an SP' do
include SamlAuthHelper

context 'a user is banned from all SPs' do
it 'does not let the user sign in to any SP' do
user = create(:user, :signed_up)

SignInRestriction.create(user: user)

sign_in_user(user)
expect_user_to_be_banned

visit_idp_from_sp_with_ial1(:saml)
sign_in_user(user)
expect_user_to_be_banned

visit_idp_from_sp_with_ial1(:oidc)
sign_in_user(user)
expect_user_to_be_banned
end
end

context 'a user is banned for a SAML SP' do
it 'bans the user from signing in to the banned SP but allows other sign ins' do
user = create(:user, :signed_up)

SignInRestriction.create(user: user, service_provider: 'http://localhost:3000')

sign_in_live_with_2fa(user)
expect(current_path).to eq(account_path)

visit_idp_from_sp_with_ial1(:saml)
expect_user_to_be_banned

visit_idp_from_sp_with_ial1(:oidc)
sign_in_live_with_2fa(user)
click_agree_and_continue
expect(current_url).to start_with('http://localhost:7654/auth/result')
end
end

context 'a user is banner for an OIDC SP' do
it 'bans the user from signing in to the banned SP but allows other sign ins' do
user = create(:user, :signed_up)

SignInRestriction.create(user: user, service_provider: 'urn:gov:gsa:openidconnect:sp:server')

sign_in_live_with_2fa(user)
expect(current_path).to eq(account_path)

visit_idp_from_sp_with_ial1(:oidc)
expect_user_to_be_banned

visit_idp_from_sp_with_ial1(:saml)
sign_in_live_with_2fa(user)
click_agree_and_continue
expect(current_path).to eq(api_saml_auth2022_path)
end
end

def expect_user_to_be_banned
expect(current_path).to eq(banned_user_path)
expect(page).to have_content(I18n.t('banned_user.title'))

visit account_path
expect(current_path).to eq(new_user_session_path)
end
end
47 changes: 47 additions & 0 deletions spec/services/banned_user_resolver_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
require 'rails_helper'

describe BannedUserResolver do
context 'the user is not banned' do
it 'returns false' do
user = create(:user)
sp = create(:service_provider)

expect(described_class.new(user).banned_for_sp?(issuer: sp.issuer)).to eq(false)
end
end

context 'the user is banned for a single SP' do
it 'returns true for the banned SP' do
user = create(:user)
sp = create(:service_provider)

SignInRestriction.create(user: user, service_provider: sp.issuer)

expect(described_class.new(user).banned_for_sp?(issuer: sp.issuer)).to eq(true)
end

it 'returns false for the not banned SP' do
user = create(:user)
sp = create(:service_provider)
banned_sp = create(:service_provider)

SignInRestriction.create(user: user, service_provider: banned_sp.issuer)

expect(described_class.new(user).banned_for_sp?(issuer: sp.issuer)).to eq(false)
end
end

context 'the user is banned for all SPs' do
it 'returns true for all SPs' do
user = create(:user)
sp1 = create(:service_provider)
sp2 = create(:service_provider)

SignInRestriction.create(user: user, service_provider: nil)

expect(described_class.new(user).banned_for_sp?(issuer: sp1.issuer)).to eq(true)
expect(described_class.new(user).banned_for_sp?(issuer: sp2.issuer)).to eq(true)
expect(described_class.new(user).banned_for_sp?(issuer: nil)).to eq(true)
end
end
end