From 668079022b3fd8c4dddd9c9c660b13b48e5749e3 Mon Sep 17 00:00:00 2001 From: Steve Laing Date: Mon, 4 Sep 2023 11:06:18 +0100 Subject: [PATCH 1/5] Add jwt gem needed for DSI API calls --- Gemfile | 1 + Gemfile.lock | 1 + 2 files changed, 2 insertions(+) diff --git a/Gemfile b/Gemfile index 65458826..7af130bc 100644 --- a/Gemfile +++ b/Gemfile @@ -21,6 +21,7 @@ gem "govuk_feature_flags", gem "govuk_markdown" gem "hashie" gem "jsbundling-rails" +gem "jwt" gem "logstash-logger" gem "mail-notify" gem "name_of_person" diff --git a/Gemfile.lock b/Gemfile.lock index 3f2835b2..a36b8e3e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -595,6 +595,7 @@ DEPENDENCIES govuk_markdown hashie jsbundling-rails + jwt launchy logstash-logger mail-notify From 48b6cca1ea0b6d39ca5413adb3ba57b4ffb764d5 Mon Sep 17 00:00:00 2001 From: Steve Laing Date: Thu, 7 Sep 2023 16:02:37 +0100 Subject: [PATCH 2/5] Add dsi_user_sessions This provides a way to record which role and organisation a user has authenticated with. --- config/analytics.yml | 9 +++++++++ .../20230907092835_create_dsi_user_sessions.rb | 12 ++++++++++++ db/schema.rb | 11 +++++++++++ 3 files changed, 32 insertions(+) create mode 100644 db/migrate/20230907092835_create_dsi_user_sessions.rb diff --git a/config/analytics.yml b/config/analytics.yml index c101183a..7b7749ae 100644 --- a/config/analytics.yml +++ b/config/analytics.yml @@ -29,6 +29,15 @@ shared: - uid - created_at - updated_at + :dsi_user_sessions: + - id + - dsi_user_id + - role_id + - role_code + - organisation_id + - organisation_name + - created_at + - updated_at :feature_flags_features: - id - name diff --git a/db/migrate/20230907092835_create_dsi_user_sessions.rb b/db/migrate/20230907092835_create_dsi_user_sessions.rb new file mode 100644 index 00000000..77d8a9cc --- /dev/null +++ b/db/migrate/20230907092835_create_dsi_user_sessions.rb @@ -0,0 +1,12 @@ +class CreateDsiUserSessions < ActiveRecord::Migration[7.0] + def change + create_table :dsi_user_sessions do |t| + t.belongs_to :dsi_user + t.string :role_id + t.string :role_code + t.string :organisation_id + t.string :organisation_name + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 616dbb69..cd1ed0f3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -90,6 +90,17 @@ create_table "data_migrations", primary_key: "version", id: :string, force: :cascade do |t| end + create_table "dsi_user_sessions", force: :cascade do |t| + t.bigint "dsi_user_id" + t.string "role_id" + t.string "role_code" + t.string "organisation_id" + t.string "organisation_name" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["dsi_user_id"], name: "index_dsi_user_sessions_on_dsi_user_id" + end + create_table "dsi_users", force: :cascade do |t| t.string "email", limit: 510, null: false t.string "first_name", limit: 510 From f5a352f16d2b2a14462ab2197070827c0ce3a50f Mon Sep 17 00:00:00 2001 From: Steve Laing Date: Thu, 7 Sep 2023 16:04:16 +0100 Subject: [PATCH 3/5] Add a service to call the DSI API for user roles This DSI API endpoint will respond with roles belonging to the user. --- .env.development | 4 ++ .env.test | 4 ++ app/lib/dfe_sign_in_api/client.rb | 35 +++++++++++++++++ .../get_user_access_to_service.rb | 34 ++++++++++++++++ .../get_user_access_to_service_spec.rb | 39 +++++++++++++++++++ 5 files changed, 116 insertions(+) create mode 100644 app/lib/dfe_sign_in_api/client.rb create mode 100644 app/lib/dfe_sign_in_api/get_user_access_to_service.rb create mode 100644 spec/lib/dfe_sign_in_api/get_user_access_to_service_spec.rb diff --git a/.env.development b/.env.development index 2fdb4a96..83fb9c2b 100644 --- a/.env.development +++ b/.env.development @@ -4,6 +4,10 @@ DFE_SIGN_IN_CLIENT_ID=checkrecordteacher DFE_SIGN_IN_REDIRECT_URL=http://check.localhost:3000/check-records/auth/dfe/callback DFE_SIGN_IN_SECRET=override-locally DFE_SIGN_IN_ISSUER=https://dev-oidc.signin.education.gov.uk +DFE_SIGN_IN_API_BASE_URL=https://dev-api.signin.education.gov.uk +DFE_SIGN_IN_API_SECRET=override-locally +DFE_SIGN_IN_API_AUDIENCE=signin.education.gov.uk +DFE_SIGN_IN_API_ROLE_CODES=override-locally GOVUK_NOTIFY_API_KEY=override-locally HOSTING_DOMAIN=http://localhost:3000 HOSTING_ENVIRONMENT_NAME=local diff --git a/.env.test b/.env.test index 74973a77..43ccc848 100644 --- a/.env.test +++ b/.env.test @@ -3,6 +3,10 @@ DFE_SIGN_IN_CLIENT_ID=checkrecordteacher DFE_SIGN_IN_ISSUER=test DFE_SIGN_IN_REDIRECT_URL=test DFE_SIGN_IN_SECRET=override-locally +DFE_SIGN_IN_API_BASE_URL=https://dev-api.signin.education.gov.uk +DFE_SIGN_IN_API_SECRET=override-locally +DFE_SIGN_IN_API_AUDIENCE=signin.education.gov.uk +DFE_SIGN_IN_API_ROLE_CODES=override-locally GOVUK_NOTIFY_API_KEY=override-locally HOSTING_DOMAIN=http://qualifications.localhost HOSTING_ENVIRONMENT_NAME=local diff --git a/app/lib/dfe_sign_in_api/client.rb b/app/lib/dfe_sign_in_api/client.rb new file mode 100644 index 00000000..0c8332a8 --- /dev/null +++ b/app/lib/dfe_sign_in_api/client.rb @@ -0,0 +1,35 @@ +require "jwt" + +module DfESignInApi + module Client + TIMEOUT_IN_SECONDS = 5 + + def client + @client ||= + Faraday.new( + url: ENV.fetch("DFE_SIGN_IN_API_BASE_URL"), + request: { + timeout: TIMEOUT_IN_SECONDS + } + ) do |faraday| + faraday.request :authorization, "Bearer", jwt + faraday.request :json + faraday.response :json + faraday.adapter Faraday.default_adapter + end + end + + private + + def jwt + @jwt ||= JWT.encode( + { + iss: ENV.fetch("DFE_SIGN_IN_CLIENT_ID"), + aud: ENV.fetch("DFE_SIGN_IN_API_AUDIENCE"), + }, + ENV.fetch("DFE_SIGN_IN_API_SECRET"), + "HS256", + ) + end + end +end diff --git a/app/lib/dfe_sign_in_api/get_user_access_to_service.rb b/app/lib/dfe_sign_in_api/get_user_access_to_service.rb new file mode 100644 index 00000000..c36758fe --- /dev/null +++ b/app/lib/dfe_sign_in_api/get_user_access_to_service.rb @@ -0,0 +1,34 @@ +module DfESignInApi + class GetUserAccessToService + include Client + + attr_reader :org_id, :user_id + + def initialize(org_id:, user_id:) + @org_id = org_id + @user_id = user_id + end + + def call + response = client.get(endpoint) + + if response.success? && response.body.key?("roles") + response.body["roles"].find { |role| authorised_role_codes.include?(role["code"]) } + end + end + + private + + def endpoint + "/services/#{service_id}/organisations/#{org_id}/users/#{user_id}" + end + + def service_id + ENV["DFE_SIGN_IN_CLIENT_ID"] + end + + def authorised_role_codes + ENV.fetch("DFE_SIGN_IN_API_ROLE_CODES").split(",") + end + end +end diff --git a/spec/lib/dfe_sign_in_api/get_user_access_to_service_spec.rb b/spec/lib/dfe_sign_in_api/get_user_access_to_service_spec.rb new file mode 100644 index 00000000..e789feea --- /dev/null +++ b/spec/lib/dfe_sign_in_api/get_user_access_to_service_spec.rb @@ -0,0 +1,39 @@ +require "rails_helper" + +RSpec.describe DfESignInApi::GetUserAccessToService do + describe "#call" do + let(:org_id) { "123" } + let(:user_id) { "456" } + let(:role_id) { "789" } + let(:role_code) { ENV.fetch("DFE_SIGN_IN_API_ROLE_CODES").split(",").first } + let(:endpoint) do + "#{ENV.fetch("DFE_SIGN_IN_API_BASE_URL")}/services/checkrecordteacher/organisations/#{org_id}/users/#{user_id}" + end + + subject { described_class.new(org_id:, user_id:).call } + + context "when the user is authorised" do + before do + stub_request(:get, endpoint) + .to_return_json( + status: 200, + body: { "roles" => [{ "id" => role_id, "code" => role_code }] }, + ) + end + + it { is_expected.to eq({ "id" => role_id, "code" => role_code }) } + end + + context "when the user is not authorised" do + before do + stub_request(:get, endpoint) + .to_return_json( + status: 200, + body: { "roles" => [{ "id" => role_id, "code" => "Unauthorised_Role" }] }, + ) + end + + it { is_expected.to be_nil } + end + end +end From a5a1795674c367c9362782c6f4035225b6f27139 Mon Sep 17 00:00:00 2001 From: Steve Laing Date: Thu, 7 Sep 2023 16:08:03 +0100 Subject: [PATCH 4/5] Create a DsiUserSession record when creating or updating the user We only do this if supplied an optional role. Bypass mechanisisms will still function without a role. --- app/models/dsi_user.rb | 12 +++++++++++- app/models/dsi_user_session.rb | 3 +++ spec/models/dsi_user_spec.rb | 27 ++++++++++++++++++++++++++- 3 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 app/models/dsi_user_session.rb diff --git a/app/models/dsi_user.rb b/app/models/dsi_user.rb index 15a08cf7..38f801e5 100644 --- a/app/models/dsi_user.rb +++ b/app/models/dsi_user.rb @@ -3,8 +3,9 @@ class DsiUser < ApplicationRecord encrypts :first_name, :last_name has_many :search_logs + has_many :dsi_user_sessions, dependent: :destroy - def self.create_or_update_from_dsi(dsi_payload) + def self.create_or_update_from_dsi(dsi_payload, role = nil) dsi_user = find_or_initialize_by(email: dsi_payload.info.fetch(:email)) dsi_user.update!( @@ -13,6 +14,15 @@ def self.create_or_update_from_dsi(dsi_payload) uid: dsi_payload.uid ) + if role.present? + dsi_user.dsi_user_sessions.create!( + role_id: role["id"], + role_code: role["code"], + organisation_id: dsi_payload.extra.raw_info.organisation.id, + organisation_name: dsi_payload.extra.raw_info.organisation.name, + ) + end + dsi_user end end diff --git a/app/models/dsi_user_session.rb b/app/models/dsi_user_session.rb new file mode 100644 index 00000000..e72d6423 --- /dev/null +++ b/app/models/dsi_user_session.rb @@ -0,0 +1,3 @@ +class DsiUserSession < ApplicationRecord + belongs_to :dsi_user +end diff --git a/spec/models/dsi_user_spec.rb b/spec/models/dsi_user_spec.rb index 3ce82659..873f878c 100644 --- a/spec/models/dsi_user_spec.rb +++ b/spec/models/dsi_user_spec.rb @@ -3,7 +3,18 @@ RSpec.describe DsiUser, type: :model do describe ".create_or_update_from_dsi" do let(:dsi_payload) do - OmniAuth::AuthHash.new(uid: "123456", info: { email:, first_name: "John", last_name: "Doe" }) + OmniAuth::AuthHash.new( + uid: "123456", + info: { email:, first_name: "John", last_name: "Doe" }, + extra: { + raw_info: { + organisation: { + id: "09876", + name: "Test Org", + } + } + } + ) end let(:email) { "test@example.com" } @@ -32,5 +43,19 @@ expect(dsi_user.last_name).to eq dsi_payload.info.last_name end end + + context "when the user has a role" do + it "creates a session record" do + role = { "id" => "123", "code" => "TestRole_code" } + described_class.create_or_update_from_dsi(dsi_payload, role) + + dsi_user_session = DsiUser.first.dsi_user_sessions.first + expect(dsi_user_session).to be_present + expect(dsi_user_session.role_id).to eq "123" + expect(dsi_user_session.role_code).to eq "TestRole_code" + expect(dsi_user_session.organisation_id).to eq "09876" + expect(dsi_user_session.organisation_name).to eq "Test Org" + end + end end end From aa4b2b774bfce7fbcdf9ba3a6dbe3a8a3f23714b Mon Sep 17 00:00:00 2001 From: Steve Laing Date: Thu, 7 Sep 2023 16:10:22 +0100 Subject: [PATCH 5/5] Check the role info from DSI API when user logs in We don't do this if bypassing DSI. The first role to match the list of authorised roles will be recorded in the DsiUserSession along with org info. The absence of a valid role takes the user to the 'Not authorised' page. --- .../omniauth_callbacks_controller.rb | 13 ++++++++- .../check_records/sign_in_controller.rb | 3 ++ .../sign_in/not_authorised.html.erb | 5 ++++ config/routes/check_records.rb | 1 + .../check_records/authentication_steps.rb | 29 +++++++++++++++++-- .../unauthorised_user_signs_in_spec.rb | 21 ++++++++++++++ 6 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 app/views/check_records/sign_in/not_authorised.html.erb create mode 100644 spec/system/check_records/unauthorised_user_signs_in_spec.rb diff --git a/app/controllers/check_records/omniauth_callbacks_controller.rb b/app/controllers/check_records/omniauth_callbacks_controller.rb index b79ff8ba..d3d3da32 100644 --- a/app/controllers/check_records/omniauth_callbacks_controller.rb +++ b/app/controllers/check_records/omniauth_callbacks_controller.rb @@ -4,7 +4,18 @@ class CheckRecords::OmniauthCallbacksController < ApplicationController protect_from_forgery except: :dfe_bypass def dfe - @dsi_user = DsiUser.create_or_update_from_dsi(request.env["omniauth.auth"]) + auth = request.env["omniauth.auth"] + + unless CheckRecords::DfESignIn.bypass? + role = DfESignInApi::GetUserAccessToService.new( + org_id: auth.extra.raw_info.organisation.id, + user_id: auth.uid, + ).call + + return redirect_to check_records_not_authorised_path unless role + end + + @dsi_user = DsiUser.create_or_update_from_dsi(auth, role) session[:dsi_user_id] = @dsi_user.id session[:dsi_user_session_expiry] = 2.hours.from_now.to_i diff --git a/app/controllers/check_records/sign_in_controller.rb b/app/controllers/check_records/sign_in_controller.rb index 6c42fd82..6f1aa5b9 100644 --- a/app/controllers/check_records/sign_in_controller.rb +++ b/app/controllers/check_records/sign_in_controller.rb @@ -5,5 +5,8 @@ class SignInController < CheckRecordsController def new end + + def not_authorised + end end end diff --git a/app/views/check_records/sign_in/not_authorised.html.erb b/app/views/check_records/sign_in/not_authorised.html.erb new file mode 100644 index 00000000..4a0a2df4 --- /dev/null +++ b/app/views/check_records/sign_in/not_authorised.html.erb @@ -0,0 +1,5 @@ +

Authorisation required

+

+ You are not authorised to access this service. +

+ diff --git a/config/routes/check_records.rb b/config/routes/check_records.rb index aead2f67..fefd52b8 100644 --- a/config/routes/check_records.rb +++ b/config/routes/check_records.rb @@ -6,6 +6,7 @@ get "/privacy", to: "static#privacy" get "/sign-in", to: "sign_in#new" + get "/not-authorised", to: "sign_in#not_authorised" get "/sign-out", to: "sign_out#new" get "/auth/dfe/callback", to: "omniauth_callbacks#dfe" diff --git a/spec/support/system/check_records/authentication_steps.rb b/spec/support/system/check_records/authentication_steps.rb index d67609fa..f7e12c5c 100644 --- a/spec/support/system/check_records/authentication_steps.rb +++ b/spec/support/system/check_records/authentication_steps.rb @@ -1,13 +1,13 @@ module CheckRecords module AuthenticationSteps - def when_i_sign_in_via_dsi - given_dsi_auth_is_mocked + def when_i_sign_in_via_dsi(authorised: true) + given_dsi_auth_is_mocked(authorised:) when_i_visit_the_sign_in_page and_click_the_dsi_sign_in_button end alias_method :and_i_am_signed_in_via_dsi, :when_i_sign_in_via_dsi - def given_dsi_auth_is_mocked + def given_dsi_auth_is_mocked(authorised:) OmniAuth.config.mock_auth[:dfe] = OmniAuth::AuthHash.new( { provider: "dfe", @@ -16,9 +16,24 @@ def given_dsi_auth_is_mocked email: "test@example.com", first_name: "Test", last_name: "User" + }, + extra: { + raw_info: { + organisation: { + id: org_id, + } + } } } ) + + stub_request( + :get, + "#{ENV.fetch("DFE_SIGN_IN_API_BASE_URL")}/services/checkrecordteacher/organisations/#{org_id}/users/123456", + ).to_return_json( + status: 200, + body: { "roles" => [{ "code" => (authorised ? role_code : "Unauthorised_Role") }] }, + ) end def when_i_visit_the_sign_in_page @@ -28,5 +43,13 @@ def when_i_visit_the_sign_in_page def and_click_the_dsi_sign_in_button click_button "Sign in" end + + def org_id + "12345678-1234-1234-1234-123456789012" + end + + def role_code + ENV.fetch("DFE_SIGN_IN_API_ROLE_CODES").split(",").first + end end end diff --git a/spec/system/check_records/unauthorised_user_signs_in_spec.rb b/spec/system/check_records/unauthorised_user_signs_in_spec.rb new file mode 100644 index 00000000..f7ef199b --- /dev/null +++ b/spec/system/check_records/unauthorised_user_signs_in_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "DSI authentication", host: :check_records do + include AuthorizationSteps + include CheckRecords::AuthenticationSteps + + scenario "Unauthorised user signs in via DfE Sign In", test: :with_stubbed_auth do + when_i_am_authorized_with_basic_auth + when_i_sign_in_via_dsi(authorised: false) + then_i_am_redirected_to_the_unauthorised_page + end + + private + + def then_i_am_redirected_to_the_unauthorised_page + expect(page).to have_current_path("/check-records/not-authorised") + expect(page).to have_content("You are not authorised to access this service") + end +end