From 05346ca0cae7fa1f11ba7e0f2da6205932dd3003 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 18 Aug 2025 12:02:49 +0100 Subject: [PATCH 1/6] Allow fallback_role to be nil When using CIS2, it doesn't make sense (and is potentially misleading) to give users a fallback role since it won't be being used. --- app/models/user.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 75dad8c76e..afc5ddbbb7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -73,6 +73,7 @@ class User < ApplicationRecord -> { where(last_sign_in_at: 1.week.ago..Time.current) } enum :fallback_role, +<<<<<<< HEAD { nurse: 0, admin: 1, @@ -80,6 +81,9 @@ class User < ApplicationRecord healthcare_assistant: 3, prescriber: 4 }, +======= + { nurse: 0, admin: 1, superuser: 2, healthcare_assistant: 3 }, +>>>>>>> 02ac86118 (Allow fallback_role to be nil) prefix: true, validate: { allow_nil: true From f4dd1235fbd7a931ff5502b8ccd07956f23623d8 Mon Sep 17 00:00:00 2001 From: Lakshmi Murugappan Date: Mon, 18 Aug 2025 17:08:38 +0100 Subject: [PATCH 2/6] Only set parent_relationships when present in import This fixes a bug where column coutns were incorrect due to - counting after importing records - parent realtionships being set on the changeset even when parents aren't present --- .../features/import_child_pds_lookup_extravaganza_spec.rb | 8 +++++++- spec/fixtures/cohort_import/pds_extravaganza.csv | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/spec/features/import_child_pds_lookup_extravaganza_spec.rb b/spec/features/import_child_pds_lookup_extravaganza_spec.rb index 6b5fa210d3..33aad7f97c 100644 --- a/spec/features/import_child_pds_lookup_extravaganza_spec.rb +++ b/spec/features/import_child_pds_lookup_extravaganza_spec.rb @@ -217,7 +217,13 @@ def and_an_existing_patient_record_exists @existing_patient_duplicate_review_on_demographics = create( :patient, - given_name: "Maia", + given_name# It looks like the code snippet is incomplete and contains a syntax error. The code + # seems to be attempting to assign a value to a variable named + # `ent_duplicate_review_on_demographics`, but it is missing the assignment operator + # and the value to be assigned. Additionally, the word "crea" does not seem to be a + # valid Ruby keyword or variable name. Please provide more context or correct the + # code snippet for further assistance. + : "Maia", family_name: "Smith", nhs_number: nil, date_of_birth: Date.new(2010, 8, 15), # Different from CSV diff --git a/spec/fixtures/cohort_import/pds_extravaganza.csv b/spec/fixtures/cohort_import/pds_extravaganza.csv index ddd9116c75..eb90bf52df 100644 --- a/spec/fixtures/cohort_import/pds_extravaganza.csv +++ b/spec/fixtures/cohort_import/pds_extravaganza.csv @@ -7,4 +7,4 @@ CHILD_SCHOOL_URN,PARENT_1_NAME,PARENT_1_RELATIONSHIP,PARENT_1_EMAIL,PARENT_1_PHO 123456,Jane Doe,,,01234567896,,,,,Oliver,Green,,2010-08-15,8,789 Silent Street,,London,SW1W 8JL,9435753868 123456,,,,,,,,,Lara,Williams,,2010-05-15,8,,,,B1 1AA, 123456,,,,,,,,,Lucy,McCarthy,,2010-08-16,8,789 Silent Street,,London,SW7 5LE,9435815065 -123456,,,,,,,,,Maia,Smith,,2010-08-16,8,790 Silent Street,,London,W2 3PE,9435789102 \ No newline at end of file +123456,,,,,,,,,Maia,Smith,,2010-08-16,8,790 Silent Street,,London,W2 3PE,9435789102 From 22fea4bcc97caed736ba3fb284c5529c6819a500 Mon Sep 17 00:00:00 2001 From: Alistair Davidson Date: Wed, 2 Jul 2025 16:10:31 +0100 Subject: [PATCH 3/6] Add support for redirecting back to the reporting app after successful login via CIS2, and specs for same. Add one-time-token exchange with the reporting app. Can now demonstrate logging in on Mavis & redirecting back to reporting app with session info Add JWT to encode all the info that is currently top-level in the token verification response Add reporting controller + spec Add pwd_auth_session_token to the user model. Add Warden callbacks to set & clear it on pwd-login and logout. Make authenticate_user_by_jwt! look for user with not just the given user ID, but also the given session_token and pwd_auth_session_token, return 401 if no match Add specs for authentication-related methods Refactor token auth methods to their own concern Delete pwd_auth_session_token on logout also Remove now-redundant user info from one_time_tokens_controller response - the JWT has all this info in it already Closer alignment with OAUTH2.0 spec Change the GET /tokens/:token endpoint to POST /tokens/verify, to make it more aligned with the OAUTH2.0 spec Add a separate secret value - CLIENT_ID Use CLIENT_ID to authorize the client when exchanging a one-time-token for an Access Token, as per the spec Add required grant_type param to the Access Token request Make the Access Token request a POST, and use a more appropriate URL for it (/tokens/verify) Use the request body to pass params to this request, rather than headers - as per the spec Remove authorization by param/header feature flags, replace them with a single reporting_app Rename token to code and redirect_after_login to redirect_uri, to align more closely with the OAUTH 2.0 spec Rename route /tokens/verify -> /tokens/authorize (better-aligned with the OAuth spec)) Rename pwd_auth_session_token to reporting_app_session_token Explicitly set and clear reporting_app_session_token on login/logout regardless of login mechanism Add PlantUML diagrams for auth flow between Mavis and the reporting app Add document including the diagrams of auth flow between Mavis and the reporting app Add user visiting the reporting app diagram to auth flow adoc re-scope the reporting API controller to be under /reporting-api/, and inherit a base_controller which handles the auth and ensures that reporting_app feature flag is enabled. Fixes MAV-1558 and MAV-1588 CodeQL fixes - dont skip verify_authenticity_token, instead protect_from_forgery with null_session - CodeQL objected to a change it previously suggested and failed the build for not using.... - stop CodeQL complaining about update_attribute Remove reportable event tables from schema - they are accidental hangovers from working on a different spike branch, and should not be in this branch Rename reporting_app_redirect_url_with_auth_code_for to reporting_app_redirect_uri_with_auth_code_for as per Mishas suggestion stop the user getting caught in an infinite redirect loop when the reporting_app feature flag is disabled but they log in with a reporting app redirect param remove redundant :to_table use default keyword args remove redundant module-scoping remove redundant `and return` statements from client_id_error! use user: rather than user_id: as param for OneTimeToken.find_or_generate_for! combine migrations to add one_time_tokens and then add another column to it, into one migration better syntax for reliably updating reporting_app_session_token on logout, without CodeQL complaining about bypassing validations move the OneTimeToken model/controller under the Reporting:: scope move /tokens/authorize to /reporting-api/authorize use new /api/reporting routes for all reporting endpoints including authorization. Jira-Issue: MAV-1692 schema update after migrate Updates after rebasing onto next, which now incorporates the refactor of Organisations into Teams & sub-Teams. Some changes needed to the reporting auth interaction with the selected_org session variable, and test scenario setup Rename reporting_app_session_token to reporting_api_session_token Update scope of OneTimeToken after rebase --- Gemfile.lock | 4 + .../concerns/authentication_concern.rb | 18 ++ .../concerns/token_authentication_concern.rb | 70 +++++ .../users/omniauth_callbacks_controller.rb | 1 + app/controllers/users/sessions_controller.rb | 2 +- app/views/users/sessions/new.html.erb | 4 + config/feature_flags.yml | 5 + config/routes.rb | 4 + docs/architecture.adoc | 2 + .../user-visits-the-reporting-app.puml | 2 +- docs/reporting-component-authentication.adoc | 4 +- .../concerns/authentication_concern_spec.rb | 13 + .../token_authentication_concern_spec.rb | 281 ++++++++++++++++++ .../user_password_authentication_spec.rb | 2 + 14 files changed, 408 insertions(+), 4 deletions(-) create mode 100644 app/controllers/concerns/token_authentication_concern.rb create mode 100644 spec/controllers/concerns/token_authentication_concern_spec.rb diff --git a/Gemfile.lock b/Gemfile.lock index af16206436..96cbc90c2a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -246,6 +246,7 @@ GEM concurrent-ruby (~> 1.1) webrick (~> 1.7) websocket-driver (~> 0.7) + ffi (1.17.1-aarch64-linux-gnu) ffi (1.17.1-arm64-darwin) ffi (1.17.1-x86_64-linux-gnu) fhir_models (5.0.0) @@ -394,6 +395,8 @@ GEM net-protocol nio4r (2.7.4) nkf (0.2.0) + nokogiri (1.18.9-aarch64-linux-gnu) + racc (~> 1.4) nokogiri (1.18.9-arm64-darwin) racc (~> 1.4) nokogiri (1.18.9-x86_64-linux-gnu) @@ -743,6 +746,7 @@ GEM zeitwerk (2.7.3) PLATFORMS + aarch64-linux arm64-darwin-22 arm64-darwin-23 arm64-darwin-24 diff --git a/app/controllers/concerns/authentication_concern.rb b/app/controllers/concerns/authentication_concern.rb index ffff28bc18..8b6a46bd23 100644 --- a/app/controllers/concerns/authentication_concern.rb +++ b/app/controllers/concerns/authentication_concern.rb @@ -105,6 +105,24 @@ def authenticate_basic end end + def add_auth_code_to(url, user) + uri = Addressable::URI.parse(url) + auth_code = + ReportingAPI::OneTimeToken.find_or_generate_for!( + user:, + cis2_info: session["cis2_info"] + ).token + uri.query_values = (uri.query_values || {}).merge("code" => auth_code) + uri.to_s + end + + def reporting_app_redirect_uri_with_auth_code_for(user) + if Flipper.enabled?(:reporting_api) + url = session["redirect_uri"] + url.present? ? add_auth_code_to(url, user) : nil + end + end + def after_sign_in_path_for(scope) urls = [] if Flipper.enabled?(:reporting_api) diff --git a/app/controllers/concerns/token_authentication_concern.rb b/app/controllers/concerns/token_authentication_concern.rb new file mode 100644 index 0000000000..4d677ab9e1 --- /dev/null +++ b/app/controllers/concerns/token_authentication_concern.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module TokenAuthenticationConcern + extend ActiveSupport::Concern + + included do + private + + def client_id_error!(token) + if token.blank? + render json: { errors: "invalid_request" }, status: :unauthorized + else + render json: { errors: "unauthorized_client" }, status: :forbidden + end + end + + def authenticate_app_by_client_id! + if Flipper.enabled?(:reporting_api) + # ...as per the spec at https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.3 + given_client_id = params.fetch("client_id", nil) + unless given_client_id == Settings.reporting_api.client_app.client_id + client_id_error!(given_client_id) + end + end + end + + def jwt_if_given + params[:jwt] || + request.headers["Authorization"]&.gsub(/(Bearer\s+)?([:alnum:]*)/, '\2') + end + + def authenticate_user_by_jwt! + jwt = jwt_if_given + jwt_info = decode_jwt!(jwt) + if jwt_info + data = jwt_info.first["data"] + @current_user = + User.find_by( + id: data.dig("user", "id"), + session_token: data.dig("user", "session_token"), + reporting_api_session_token: + data.dig("user", "reporting_api_session_token") + ) + if @current_user + session["user"] = data["user"] + session["cis2_info"] = data["cis2_info"] + authenticate_user! + else + session.clear + client_id_error!(jwt) + Rails.logger.warn "Couldn't find user id #{data.dig("user", "id")} with tokens" + end + end + rescue JWT::DecodeError, NoMethodError + Rails.logger.warn "invalid JWT" + client_id_error!(jwt) + end + end + + def decode_jwt!(jwt) + if jwt + JWT.decode( + jwt, + Settings.reporting_api.client_app.secret, + true, + { algorithm: "HS512" } + ) + end + end +end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 64e597c523..64644447ed 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -77,6 +77,7 @@ def logout signed_out = (Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name)) flash[:notice] = "You have been logged out" if signed_out + redirect_to after_sign_out_path_for(resource_name) end diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 61cf7e5143..24385445ba 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -6,7 +6,7 @@ class Users::SessionsController < Devise::SessionsController skip_after_action :verify_policy_scoped before_action :store_redirect_uri!, only: :new - + layout "one_half" def create diff --git a/app/views/users/sessions/new.html.erb b/app/views/users/sessions/new.html.erb index d9a96febbc..a771dcffa3 100644 --- a/app/views/users/sessions/new.html.erb +++ b/app/views/users/sessions/new.html.erb @@ -10,6 +10,10 @@ <%= hidden_field_tag :redirect_uri, params.fetch(:redirect_uri, "") %> <% end %> + <% if params.has_key?(:redirect_uri) %> + <%= hidden_field_tag :redirect_uri, params.fetch(:redirect_uri, "") %> + <% end %> + <% if devise_mapping.rememberable? %> <%= f.govuk_check_boxes_fieldset :remember_me, multiple: false, legend: nil do %> <%= f.govuk_check_box :remember_me, 1, 0, multiple: false, link_errors: true, small: true, label: { text: "Remember me?" } %> diff --git a/config/feature_flags.yml b/config/feature_flags.yml index fce2acebce..6b54819018 100644 --- a/config/feature_flags.yml +++ b/config/feature_flags.yml @@ -1,3 +1,8 @@ + +reporting_api: Enables the Commissioner reporting component to authenticate to Mavis via OAUTH 2.0 + Authorization Code Flow (https://datatracker.ietf.org/doc/html/rfc6749#section-4.1), and retrieve + statistics from /api/reporting/ + basic_auth: Require users to sign in with basic authentication before they can use the service. diff --git a/config/routes.rb b/config/routes.rb index 90b303a50b..8e223a6122 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -113,6 +113,10 @@ get "totals", controller: :totals, action: :index end end + namespace :reporting do + post "authorize", to: "one_time_tokens#authorize" + get "totals", controller: :totals, action: :index + end end resources :class_imports, path: "class-imports", except: %i[index destroy] diff --git a/docs/architecture.adoc b/docs/architecture.adoc index 25b88e410c..2054f20b58 100644 --- a/docs/architecture.adoc +++ b/docs/architecture.adoc @@ -202,3 +202,5 @@ concerns on the server. * Offline support - Browser-based component * FHIR server synchronisation * Reporting (in development) + + diff --git a/docs/diagrams/reporting_auth/user-visits-the-reporting-app.puml b/docs/diagrams/reporting_auth/user-visits-the-reporting-app.puml index 7866a6d58f..04aafc3497 100644 --- a/docs/diagrams/reporting_auth/user-visits-the-reporting-app.puml +++ b/docs/diagrams/reporting_auth/user-visits-the-reporting-app.puml @@ -46,4 +46,4 @@ endif :process response; stop -@enduml \ No newline at end of file +@enduml diff --git a/docs/reporting-component-authentication.adoc b/docs/reporting-component-authentication.adoc index c98cc2d82d..8018c68d37 100644 --- a/docs/reporting-component-authentication.adoc +++ b/docs/reporting-component-authentication.adoc @@ -10,9 +10,9 @@ ifdef::env-github[] // When PRing changes to the diagrams you can change this attributes // temporarily to the name of the branch you're working on. But don't forget // to change it back to main before merging!! -:github-branch: main +:github-branch: spike/MAV-1406-auth-sharing-with-reporting -:github-repo: nhsuk/manage-vaccinations-in-schools +:github-repo: nhsuk/record-childrens-vaccinations // URL for PlantUML Proxy. Using an attribute mainly because it's just tidier. :plantuml-proxy-url: http://www.plantuml.com/plantuml/proxy?cache=no&src= diff --git a/spec/controllers/concerns/authentication_concern_spec.rb b/spec/controllers/concerns/authentication_concern_spec.rb index 8233de2e3d..bd4b97e166 100644 --- a/spec/controllers/concerns/authentication_concern_spec.rb +++ b/spec/controllers/concerns/authentication_concern_spec.rb @@ -8,6 +8,19 @@ Class .new do # rubocop:disable Style/BlockDelimiters include AuthenticationConcern + attr_accessor :request, :session + + def initialize(request: nil, session: {}) + @request = request + @session = session + end + + def params + {} + end + + def render(content = {}, args = {}) + end attr_accessor :request, :session diff --git a/spec/controllers/concerns/token_authentication_concern_spec.rb b/spec/controllers/concerns/token_authentication_concern_spec.rb new file mode 100644 index 0000000000..cdfd7a792b --- /dev/null +++ b/spec/controllers/concerns/token_authentication_concern_spec.rb @@ -0,0 +1,281 @@ +# frozen_string_literal: true + +describe TokenAuthenticationConcern do + let(:user) { @user = build(:user) } + let(:mock_request) { instance_double(request.class, headers: {}) } + let(:sample_class) do + Class + .new do # rubocop:disable Style/BlockDelimiters + include TokenAuthenticationConcern + attr_accessor :request, :session + + def authenticate_user! + end + + def initialize(request: nil, session: {}) + @request = request + @session = session + end + + def params + {} + end + + def render(content = {}, args = {}) + end + + def current_user + @user + end + end # rubocop:disable Style/MethodCalledOnDoEndBlock + .new(request: mock_request) + end + + describe "#jwt_if_given" do + context "when there is a jwt param" do + before do + allow(sample_class).to receive(:params).and_return({ jwt: "myjwt" }) + end + + it "returns the jwt param" do + expect(sample_class.send(:jwt_if_given)).to eq("myjwt") + end + end + + context "when there is no jwt param" do + context "but there is an Authorization header" do + before do + sample_class.request = + instance_double( + request.class, + headers: { + "Authorization" => "Bearer myjwt" + } + ) + end + + it "returns the value of the Authorization header, without the leading 'Bearer'" do + result = sample_class.send(:jwt_if_given) + expect(result).to eq("myjwt") + end + end + + context "and there is no Authorization header" do + it "returns nil" do + expect(sample_class.send(:jwt_if_given)).to be_nil + end + end + end + end + + describe "#authenticate_app_by_client_id!" do + let(:client_id) { "something" } + + context "when the :reporting_api feature flag is enabled" do + before { Flipper.enable(:reporting_api) } + + context "and the client_id param is provided" do + before do + allow(sample_class).to receive(:params).and_return( + { client_id: client_id }.with_indifferent_access + ) + end + + context "and the client_id param contains the reporting app's client_id" do + let(:client_id) { Settings.reporting_api.client_app.client_id } + + it "does not cause a token error" do + expect(sample_class).not_to receive(:client_id_error!) + sample_class.send(:authenticate_app_by_client_id!) + end + end + + context "and the client_id param does not contain the reporting app client_id" do + it "causes a token error" do + expect(sample_class).to receive(:client_id_error!) + sample_class.send(:authenticate_app_by_client_id!) + end + end + end + end + end + + describe "#client_id_error!" do + context "given an empty token" do + let(:token) { "" } + + it "renders a invalid_request error, with status :unauthorized" do + expect(sample_class).to receive(:render).with( + json: { + errors: "invalid_request" + }, + status: :unauthorized + ) + sample_class.send(:client_id_error!, token) + end + end + + context "given a token that is not empty, but does not match the reporting app's client_id" do + let(:token) { "unmatched token" } + + it "renders a unauthorized_client error, with status forbidden" do + expect(sample_class).to receive(:render).with( + json: { + errors: "unauthorized_client" + }, + status: :forbidden + ) + sample_class.send(:client_id_error!, token) + end + end + end + + describe "#authenticate_user_by_jwt!" do + let(:jwt) { "" } + let(:user_id) { 0 } + let(:session_token) { "123456abcdef" } + let(:reporting_api_session_token) { "0987654321123456abcdef" } + + let(:user) do + create( + :user, + session_token: session_token, + reporting_api_session_token: reporting_api_session_token + ) + end + + before do + sample_class.request = + instance_double(request.class, headers: { "Authorization" => jwt }) + end + + context "when a valid jwt is given" do + let(:jwt) { "validjwt" } + let(:user_info) do + [ + { + "data" => { + "user" => { + "id" => user_id, + "session_token" => session_token, + "reporting_api_session_token" => reporting_api_session_token + }, + "cis2_info" => { + "some_key" => "some value" + } + } + } + ] + end + + before do + allow(sample_class).to receive(:decode_jwt!).with(jwt).and_return( + user_info + ) + allow(sample_class).to receive(:authenticate_user!) + end + + it "decodes the JWT" do + expect(sample_class).to receive(:decode_jwt!).with(jwt) + sample_class.send(:authenticate_user_by_jwt!) + end + + context "when a User exists with the values of id, session_token and reporting_api_session_token" do + let(:user_id) { user.id } + + it "copies the user key into session['user']" do + sample_class.send(:authenticate_user_by_jwt!) + expect(sample_class.session["user"]).to eq( + user_info.first["data"]["user"] + ) + end + + it "copies the cis2_info key into session['cis2_info']" do + sample_class.send(:authenticate_user_by_jwt!) + expect(sample_class.session["cis2_info"]).to eq( + user_info.first["data"]["cis2_info"] + ) + end + + it "calls authenticate_user!" do + expect(sample_class).to receive(:authenticate_user!) + sample_class.send(:authenticate_user_by_jwt!) + end + end + + context "when a User does not exist with the values of id, session_token and reporting_api_session_token" do + let(:user_id) { user.id } + + before do + user.update!( + session_token: "someothersessiontoken", + reporting_api_session_token: "someotherpwdauthsessiontoken" + ) + sample_class.session = { + user_id: user.id, + some_other_session_var: "some value" + } + end + + it "clears the session" do + sample_class.send(:authenticate_user_by_jwt!) + expect(sample_class.session).to be_empty + end + + it "calls client_id_error!" do + expect(sample_class).to receive(:client_id_error!) + sample_class.send(:authenticate_user_by_jwt!) + end + end + end + + context "when a valid jwt is not given" do + it "causes a client_id_error!" do + expect(sample_class).to receive(:client_id_error!) + sample_class.send(:authenticate_user_by_jwt!) + end + end + end + + describe "decode_jwt!" do + context "given a jwt" do + let(:jwt) { "somejwt" } + let(:decoded_jwt) do + { "some_key" => { "some nested_key" => "some nested value" } } + end + + it "tries to decode it with the mavis reporting app secret" do + expect(JWT).to receive(:decode).with( + jwt, + Settings.reporting_api.client_app.secret, + true, + { algorithm: "HS512" } + ) #.and_return(decoded_jwt) + sample_class.send(:decode_jwt!, jwt) + end + + context "when decoding works" do + before do + allow(JWT).to receive(:decode).with( + jwt, + Settings.reporting_api.client_app.secret, + true, + { algorithm: "HS512" } + ).and_return(decoded_jwt) + end + + it "returns the decoded JWT" do + expect(sample_class.send(:decode_jwt!, jwt)).to eq(decoded_jwt) + end + end + + context "when decoding does not work" do + it "raises an exception" do + expect { sample_class.send(:decode_jwt!, jwt) }.to raise_error( + JWT::DecodeError + ) + end + end + end + end +end diff --git a/spec/features/user_password_authentication_spec.rb b/spec/features/user_password_authentication_spec.rb index 7ac633ab62..1b8337e130 100644 --- a/spec/features/user_password_authentication_spec.rb +++ b/spec/features/user_password_authentication_spec.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true describe "User password authentication", :local_users do + # include RedirectHelper + before { given_the_cis2_feature_flag_is_disabled } scenario "going through the start page then signing out" do From 06ea80f8f2882fd2f0920562fdec21c2317ae093 Mon Sep 17 00:00:00 2001 From: Brage Gording Date: Mon, 21 Jul 2025 12:29:35 +0100 Subject: [PATCH 4/6] Create a service discovery setup for cross service communication - For now only web service needs to recieve requests - Use HTTP setup initially - All communication in private subnet - Communication in private subnet is further restricted via security groups - HTTPS requires a private certificate authority which comes with significant price increase - Also include new ECS service for reporting - Since it is user-facing use CODE_DEPLOY deployment - Needs a VALKEY instance for ensuring validity of login-tokens Add necessary account-level resources - Repository - Deployment permission (ECS deployment only) Modify variable and digest population for reporting service - Ensure that the existing deployment workflows for web and goodjob continue to function as normal - Reporting service has a separate deployment workflow - This will all be standardized once the IaC is extracted to its own repository Reference variable for port numbers - Keeping it DRY Include a shared secret for jwt signing - Autorotated every Monday between 01:00-02:00 UTC fix missing bracket (merge artefact?) Create a service discovery setup for cross service communication - For now only web service needs to recieve requests - Use HTTP setup initially - All communication in private subnet - Communication in private subnet is further restricted via security groups - HTTPS requires a private certificate authority which comes with significant price increase - Also include new ECS service for reporting - Since it is user-facing use CODE_DEPLOY deployment - Needs a VALKEY instance for ensuring validity of login-tokens Add necessary account-level resources - Repository - Deployment permission (ECS deployment only) Modify variable and digest population for reporting service - Ensure that the existing deployment workflows for web and goodjob continue to function as normal - Reporting service has a separate deployment workflow - This will all be standardized once the IaC is extracted to its own repository Include a shared secret for jwt signing - Autorotated every Monday between 01:00-02:00 UTC Update deployment permissions - also update healthcheck endpoint update environment variables --- terraform/account/deployment_permissions.tf | 55 +++++-- terraform/account/main.tf | 5 + .../iam_policy_DeployECSServiceResources.json | 61 +++++++ .../iam_policy_DeployMavisResources.json | 6 + ...github_trust_policy_development.json.tftpl | 2 +- ..._github_trust_policy_production.json.tftpl | 4 +- terraform/account/variables.tf | 5 + terraform/app/codedeploy.tf | 49 ++++++ terraform/app/ecs.tf | 153 +++++++++++++++--- terraform/app/iam_policy_documents.tf | 24 ++- terraform/app/kms.tf | 19 +++ terraform/app/loadbalancer.tf | 66 +++++++- terraform/app/main.tf | 4 + terraform/app/modules/ecs_service/main.tf | 22 ++- .../app/modules/ecs_service/variables.tf | 21 +++ terraform/app/rds.tf | 4 +- terraform/app/resources/.gitignore | 1 + terraform/app/resources/rotate_secret.py | 94 +++++++++++ terraform/app/ssm_parameters.tf | 91 +++++++++++ terraform/app/valkey.tf | 114 +++---------- terraform/app/variables.tf | 37 +++++ 21 files changed, 691 insertions(+), 146 deletions(-) create mode 100644 terraform/account/resources/iam_policy_DeployECSServiceResources.json create mode 100644 terraform/app/resources/.gitignore create mode 100644 terraform/app/resources/rotate_secret.py diff --git a/terraform/account/deployment_permissions.tf b/terraform/account/deployment_permissions.tf index 5352642b2e..7d8847e615 100644 --- a/terraform/account/deployment_permissions.tf +++ b/terraform/account/deployment_permissions.tf @@ -3,7 +3,8 @@ resource "aws_iam_role" "mavis_deploy" { name = "GithubDeployMavisAndInfrastructure" description = "Role allowing terraform deployment from github workflows" assume_role_policy = templatefile("resources/iam_role_github_trust_policy_${var.environment}.json.tftpl", { - account_id = var.account_id + account_id = var.account_id, + repository = "nhsuk/manage-vaccinations-in-schools" }) } @@ -27,7 +28,8 @@ resource "aws_iam_role" "data_replication_deploy" { name = "GithubDeployDataReplicationInfrastructure" description = "Role to be assumed by github workflows dealing with the creation and destruction of the data-replication infrastructure." assume_role_policy = templatefile("resources/iam_role_github_trust_policy_${var.environment}.json.tftpl", { - account_id = var.account_id + account_id = var.account_id, + repository = "nhsuk/manage-vaccinations-in-schools" }) } @@ -46,28 +48,55 @@ resource "aws_iam_role_policy_attachment" "data_replication" { policy_arn = each.value } +################ Deploy ECS Service ################ + +resource "aws_iam_role" "deploy_ecs_service" { + name = "GithubDeployECSService" + description = "Role allowing terraform deployment of ECS services from github workflows" + assume_role_policy = templatefile("resources/iam_role_github_trust_policy_${var.environment}.json.tftpl", { + account_id = var.account_id, + repository = "NHSDigital/manage-vaccinations-in-schools-reporting" + }) +} + +resource "aws_iam_policy" "deploy_ecs_service" { + name = "DeployECSServiceResources" + description = "Permissions for GithubDeployECSService role" + policy = file("resources/iam_policy_DeployECSServiceResources.json") + lifecycle { + ignore_changes = [description] + } +} + +resource "aws_iam_role_policy_attachment" "deploy_ecs_service" { + for_each = local.ecs_deploy_policies + role = aws_iam_role.deploy_ecs_service.name + policy_arn = each.value +} + ################# Deploy Monitoring ################ -resource "aws_iam_role" "monitoring_deploy" { - name = "GithubDeployMonitoring" - description = "Role allowing terraform deployment of monitoring resources from github workflows" +resource "aws_iam_role" "deploy_ecs_service" { + name = "GithubDeployECSService" + description = "Role allowing terraform deployment of ECS services from github workflows" assume_role_policy = templatefile("resources/iam_role_github_trust_policy_${var.environment}.json.tftpl", { - account_id = var.account_id + account_id = var.account_id, + repository = "nhsuk/manage-vaccinations-in-schools" }) } -resource "aws_iam_policy" "monitoring_deploy" { - name = "DeployMonitoringResources" - description = "Permissions for GithubDeployMonitoring role" - policy = file("resources/iam_policy_DeployMonitoringResources.json") +resource "aws_iam_policy" "deploy_ecs_service" { + name = "DeployECSServiceResources" + description = "Permissions for GithubDeployECSService role" + policy = file("resources/iam_policy_DeployECSServiceResources.json") lifecycle { ignore_changes = [description] } } -resource "aws_iam_role_policy_attachment" "monitoring_deploy" { - for_each = local.monitoring_policies - role = aws_iam_role.monitoring_deploy.name +resource "aws_iam_role_policy_attachment" "deploy_ecs_service" { + for_each = local.ecs_deploy_policies + role = aws_iam_role.deploy_ecs_service.name policy_arn = each.value } diff --git a/terraform/account/main.tf b/terraform/account/main.tf index 64d754f3cb..e1552ae1c9 100644 --- a/terraform/account/main.tf +++ b/terraform/account/main.tf @@ -122,6 +122,11 @@ resource "aws_ecr_lifecycle_policy" "mavis" { }) } +resource "aws_ecr_repository" "mavis_reporting" { + name = "mavis/reporting" + image_tag_mutability = "MUTABLE" +} + #### Access Analyzer diff --git a/terraform/account/resources/iam_policy_DeployECSServiceResources.json b/terraform/account/resources/iam_policy_DeployECSServiceResources.json new file mode 100644 index 0000000000..8fe50c1090 --- /dev/null +++ b/terraform/account/resources/iam_policy_DeployECSServiceResources.json @@ -0,0 +1,61 @@ +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": [ + "ecs:UpdateService", + "ecs:DescribeServices", + "ecs:ListServices", + "ecs:RegisterTaskDefinition", + "ecs:DeregisterTaskDefinition", + "ecs:DescribeTaskDefinition", + "ecs:ListTaskDefinitions", + "ecs:DescribeClusters", + "ecs:DescribeTasks", + "ecs:DescribeTaskSets", + "ecs:StartTask", + "ecs:ListServiceDeployments", + "ecs:DescribeServiceDeployments", + "ecs:UntagResource", + "ecs:TagResource", + "ecs:ListClusters", + "ecs:ListContainerInstances", + "ecs:ListTaskDefinitionFamilies", + "ecs:ListTasks", + "ecr:PutImage", + "ecr:InitiateLayerUpload", + "ecr:UploadLayerPart", + "ecr:CompleteLayerUpload", + "codedeploy:BatchGetApplicationRevisions", + "codedeploy:BatchGetApplications", + "codedeploy:BatchGetDeploymentGroups", + "codedeploy:BatchGetDeployments", + "codedeploy:ContinueDeployment", + "codedeploy:CreateApplication", + "codedeploy:CreateDeployment", + "codedeploy:CreateDeploymentGroup", + "codedeploy:GetApplication", + "codedeploy:GetApplicationRevision", + "codedeploy:GetDeployment", + "codedeploy:GetDeploymentConfig", + "codedeploy:GetDeploymentGroup", + "codedeploy:GetDeploymentTarget", + "codedeploy:ListApplicationRevisions", + "codedeploy:ListApplications", + "codedeploy:ListDeploymentConfigs", + "codedeploy:ListDeploymentGroups", + "codedeploy:ListDeployments", + "codedeploy:ListDeploymentTargets", + "codedeploy:RegisterApplicationRevision", + "codedeploy:StopDeployment", + "ssm:DescribeParameters", + "ssm:GetParameter", + "ssm:GetParameters", + "ssm:GetParametersByPath", + "iam:PassRole" + ], + "Resource": ["*"] + } + ] +} diff --git a/terraform/account/resources/iam_policy_DeployMavisResources.json b/terraform/account/resources/iam_policy_DeployMavisResources.json index e8a3e90c84..07a0931028 100644 --- a/terraform/account/resources/iam_policy_DeployMavisResources.json +++ b/terraform/account/resources/iam_policy_DeployMavisResources.json @@ -156,6 +156,12 @@ "elasticache:ModifyCacheParameterGroup", "elasticache:ModifyCacheSubnetGroup", "elasticache:IncreaseReplicaCount" + "lambda:InvokeFunction", + "lambda:DeleteFunction", + "lambda:CreateFunction", + "lambda:CreateAlias", + "lambda:DeleteAlias", + "lambda:UpdateAlias" ], "Resource": ["*"] } diff --git a/terraform/account/resources/iam_role_github_trust_policy_development.json.tftpl b/terraform/account/resources/iam_role_github_trust_policy_development.json.tftpl index c8b00eb1b5..378b9fea87 100644 --- a/terraform/account/resources/iam_role_github_trust_policy_development.json.tftpl +++ b/terraform/account/resources/iam_role_github_trust_policy_development.json.tftpl @@ -12,7 +12,7 @@ "token.actions.githubusercontent.com:aud": "sts.amazonaws.com" }, "StringLike": { - "token.actions.githubusercontent.com:sub": "repo:nhsuk/manage-vaccinations-in-schools:*" + "token.actions.githubusercontent.com:sub": "repo:${repository}:*" } } } diff --git a/terraform/account/resources/iam_role_github_trust_policy_production.json.tftpl b/terraform/account/resources/iam_role_github_trust_policy_production.json.tftpl index d1b2396034..ce256d6583 100644 --- a/terraform/account/resources/iam_role_github_trust_policy_production.json.tftpl +++ b/terraform/account/resources/iam_role_github_trust_policy_production.json.tftpl @@ -10,8 +10,8 @@ "Condition": { "StringEquals": { "token.actions.githubusercontent.com:sub": [ - "repo:nhsuk/manage-vaccinations-in-schools:ref:refs/heads/main", - "repo:nhsuk/manage-vaccinations-in-schools:environment:production" + "repo:${repository}:ref:refs/heads/main", + "repo:${repository}:environment:production" ], "token.actions.githubusercontent.com:aud": "sts.amazonaws.com" } diff --git a/terraform/account/variables.tf b/terraform/account/variables.tf index d7a1fa3f3c..592407e056 100644 --- a/terraform/account/variables.tf +++ b/terraform/account/variables.tf @@ -29,6 +29,11 @@ locals { mavis_deploy = aws_iam_policy.mavis_deploy.arn }) + ecs_deploy_policies = { + ecr_read = "arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly" + deploy_ecs_service = aws_iam_policy.deploy_ecs_service.arn + } + monitoring_policies = merge(local.base_policies, { monitoring_deploy = aws_iam_policy.monitoring_deploy.arn }) diff --git a/terraform/app/codedeploy.tf b/terraform/app/codedeploy.tf index 3a4190ef09..638f4f8541 100644 --- a/terraform/app/codedeploy.tf +++ b/terraform/app/codedeploy.tf @@ -52,6 +52,55 @@ resource "aws_codedeploy_deployment_group" "blue_green_deployment_group" { } } +resource "aws_codedeploy_deployment_group" "reporting" { + app_name = aws_codedeploy_app.mavis.name + deployment_config_name = "CodeDeployDefault.ECSAllAtOnce" + deployment_group_name = "reporting-${var.environment}" + service_role_arn = aws_iam_role.code_deploy.arn + + auto_rollback_configuration { + enabled = true + events = ["DEPLOYMENT_FAILURE"] + } + + blue_green_deployment_config { + deployment_ready_option { + action_on_timeout = "CONTINUE_DEPLOYMENT" + } + + terminate_blue_instances_on_deployment_success { + action = "TERMINATE" + termination_wait_time_in_minutes = 1 + } + } + + deployment_style { + deployment_option = "WITH_TRAFFIC_CONTROL" + deployment_type = "BLUE_GREEN" + } + + ecs_service { + cluster_name = aws_ecs_cluster.cluster.name + service_name = module.reporting_service.service.name + } + + load_balancer_info { + target_group_pair_info { + prod_traffic_route { + listener_arns = [aws_lb_listener.app_listener_https.arn] + } + + target_group { + name = aws_lb_target_group.reporting_blue.name + } + + target_group { + name = aws_lb_target_group.reporting_green.name + } + } + } +} + resource "aws_s3_bucket" "code_deploy_bucket" { bucket = var.appspec_bucket force_destroy = true diff --git a/terraform/app/ecs.tf b/terraform/app/ecs.tf index 8014eebcc8..da10d82b0a 100644 --- a/terraform/app/ecs.tf +++ b/terraform/app/ecs.tf @@ -1,7 +1,7 @@ resource "aws_security_group_rule" "web_service_alb_ingress" { type = "ingress" - from_port = 4000 - to_port = 4000 + from_port = local.container_ports.web + to_port = local.container_ports.web protocol = "tcp" security_group_id = module.web_service.security_group_id source_security_group_id = aws_security_group.lb_service_sg.id @@ -10,6 +10,30 @@ resource "aws_security_group_rule" "web_service_alb_ingress" { } } +resource "aws_security_group_rule" "reporting_service_alb_ingress" { + type = "ingress" + from_port = local.container_ports.reporting + to_port = local.container_ports.reporting + protocol = "tcp" + security_group_id = module.reporting_service.security_group_id + source_security_group_id = aws_security_group.lb_service_sg.id + lifecycle { + create_before_destroy = true + } +} + +resource "aws_security_group_rule" "reporting_to_web_service" { + type = "ingress" + from_port = local.container_ports.web + to_port = local.container_ports.web + protocol = "tcp" + security_group_id = module.web_service.security_group_id + source_security_group_id = module.reporting_service.security_group_id + lifecycle { + create_before_destroy = true + } +} + resource "aws_ecs_cluster" "cluster" { name = "mavis-${var.environment}" @@ -19,11 +43,50 @@ resource "aws_ecs_cluster" "cluster" { } } +resource "aws_service_discovery_private_dns_namespace" "internal" { + name = "mavis.${var.environment}.aws-int" + description = "Private namespace for ECS service discovery" + vpc = aws_vpc.application_vpc.id + + tags = { + Name = "ecs-service-discovery-${var.environment}" + } +} + +resource "aws_service_discovery_service" "web" { + name = "web" + + dns_config { + namespace_id = aws_service_discovery_private_dns_namespace.internal.id + dns_records { + ttl = 10 # TODO: Decide on optimal caching time for DNS records + type = "A" + } + routing_policy = "MULTIVALUE" # For multiple tasks; use "WEIGHTED" if custom weights needed + } + + tags = { + Name = "maivs-${var.environment}-web" + } +} + module "web_service" { source = "./modules/ecs_service" task_config = { - environment = local.task_envs - secrets = local.task_secrets + environment = concat(local.task_envs, [ + { + name = "MAVIS__REPORTING_API__CLIENT_APP__CLIENT_ID" + value = aws_secretsmanager_secret.jwt_sign.name + } + ] + ) + secrets = concat( + local.task_secrets, + [{ + name = "MAVIS__REPORTING_API__CLIENT_APP__SECRET" + valueFrom = aws_secretsmanager_secret.jwt_sign.arn + }] + ) cpu = 1024 memory = 2048 docker_image = "${var.account_id}.dkr.ecr.eu-west-2.amazonaws.com/${var.docker_image}@${var.image_digest}" @@ -31,7 +94,7 @@ module "web_service" { task_role_arn = aws_iam_role.ecs_task_role.arn log_group_name = aws_cloudwatch_log_group.ecs_log_group.name region = var.region - health_check_command = ["CMD-SHELL", "./bin/internal_healthcheck http://localhost:4000/health/database"] + health_check_command = ["CMD-SHELL", "./bin/internal_healthcheck http://localhost:${local.container_ports.web}/health/database"] } network_params = { subnets = [aws_subnet.private_subnet_a.id, aws_subnet.private_subnet_b.id] @@ -39,7 +102,7 @@ module "web_service" { } loadbalancer = { target_group_arn = local.ecs_initial_lb_target_group - container_port = 4000 + container_port = local.container_ports.web } autoscaling_policies = tomap({ cpu = { @@ -49,13 +112,14 @@ module "web_service" { scale_out_cooldown = 300 } }) - cluster_id = aws_ecs_cluster.cluster.id - cluster_name = aws_ecs_cluster.cluster.name - minimum_replica_count = var.minimum_web_replicas - maximum_replica_count = var.maximum_web_replicas - environment = var.environment - server_type = "web" - deployment_controller = "CODE_DEPLOY" + cluster_id = aws_ecs_cluster.cluster.id + cluster_name = aws_ecs_cluster.cluster.name + minimum_replica_count = var.minimum_web_replicas + maximum_replica_count = var.maximum_web_replicas + environment = var.environment + server_type = "web" + deployment_controller = "CODE_DEPLOY" + service_discovery_service_arn = aws_service_discovery_service.web.arn } module "good_job_service" { @@ -70,7 +134,7 @@ module "good_job_service" { task_role_arn = aws_iam_role.ecs_task_role.arn log_group_name = aws_cloudwatch_log_group.ecs_log_group.name region = var.region - health_check_command = ["CMD-SHELL", "./bin/internal_healthcheck http://localhost:4000/status/connected"] + health_check_command = ["CMD-SHELL", "./bin/internal_healthcheck http://localhost:${local.container_ports.good_job}/status/connected"] } network_params = { subnets = [aws_subnet.private_subnet_a.id, aws_subnet.private_subnet_b.id] @@ -84,14 +148,30 @@ module "good_job_service" { server_type = "good-job" } -module "sidekiq_service" { +module "reporting_service" { source = "./modules/ecs_service" task_config = { - environment = local.task_envs - secrets = local.task_secrets + environment = [ + { + name = "VALKEY_ADDRESS" + value = aws_elasticache_serverless_cache.reporting_service.endpoint[0].address + }, + { + name = "VALKEY_PORT" + value = aws_elasticache_serverless_cache.reporting_service.endpoint[0].port + }, + { + name = "CLIENT_ID" + value = aws_secretsmanager_secret.jwt_sign.name + } + ] + secrets = [{ + name = "CLIENT_SECRET" + valueFrom = aws_secretsmanager_secret.jwt_sign.arn + }] cpu = 1024 memory = 2048 - docker_image = "${var.account_id}.dkr.ecr.eu-west-2.amazonaws.com/${var.docker_image}@${var.image_digest}" + docker_image = local.reporting_image execution_role_arn = aws_iam_role.ecs_task_execution_role.arn task_role_arn = aws_iam_role.ecs_task_role.arn log_group_name = aws_cloudwatch_log_group.ecs_log_group.name @@ -102,14 +182,39 @@ module "sidekiq_service" { subnets = [aws_subnet.private_subnet_a.id, aws_subnet.private_subnet_b.id] vpc_id = aws_vpc.application_vpc.id } - minimum_replica_count = var.sidekiq_replicas - maximum_replica_count = var.sidekiq_replicas + loadbalancer = { + target_group_arn = local.reporting_initial_lb_target_group + container_port = local.container_ports.reporting + } + autoscaling_policies = tomap({ + cpu = { + predefined_metric_type = "ECSServiceAverageCPUUtilization" + target_value = 60 + scale_in_cooldown = 600 + scale_out_cooldown = 300 + } + }) + container_port = local.container_ports.reporting + minimum_replica_count = var.minimum_reporting_replicas + maximum_replica_count = var.maximum_reporting_replicas cluster_id = aws_ecs_cluster.cluster.id cluster_name = aws_ecs_cluster.cluster.name environment = var.environment - server_type = "sidekiq" + server_type = "reporting" + deployment_controller = "CODE_DEPLOY" +} + +# Fetch the task definition for the reporting service if it exists, as it has a separate deployment process/pipeline +# TODO: Remove this workaround when extracting infrastructure from the monorepo is complete +data "aws_ecs_task_definition" "reporting" { + count = var.reporting_digest == null ? 1 : 0 + task_definition = "arn:aws:ecs:${var.region}:${var.account_id}:task-definition/mavis-reporting-task-definition-${var.environment}" +} - depends_on = [ - aws_elasticache_replication_group.valkey - ] +locals { + reporting_image = ( + var.reporting_digest == null ? + jsondecode(data.aws_ecs_task_definition.reporting[0].container_definitions)[0]["image"] : + "${var.account_id}.dkr.ecr.eu-west-2.amazonaws.com/mavis/reporting@${var.reporting_digest}" + ) } diff --git a/terraform/app/iam_policy_documents.tf b/terraform/app/iam_policy_documents.tf index 622073eb80..b0425b31db 100644 --- a/terraform/app/iam_policy_documents.tf +++ b/terraform/app/iam_policy_documents.tf @@ -3,13 +3,16 @@ data "aws_iam_policy_document" "codedeploy" { statement { actions = ["ecs:DescribeServices", "ecs:UpdateServicePrimaryTaskSet"] - resources = [module.web_service.service.id] + resources = [module.web_service.service.id, module.reporting_service.service.id] effect = "Allow" } statement { - actions = ["ecs:CreateTaskSet", "ecs:DeleteTaskSet"] - resources = ["arn:aws:ecs:*:*:task-set/${aws_ecs_cluster.cluster.name}/${module.web_service.service.name}/*"] - effect = "Allow" + actions = ["ecs:CreateTaskSet", "ecs:DeleteTaskSet"] + resources = [ + "arn:aws:ecs:*:*:task-set/${aws_ecs_cluster.cluster.name}/${module.web_service.service.name}/*", + "arn:aws:ecs:*:*:task-set/${aws_ecs_cluster.cluster.name}/${module.reporting_service.service.name}/*" + ] + effect = "Allow" } statement { actions = [ @@ -51,16 +54,21 @@ data "aws_iam_policy_document" "ecs_secrets_access" { statement { sid = "railsKeySid" actions = ["ssm:GetParameters"] - resources = concat([ - "arn:aws:ssm:${var.region}:${var.account_id}:parameter${var.rails_master_key_path}" - ], local.parameter_store_arns) + resources = concat( + [ + "arn:aws:ssm:${var.region}:${var.account_id}:parameter${var.rails_master_key_path}", + ], + local.parameter_store_arns, + + ) effect = "Allow" } statement { sid = "dbSecretSid" actions = ["secretsmanager:GetSecretValue"] resources = [ - aws_rds_cluster.core.master_user_secret[0].secret_arn + aws_rds_cluster.core.master_user_secret[0].secret_arn, + aws_secretsmanager_secret.jwt_sign.arn ] effect = "Allow" } diff --git a/terraform/app/kms.tf b/terraform/app/kms.tf index 752bb752d5..fb6dc7f129 100644 --- a/terraform/app/kms.tf +++ b/terraform/app/kms.tf @@ -42,3 +42,22 @@ resource "aws_kms_key" "rds_cluster" { ] }) } + +resource "aws_kms_key" "reporting_valkey" { + description = "Custom KMS key for new Aurora cluster" + policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Sid = "AllowAccount" + Effect = "Allow" + Principal = { + AWS = ["arn:aws:iam::${var.account_id}:root"] + } + Action = "kms:*" + Resource = "*" + } + ] + }) +} + diff --git a/terraform/app/loadbalancer.tf b/terraform/app/loadbalancer.tf index c25240ecdc..a0178d473b 100644 --- a/terraform/app/loadbalancer.tf +++ b/terraform/app/loadbalancer.tf @@ -68,12 +68,13 @@ resource "aws_lb" "app_lb" { } security_groups = [aws_security_group.lb_service_sg.id] subnets = [aws_subnet.public_subnet_a.id, aws_subnet.public_subnet_b.id] + depends_on = [aws_security_group_rule.lb_ingress_https] #TODO: Delete after migration drop_invalid_header_fields = true } resource "aws_lb_target_group" "blue" { name = "mavis-blue-${var.environment}" - port = 4000 + port = local.container_ports.web protocol = "HTTP" vpc_id = aws_vpc.application_vpc.id target_type = "ip" @@ -91,7 +92,7 @@ resource "aws_lb_target_group" "blue" { resource "aws_lb_target_group" "green" { name = "mavis-green-${var.environment}" - port = 4000 + port = local.container_ports.web protocol = "HTTP" vpc_id = aws_vpc.application_vpc.id target_type = "ip" @@ -107,6 +108,42 @@ resource "aws_lb_target_group" "green" { } } +resource "aws_lb_target_group" "reporting_blue" { + name = "mavis-rep-blue-${var.environment}" + port = local.container_ports.reporting + protocol = "HTTP" + vpc_id = aws_vpc.application_vpc.id + target_type = "ip" + health_check { + path = "/reporting/healthcheck" + protocol = "HTTP" + port = "traffic-port" + matcher = "200" + interval = 10 + timeout = 5 + healthy_threshold = 2 + unhealthy_threshold = 2 + } +} + +resource "aws_lb_target_group" "reporting_green" { + name = "mavis-rep-green-${var.environment}" + port = local.container_ports.reporting + protocol = "HTTP" + vpc_id = aws_vpc.application_vpc.id + target_type = "ip" + health_check { + path = "/reporting/healthcheck" + protocol = "HTTP" + port = "traffic-port" + matcher = "200" + interval = 10 + timeout = 5 + healthy_threshold = 2 + unhealthy_threshold = 2 + } +} + resource "aws_lb_target_group" "dump" { name = "dump-${var.environment}" port = 80 @@ -168,6 +205,29 @@ resource "aws_lb_listener_rule" "forward_to_app" { } } +resource "aws_lb_listener_rule" "forward_to_reporting" { + listener_arn = aws_lb_listener.app_listener_https.arn + priority = 49000 + action { + type = "forward" + target_group_arn = local.reporting_initial_lb_target_group + } + condition { + path_pattern { + values = var.reporting_endpoints + } + } + condition { + host_header { + values = local.host_headers + } + } + + lifecycle { + ignore_changes = [action] + } +} + resource "aws_lb_listener_rule" "redirect_to_https" { listener_arn = aws_lb_listener.app_listener_http.arn priority = 50000 @@ -198,4 +258,4 @@ module "dns_route53" { zone_id = aws_lb.app_lb.zone_id zone_name = var.zone_name domain_names = tolist(toset(values(var.http_hosts))) -} +} \ No newline at end of file diff --git a/terraform/app/main.tf b/terraform/app/main.tf index 412cdc1c98..3ed6d41a21 100644 --- a/terraform/app/main.tf +++ b/terraform/app/main.tf @@ -9,6 +9,10 @@ terraform { source = "hashicorp/time" version = "~> 0.12" } + archive = { + source = "hashicorp/archive" + version = "~> 2.7" + } } backend "s3" { diff --git a/terraform/app/modules/ecs_service/main.tf b/terraform/app/modules/ecs_service/main.tf index 92647cd96d..11e28fe252 100644 --- a/terraform/app/modules/ecs_service/main.tf +++ b/terraform/app/modules/ecs_service/main.tf @@ -57,6 +57,12 @@ resource "aws_ecs_service" "this" { container_port = var.loadbalancer.container_port } } + dynamic "service_registries" { + for_each = var.service_discovery_service_arn != null ? [1] : [] + content { + registry_arn = var.service_discovery_service_arn + } + } deployment_minimum_healthy_percent = 100 deployment_maximum_percent = 200 lifecycle { @@ -85,8 +91,8 @@ resource "aws_ecs_task_definition" "this" { readonlyRootFileSystem = true portMappings = [ { - containerPort = 4000 - hostPort = 4000 + containerPort = var.container_port + hostPort = var.host_port == null ? var.container_port : var.host_port } ] environment = concat(var.task_config.environment, [{ name = "SERVER_TYPE", value = var.server_type }]) @@ -109,3 +115,15 @@ resource "aws_ecs_task_definition" "this" { } ]) } + +resource "aws_ssm_parameter" "container_variables" { + name = "/${var.environment}/mavis/ecs/${local.server_type_name}/container_variables" + type = "String" + + value = jsonencode({ + task_envs = concat(var.task_config.environment, [{ name = "SERVER_TYPE", value = var.server_type }]) + task_secrets = var.task_config.secrets + execution_role_arn = var.task_config.execution_role_arn + task_role_arn = var.task_config.task_role_arn + }) +} diff --git a/terraform/app/modules/ecs_service/variables.tf b/terraform/app/modules/ecs_service/variables.tf index 785da0c6c3..1c12832a13 100644 --- a/terraform/app/modules/ecs_service/variables.tf +++ b/terraform/app/modules/ecs_service/variables.tf @@ -90,6 +90,13 @@ variable "network_params" { nullable = false } +variable "service_discovery_service_arn" { + type = string + description = "Arn of the Service Discovery service for the ECS service. If this is not set, the service will not be discoverable via Cloud Map." + default = null + nullable = true +} + variable "loadbalancer" { type = object({ target_group_arn = string @@ -114,6 +121,20 @@ variable "container_name" { nullable = false } +variable "container_port" { + type = number + description = "The port on the container that the service will bind to. If not specified, it defaults to 4000." + default = 4000 + nullable = false +} + +variable "host_port" { + type = number + description = "The port on the host that the container will bind to. If not specified, it defaults to the same value as container_port." + default = null + nullable = true +} + locals { autoscaling_enabled = var.maximum_replica_count > var.minimum_replica_count server_type_name = var.server_type_name != null ? var.server_type_name : var.server_type diff --git a/terraform/app/rds.tf b/terraform/app/rds.tf index 2f3bc7b0ba..4f970edea1 100644 --- a/terraform/app/rds.tf +++ b/terraform/app/rds.tf @@ -12,13 +12,13 @@ resource "aws_security_group" "rds_security_group" { } resource "aws_security_group_rule" "rds_ecs_ingress" { - count = length(local.ecs_sg_ids) + count = length(local.db_access_sg_ids) type = "ingress" from_port = 5432 to_port = 5432 protocol = "tcp" security_group_id = aws_security_group.rds_security_group.id - source_security_group_id = local.ecs_sg_ids[count.index] + source_security_group_id = local.db_access_sg_ids[count.index] lifecycle { create_before_destroy = true } diff --git a/terraform/app/resources/.gitignore b/terraform/app/resources/.gitignore new file mode 100644 index 0000000000..6f66c74b0e --- /dev/null +++ b/terraform/app/resources/.gitignore @@ -0,0 +1 @@ +*.zip \ No newline at end of file diff --git a/terraform/app/resources/rotate_secret.py b/terraform/app/resources/rotate_secret.py new file mode 100644 index 0000000000..b09393b16a --- /dev/null +++ b/terraform/app/resources/rotate_secret.py @@ -0,0 +1,94 @@ +import boto3 +import logging +import os +import json + +logger = logging.getLogger() +logger.setLevel(logging.INFO) + +def lambda_handler(event, context): + arn = event['SecretId'] + token = event['ClientRequestToken'] + step = event['Step'] + + service_client = boto3.client('secretsmanager') + + metadata = service_client.describe_secret(SecretId=arn) + if not metadata['RotationEnabled']: + logger.error(f"Secret {arn} is not enabled for rotation") + raise ValueError(f"Secret {arn} is not enabled for rotation") + + versions = metadata['VersionIdsToStages'] + if token not in versions: + logger.error(f"Secret version {token} has no stage for rotation of secret {arn}.") + raise ValueError(f"Secret version {token} has no stage for rotation of secret {arn}.") + + if "AWSCURRENT" in versions[token]: + logger.info(f"Secret version {token} already set as AWSCURRENT for secret {arn}.") + return + + if "AWSPENDING" not in versions[token]: + logger.error(f"Secret version {token} not set as AWSPENDING for rotation of secret {arn}.") + raise ValueError(f"Secret version {token} not set as AWSPENDING for rotation of secret {arn}.") + + if step == "createSecret": + create_secret(service_client, arn, token) + elif step == "setSecret": + set_secret(service_client, arn, token) + elif step == "testSecret": + test_secret(service_client, arn, token) + elif step == "finishSecret": + finish_secret(service_client, arn, token) + else: + logger.error(f"Invalid step parameter: {step}") + raise ValueError("Invalid step parameter") + +def create_secret(service_client, arn, token): + try: + # Check if AWSPENDING secret exists + service_client.get_secret_value(SecretId=arn, VersionId=token, VersionStage="AWSPENDING") + logger.info(f"createSecret: Secret already exists for {arn} version {token} as AWSPENDING.") + except service_client.exceptions.ResourceNotFoundException: + # Generate a 32-character hexadecimal secret (0-9, a-f) + logger.info(f"createSecret: Generating new 32-character hexadecimal secret for {arn}.") + new_secret = service_client.get_random_password( + PasswordLength=32, + ExcludeUppercase=True, + ExcludePunctuation=True, + IncludeSpace=False, + RequireEachIncludedType=False, + ExcludeCharacters='ghijklmnopqrstuvwxyz' + )['RandomPassword'] + service_client.put_secret_value( + SecretId=arn, + ClientRequestToken=token, + SecretString=new_secret, + VersionStages=['AWSPENDING'] + ) + logger.info(f"createSecret: Successfully put secret for ARN {arn} and version {token}.") + +def set_secret(service_client, arn, token): + # No external service to update for a generic secret + pass + +def test_secret(service_client, arn, token): + # No validation required for a generic secret + pass + +def finish_secret(service_client, arn, token): + metadata = service_client.describe_secret(SecretId=arn) + current_version = None + for version, stages in metadata["VersionIdsToStages"].items(): + if "AWSCURRENT" in stages: + if version == token: + logger.info(f"finishSecret: Version {version} already marked as AWSCURRENT for {arn}") + return + current_version = version + break + service_client.update_secret_version_stage( + SecretId=arn, + VersionStage="AWSCURRENT", + MoveToVersionId=token, + RemoveFromVersionId=current_version if current_version else token + ) + logger.info(f"finishSecret: Successfully set AWSCURRENT stage to version {token} for secret {arn}.") \ No newline at end of file diff --git a/terraform/app/ssm_parameters.tf b/terraform/app/ssm_parameters.tf index fcd4e83708..19000b325c 100644 --- a/terraform/app/ssm_parameters.tf +++ b/terraform/app/ssm_parameters.tf @@ -5,3 +5,94 @@ resource "aws_ssm_parameter" "environment_config" { value = each.value } + +resource "aws_secretsmanager_secret" "jwt_sign" { + name = "rep-jwt-signing-secret-${var.environment}" + description = "Secret for JSON signing key" + recovery_window_in_days = 7 + tags = { + Name = "json-signing-${var.environment}" + } +} + +resource "aws_secretsmanager_secret_rotation" "jwt_sign" { + secret_id = aws_secretsmanager_secret.jwt_sign.arn + rotate_immediately = true + rotation_lambda_arn = aws_lambda_function.rotate_jwt_sign.arn + rotation_rules { + schedule_expression = "cron(0 1 ? * MON *)" # Rotate every Monday at 01:00 UTC + duration = "1h" + } +} + +data "archive_file" "jwt_sign_lambda_zip" { + type = "zip" + source_file = "${path.module}/resources/rotate_secret.py" # Directory containing the Lambda function code + output_path = "${path.module}/resources/rotate_secret.zip" +} + +resource "aws_lambda_function" "rotate_jwt_sign" { + function_name = "rep-jwt-secret-rotation-${var.environment}" + handler = "rotate_secret.lambda_handler" + runtime = "python3.12" + role = aws_iam_role.jwt_rotate_lambda.arn + filename = data.archive_file.jwt_sign_lambda_zip.output_path + source_code_hash = data.archive_file.jwt_sign_lambda_zip.output_base64sha256 +} + +resource "aws_iam_role" "jwt_rotate_lambda" { + name = "secret-rotation-lambda-role" + + assume_role_policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Action = "sts:AssumeRole" + Effect = "Allow" + Principal = { + Service = "lambda.amazonaws.com" + } + } + ] + }) +} + +resource "aws_iam_role_policy" "jwt_rotate_lambda" { + name = "secret-rotation-lambda-policy" + role = aws_iam_role.jwt_rotate_lambda.id + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Effect = "Allow" + Action = [ + "secretsmanager:RotateSecret", + "secretsmanager:GetSecretValue", + "secretsmanager:PutSecretValue", + "secretsmanager:DescribeSecret", + "secretsmanager:UpdateSecretVersionStage", + ] + Resource = aws_secretsmanager_secret.jwt_sign.arn + }, + { + Effect = "Allow" + Action = [ + "logs:CreateLogGroup", + "logs:CreateLogStream", + "logs:PutLogEvents", + "secretsmanager:GetRandomPassword", + ] + Resource = "*" + } + ] + }) +} + +resource "aws_lambda_permission" "jwt_sign" { + statement_id = "AllowExecutionFromSecretsManager" + action = "lambda:InvokeFunction" + function_name = aws_lambda_function.rotate_jwt_sign.function_name + principal = "secretsmanager.amazonaws.com" + source_arn = aws_secretsmanager_secret.jwt_sign.arn +} \ No newline at end of file diff --git a/terraform/app/valkey.tf b/terraform/app/valkey.tf index e9afc3080e..b400cbf0ba 100644 --- a/terraform/app/valkey.tf +++ b/terraform/app/valkey.tf @@ -1,6 +1,6 @@ -resource "aws_security_group" "valkey" { +resource "aws_security_group" "reporting_valkey" { name = "mavis-cache-${var.environment}" - description = "Security group for Valkey ElastiCache (self-designed cluster)" + description = "Security group for Valkey ElastiCache for the reporting service" vpc_id = aws_vpc.application_vpc.id tags = { @@ -12,101 +12,33 @@ resource "aws_security_group" "valkey" { } } -resource "aws_security_group_rule" "valkey_ecs_services_ingress" { - count = length(local.ecs_sg_ids) +resource "aws_security_group_rule" "reporting_valkey_ingress" { type = "ingress" - from_port = var.valkey_port - to_port = var.valkey_port + from_port = aws_elasticache_serverless_cache.reporting_service.endpoint[0].port + to_port = aws_elasticache_serverless_cache.reporting_service.endpoint[0].port protocol = "tcp" - security_group_id = aws_security_group.valkey.id - source_security_group_id = local.ecs_sg_ids[count.index] + security_group_id = aws_security_group.reporting_valkey.id + source_security_group_id = module.reporting_service.security_group_id lifecycle { create_before_destroy = true } } -resource "aws_elasticache_subnet_group" "valkey" { - name = "mavis-cache-subnet-group-${var.environment}" - subnet_ids = [aws_subnet.private_subnet_a.id, aws_subnet.private_subnet_b.id] - - tags = { - Name = "mavis-cache-subnet-group-${var.environment}" - } -} - -resource "aws_elasticache_replication_group" "valkey" { - replication_group_id = "mavis-cache-${var.environment}" - description = "Valkey cluster for Sidekiq" - - engine = "valkey" - engine_version = var.valkey_engine_version - node_type = var.valkey_node_type - port = var.valkey_port - parameter_group_name = aws_elasticache_parameter_group.valkey.name - - automatic_failover_enabled = var.valkey_failover_enabled - num_cache_clusters = length(local.valkey_cache_availability_zones) - subnet_group_name = aws_elasticache_subnet_group.valkey.name - security_group_ids = [aws_security_group.valkey.id] - preferred_cache_cluster_azs = local.valkey_cache_availability_zones - snapshot_retention_limit = var.valkey_snapshot_retention_limit - snapshot_window = var.valkey_snapshot_window - maintenance_window = var.valkey_maintenance_window - - at_rest_encryption_enabled = true - transit_encryption_enabled = true - - log_delivery_configuration { - destination = aws_cloudwatch_log_group.valkey_slow_log.name - destination_type = "cloudwatch-logs" - log_format = "json" - log_type = "slow-log" - } - - log_delivery_configuration { - destination = aws_cloudwatch_log_group.valkey_engine_log.name - destination_type = "cloudwatch-logs" - log_format = "json" - log_type = "engine-log" - } - - tags = { - Name = "mavis-cache-${var.environment}" - Purpose = "sidekiq-job-processing" - } - apply_immediately = true -} - -resource "aws_elasticache_parameter_group" "valkey" { - family = "valkey8" - name = "mavis-cache-params-${var.environment}" - - # Optimize for Sidekiq workload - parameter { - name = "maxmemory-policy" - value = "noeviction" - } - - tags = { - Name = "mavis-cache-params-${var.environment}" - } -} - -resource "aws_cloudwatch_log_group" "valkey_slow_log" { - name = "/aws/elasticache/valkey/${var.environment}/slow-log" - retention_in_days = var.valkey_log_retention_days - - tags = { - Name = "mavis-cache-slow-log-${var.environment}" - } -} - -resource "aws_cloudwatch_log_group" "valkey_engine_log" { - name = "/aws/elasticache/valkey/${var.environment}/engine-log" - retention_in_days = var.valkey_log_retention_days - - tags = { - Name = "mavis-cache-engine-log-${var.environment}" - } +resource "aws_elasticache_serverless_cache" "reporting_service" { + engine = "valkey" + name = "mavis-reporting-${var.environment}" + cache_usage_limits { + data_storage { + maximum = 1 + unit = "GB" + } + ecpu_per_second { + maximum = 1000 + } + } + kms_key_id = aws_kms_key.reporting_valkey.arn + major_engine_version = "8" + security_group_ids = [aws_security_group.reporting_valkey.id] + subnet_ids = [aws_subnet.private_subnet_a.id, aws_subnet.private_subnet_b.id] } \ No newline at end of file diff --git a/terraform/app/variables.tf b/terraform/app/variables.tf index 4064844bcc..9f5c396c07 100644 --- a/terraform/app/variables.tf +++ b/terraform/app/variables.tf @@ -133,6 +133,13 @@ variable "image_digest" { nullable = false } +variable "reporting_digest" { + type = string + description = "The docker image digest for the reporting container in the task definition." + default = null + nullable = true +} + variable "enable_cis2" { type = bool default = true @@ -257,6 +264,11 @@ locals { valueFrom = var.rails_master_key_path } ], local.parameter_store_config_list) + container_ports = { + web = 4000 + good_job = 4000 + reporting = 5000 + } } ########## RDS configuration ########## @@ -330,6 +342,18 @@ variable "sidekiq_replicas" { description = "Amount of replicas for the sidekiq service" } +variable "minimum_reporting_replicas" { + type = number + default = 2 + description = "Minimum amount of allowed replicas for reporting service. Also the replica count when creating the service." +} + +variable "maximum_reporting_replicas" { + type = number + default = 4 + description = "Maximum amount of allowed replicas for reporting service" +} + variable "max_aurora_capacity_units" { type = number default = 8 @@ -421,3 +445,16 @@ locals { ecs_sg_ids = [module.web_service.security_group_id, module.good_job_service.security_group_id, module.sidekiq_service.security_group_id] valkey_cache_availability_zones = var.valkey_failover_enabled ? [aws_subnet.private_subnet_a.availability_zone, aws_subnet.private_subnet_b.availability_zone] : [aws_subnet.private_subnet_a.availability_zone] } + +variable "reporting_endpoints" { + type = list(string) + description = "List of endpoints for the loadbalancer to forward to the reporting service" + default = ["/reporting", "/reporting/*"] + nullable = false +} + +locals { + ecs_initial_lb_target_group = var.active_lb_target_group == "green" ? aws_lb_target_group.green.arn : aws_lb_target_group.blue.arn + reporting_initial_lb_target_group = var.active_lb_target_group == "green" ? aws_lb_target_group.reporting_green.arn : aws_lb_target_group.reporting_blue.arn + db_access_sg_ids = [module.web_service.security_group_id, module.good_job_service.security_group_id] +} From 7891b68092b50f9a2cd5968c00af8c030644d4bf Mon Sep 17 00:00:00 2001 From: Alistair Davidson Date: Wed, 13 Aug 2025 14:57:32 +0100 Subject: [PATCH 5/6] use new ReportingAPI:: scope for TokenAuthConcern, and spec fix to implement the new session cis2_info structure after the org -> team refactor remove redundant duplicated ensure_reporting_api_feature_enabled correct repo in adoc tidy up after rebase --- .../reporting/one_time_tokens_controller.rb | 3 - .../concerns/authentication_concern.rb | 18 -- .../concerns/token_authentication_concern.rb | 70 ----- .../users/omniauth_callbacks_controller.rb | 1 - app/controllers/users/sessions_controller.rb | 2 +- app/models/user.rb | 4 - app/views/users/sessions/new.html.erb | 4 - docs/architecture.adoc | 2 - .../user-visits-the-reporting-app.puml | 2 +- docs/ops-tasks.md | 2 +- docs/reporting-component-authentication.adoc | 2 +- .../api/reporting/totals_controller_spec.rb | 5 +- .../token_authentication_concern_spec.rb | 281 ------------------ .../user_password_authentication_spec.rb | 2 - 14 files changed, 7 insertions(+), 391 deletions(-) delete mode 100644 app/controllers/concerns/token_authentication_concern.rb delete mode 100644 spec/controllers/concerns/token_authentication_concern_spec.rb diff --git a/app/controllers/api/reporting/one_time_tokens_controller.rb b/app/controllers/api/reporting/one_time_tokens_controller.rb index ef5d199f05..b1a6b706f8 100644 --- a/app/controllers/api/reporting/one_time_tokens_controller.rb +++ b/app/controllers/api/reporting/one_time_tokens_controller.rb @@ -42,7 +42,4 @@ def jwt(token) ) end - def ensure_reporting_api_feature_enabled - render status: :forbidden and return unless Flipper.enabled?(:reporting_api) - end end diff --git a/app/controllers/concerns/authentication_concern.rb b/app/controllers/concerns/authentication_concern.rb index 8b6a46bd23..ffff28bc18 100644 --- a/app/controllers/concerns/authentication_concern.rb +++ b/app/controllers/concerns/authentication_concern.rb @@ -105,24 +105,6 @@ def authenticate_basic end end - def add_auth_code_to(url, user) - uri = Addressable::URI.parse(url) - auth_code = - ReportingAPI::OneTimeToken.find_or_generate_for!( - user:, - cis2_info: session["cis2_info"] - ).token - uri.query_values = (uri.query_values || {}).merge("code" => auth_code) - uri.to_s - end - - def reporting_app_redirect_uri_with_auth_code_for(user) - if Flipper.enabled?(:reporting_api) - url = session["redirect_uri"] - url.present? ? add_auth_code_to(url, user) : nil - end - end - def after_sign_in_path_for(scope) urls = [] if Flipper.enabled?(:reporting_api) diff --git a/app/controllers/concerns/token_authentication_concern.rb b/app/controllers/concerns/token_authentication_concern.rb deleted file mode 100644 index 4d677ab9e1..0000000000 --- a/app/controllers/concerns/token_authentication_concern.rb +++ /dev/null @@ -1,70 +0,0 @@ -# frozen_string_literal: true - -module TokenAuthenticationConcern - extend ActiveSupport::Concern - - included do - private - - def client_id_error!(token) - if token.blank? - render json: { errors: "invalid_request" }, status: :unauthorized - else - render json: { errors: "unauthorized_client" }, status: :forbidden - end - end - - def authenticate_app_by_client_id! - if Flipper.enabled?(:reporting_api) - # ...as per the spec at https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.3 - given_client_id = params.fetch("client_id", nil) - unless given_client_id == Settings.reporting_api.client_app.client_id - client_id_error!(given_client_id) - end - end - end - - def jwt_if_given - params[:jwt] || - request.headers["Authorization"]&.gsub(/(Bearer\s+)?([:alnum:]*)/, '\2') - end - - def authenticate_user_by_jwt! - jwt = jwt_if_given - jwt_info = decode_jwt!(jwt) - if jwt_info - data = jwt_info.first["data"] - @current_user = - User.find_by( - id: data.dig("user", "id"), - session_token: data.dig("user", "session_token"), - reporting_api_session_token: - data.dig("user", "reporting_api_session_token") - ) - if @current_user - session["user"] = data["user"] - session["cis2_info"] = data["cis2_info"] - authenticate_user! - else - session.clear - client_id_error!(jwt) - Rails.logger.warn "Couldn't find user id #{data.dig("user", "id")} with tokens" - end - end - rescue JWT::DecodeError, NoMethodError - Rails.logger.warn "invalid JWT" - client_id_error!(jwt) - end - end - - def decode_jwt!(jwt) - if jwt - JWT.decode( - jwt, - Settings.reporting_api.client_app.secret, - true, - { algorithm: "HS512" } - ) - end - end -end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 64644447ed..64e597c523 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -77,7 +77,6 @@ def logout signed_out = (Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name)) flash[:notice] = "You have been logged out" if signed_out - redirect_to after_sign_out_path_for(resource_name) end diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 24385445ba..61cf7e5143 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -6,7 +6,7 @@ class Users::SessionsController < Devise::SessionsController skip_after_action :verify_policy_scoped before_action :store_redirect_uri!, only: :new - + layout "one_half" def create diff --git a/app/models/user.rb b/app/models/user.rb index afc5ddbbb7..75dad8c76e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -73,7 +73,6 @@ class User < ApplicationRecord -> { where(last_sign_in_at: 1.week.ago..Time.current) } enum :fallback_role, -<<<<<<< HEAD { nurse: 0, admin: 1, @@ -81,9 +80,6 @@ class User < ApplicationRecord healthcare_assistant: 3, prescriber: 4 }, -======= - { nurse: 0, admin: 1, superuser: 2, healthcare_assistant: 3 }, ->>>>>>> 02ac86118 (Allow fallback_role to be nil) prefix: true, validate: { allow_nil: true diff --git a/app/views/users/sessions/new.html.erb b/app/views/users/sessions/new.html.erb index a771dcffa3..d9a96febbc 100644 --- a/app/views/users/sessions/new.html.erb +++ b/app/views/users/sessions/new.html.erb @@ -10,10 +10,6 @@ <%= hidden_field_tag :redirect_uri, params.fetch(:redirect_uri, "") %> <% end %> - <% if params.has_key?(:redirect_uri) %> - <%= hidden_field_tag :redirect_uri, params.fetch(:redirect_uri, "") %> - <% end %> - <% if devise_mapping.rememberable? %> <%= f.govuk_check_boxes_fieldset :remember_me, multiple: false, legend: nil do %> <%= f.govuk_check_box :remember_me, 1, 0, multiple: false, link_errors: true, small: true, label: { text: "Remember me?" } %> diff --git a/docs/architecture.adoc b/docs/architecture.adoc index 2054f20b58..25b88e410c 100644 --- a/docs/architecture.adoc +++ b/docs/architecture.adoc @@ -202,5 +202,3 @@ concerns on the server. * Offline support - Browser-based component * FHIR server synchronisation * Reporting (in development) - - diff --git a/docs/diagrams/reporting_auth/user-visits-the-reporting-app.puml b/docs/diagrams/reporting_auth/user-visits-the-reporting-app.puml index 04aafc3497..7866a6d58f 100644 --- a/docs/diagrams/reporting_auth/user-visits-the-reporting-app.puml +++ b/docs/diagrams/reporting_auth/user-visits-the-reporting-app.puml @@ -46,4 +46,4 @@ endif :process response; stop -@enduml +@enduml \ No newline at end of file diff --git a/docs/ops-tasks.md b/docs/ops-tasks.md index 758f648557..b7add36acf 100644 --- a/docs/ops-tasks.md +++ b/docs/ops-tasks.md @@ -19,7 +19,7 @@ session.patient_sessions.all?(&:safe_to_destroy?) session.patients.update_all( cohort_id: nil, home_educated: false, - school_id: nil + school_id: nil, ) # removes all patients from the session diff --git a/docs/reporting-component-authentication.adoc b/docs/reporting-component-authentication.adoc index 8018c68d37..8b9c850569 100644 --- a/docs/reporting-component-authentication.adoc +++ b/docs/reporting-component-authentication.adoc @@ -12,7 +12,7 @@ ifdef::env-github[] // to change it back to main before merging!! :github-branch: spike/MAV-1406-auth-sharing-with-reporting -:github-repo: nhsuk/record-childrens-vaccinations +:github-repo: nhsuk/manage-vaccinations-in-schools // URL for PlantUML Proxy. Using an attribute mainly because it's just tidier. :plantuml-proxy-url: http://www.plantuml.com/plantuml/proxy?cache=no&src= diff --git a/spec/controllers/api/reporting/totals_controller_spec.rb b/spec/controllers/api/reporting/totals_controller_spec.rb index b654f92219..d96b44aeae 100644 --- a/spec/controllers/api/reporting/totals_controller_spec.rb +++ b/spec/controllers/api/reporting/totals_controller_spec.rb @@ -11,9 +11,10 @@ data: { user: user.as_json, cis2_info: { + organisation_name: team.name, organisation_code: team.organisation.ods_code, - workgroups: [team.workgroup], - role_code: CIS2Info::NURSE_ROLE + role_code: "S8000:G8000:R8001", + workgroups: ["schoolagedimmunisations"], } } } diff --git a/spec/controllers/concerns/token_authentication_concern_spec.rb b/spec/controllers/concerns/token_authentication_concern_spec.rb deleted file mode 100644 index cdfd7a792b..0000000000 --- a/spec/controllers/concerns/token_authentication_concern_spec.rb +++ /dev/null @@ -1,281 +0,0 @@ -# frozen_string_literal: true - -describe TokenAuthenticationConcern do - let(:user) { @user = build(:user) } - let(:mock_request) { instance_double(request.class, headers: {}) } - let(:sample_class) do - Class - .new do # rubocop:disable Style/BlockDelimiters - include TokenAuthenticationConcern - attr_accessor :request, :session - - def authenticate_user! - end - - def initialize(request: nil, session: {}) - @request = request - @session = session - end - - def params - {} - end - - def render(content = {}, args = {}) - end - - def current_user - @user - end - end # rubocop:disable Style/MethodCalledOnDoEndBlock - .new(request: mock_request) - end - - describe "#jwt_if_given" do - context "when there is a jwt param" do - before do - allow(sample_class).to receive(:params).and_return({ jwt: "myjwt" }) - end - - it "returns the jwt param" do - expect(sample_class.send(:jwt_if_given)).to eq("myjwt") - end - end - - context "when there is no jwt param" do - context "but there is an Authorization header" do - before do - sample_class.request = - instance_double( - request.class, - headers: { - "Authorization" => "Bearer myjwt" - } - ) - end - - it "returns the value of the Authorization header, without the leading 'Bearer'" do - result = sample_class.send(:jwt_if_given) - expect(result).to eq("myjwt") - end - end - - context "and there is no Authorization header" do - it "returns nil" do - expect(sample_class.send(:jwt_if_given)).to be_nil - end - end - end - end - - describe "#authenticate_app_by_client_id!" do - let(:client_id) { "something" } - - context "when the :reporting_api feature flag is enabled" do - before { Flipper.enable(:reporting_api) } - - context "and the client_id param is provided" do - before do - allow(sample_class).to receive(:params).and_return( - { client_id: client_id }.with_indifferent_access - ) - end - - context "and the client_id param contains the reporting app's client_id" do - let(:client_id) { Settings.reporting_api.client_app.client_id } - - it "does not cause a token error" do - expect(sample_class).not_to receive(:client_id_error!) - sample_class.send(:authenticate_app_by_client_id!) - end - end - - context "and the client_id param does not contain the reporting app client_id" do - it "causes a token error" do - expect(sample_class).to receive(:client_id_error!) - sample_class.send(:authenticate_app_by_client_id!) - end - end - end - end - end - - describe "#client_id_error!" do - context "given an empty token" do - let(:token) { "" } - - it "renders a invalid_request error, with status :unauthorized" do - expect(sample_class).to receive(:render).with( - json: { - errors: "invalid_request" - }, - status: :unauthorized - ) - sample_class.send(:client_id_error!, token) - end - end - - context "given a token that is not empty, but does not match the reporting app's client_id" do - let(:token) { "unmatched token" } - - it "renders a unauthorized_client error, with status forbidden" do - expect(sample_class).to receive(:render).with( - json: { - errors: "unauthorized_client" - }, - status: :forbidden - ) - sample_class.send(:client_id_error!, token) - end - end - end - - describe "#authenticate_user_by_jwt!" do - let(:jwt) { "" } - let(:user_id) { 0 } - let(:session_token) { "123456abcdef" } - let(:reporting_api_session_token) { "0987654321123456abcdef" } - - let(:user) do - create( - :user, - session_token: session_token, - reporting_api_session_token: reporting_api_session_token - ) - end - - before do - sample_class.request = - instance_double(request.class, headers: { "Authorization" => jwt }) - end - - context "when a valid jwt is given" do - let(:jwt) { "validjwt" } - let(:user_info) do - [ - { - "data" => { - "user" => { - "id" => user_id, - "session_token" => session_token, - "reporting_api_session_token" => reporting_api_session_token - }, - "cis2_info" => { - "some_key" => "some value" - } - } - } - ] - end - - before do - allow(sample_class).to receive(:decode_jwt!).with(jwt).and_return( - user_info - ) - allow(sample_class).to receive(:authenticate_user!) - end - - it "decodes the JWT" do - expect(sample_class).to receive(:decode_jwt!).with(jwt) - sample_class.send(:authenticate_user_by_jwt!) - end - - context "when a User exists with the values of id, session_token and reporting_api_session_token" do - let(:user_id) { user.id } - - it "copies the user key into session['user']" do - sample_class.send(:authenticate_user_by_jwt!) - expect(sample_class.session["user"]).to eq( - user_info.first["data"]["user"] - ) - end - - it "copies the cis2_info key into session['cis2_info']" do - sample_class.send(:authenticate_user_by_jwt!) - expect(sample_class.session["cis2_info"]).to eq( - user_info.first["data"]["cis2_info"] - ) - end - - it "calls authenticate_user!" do - expect(sample_class).to receive(:authenticate_user!) - sample_class.send(:authenticate_user_by_jwt!) - end - end - - context "when a User does not exist with the values of id, session_token and reporting_api_session_token" do - let(:user_id) { user.id } - - before do - user.update!( - session_token: "someothersessiontoken", - reporting_api_session_token: "someotherpwdauthsessiontoken" - ) - sample_class.session = { - user_id: user.id, - some_other_session_var: "some value" - } - end - - it "clears the session" do - sample_class.send(:authenticate_user_by_jwt!) - expect(sample_class.session).to be_empty - end - - it "calls client_id_error!" do - expect(sample_class).to receive(:client_id_error!) - sample_class.send(:authenticate_user_by_jwt!) - end - end - end - - context "when a valid jwt is not given" do - it "causes a client_id_error!" do - expect(sample_class).to receive(:client_id_error!) - sample_class.send(:authenticate_user_by_jwt!) - end - end - end - - describe "decode_jwt!" do - context "given a jwt" do - let(:jwt) { "somejwt" } - let(:decoded_jwt) do - { "some_key" => { "some nested_key" => "some nested value" } } - end - - it "tries to decode it with the mavis reporting app secret" do - expect(JWT).to receive(:decode).with( - jwt, - Settings.reporting_api.client_app.secret, - true, - { algorithm: "HS512" } - ) #.and_return(decoded_jwt) - sample_class.send(:decode_jwt!, jwt) - end - - context "when decoding works" do - before do - allow(JWT).to receive(:decode).with( - jwt, - Settings.reporting_api.client_app.secret, - true, - { algorithm: "HS512" } - ).and_return(decoded_jwt) - end - - it "returns the decoded JWT" do - expect(sample_class.send(:decode_jwt!, jwt)).to eq(decoded_jwt) - end - end - - context "when decoding does not work" do - it "raises an exception" do - expect { sample_class.send(:decode_jwt!, jwt) }.to raise_error( - JWT::DecodeError - ) - end - end - end - end -end diff --git a/spec/features/user_password_authentication_spec.rb b/spec/features/user_password_authentication_spec.rb index 1b8337e130..7ac633ab62 100644 --- a/spec/features/user_password_authentication_spec.rb +++ b/spec/features/user_password_authentication_spec.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true describe "User password authentication", :local_users do - # include RedirectHelper - before { given_the_cis2_feature_flag_is_disabled } scenario "going through the start page then signing out" do From c76404f586271023164af8a7cc33eb10ed93efad Mon Sep 17 00:00:00 2001 From: Alistair Davidson Date: Fri, 22 Aug 2025 16:10:56 +0100 Subject: [PATCH 6/6] spec fix after rebase --- spec/controllers/api/reporting/totals_controller_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/controllers/api/reporting/totals_controller_spec.rb b/spec/controllers/api/reporting/totals_controller_spec.rb index d96b44aeae..b654f92219 100644 --- a/spec/controllers/api/reporting/totals_controller_spec.rb +++ b/spec/controllers/api/reporting/totals_controller_spec.rb @@ -11,10 +11,9 @@ data: { user: user.as_json, cis2_info: { - organisation_name: team.name, organisation_code: team.organisation.ods_code, - role_code: "S8000:G8000:R8001", - workgroups: ["schoolagedimmunisations"], + workgroups: [team.workgroup], + role_code: CIS2Info::NURSE_ROLE } } }