Add OIDC strategy for Identity

The Identity service is an OpenID Connect provider. Changing the strategy we use with OmniAuth can make this more obvious. We currently have implemented the OAuth2 strategy for Identity and extended it to take advantage of the OIDC logout feature. Switching to an OIDC strategy makes it more obvious what features the Identity service provides. It also, hopefully, makes things clearer for any future maintainer of the code.
DFE-Digital · Jul 13, 2023 · da0e478 · da0e478
1 parent 73a8ea8
commit da0e478
Show file tree

Hide file tree

Showing 11 changed files with 82 additions and 147 deletions.
diff --git a/.env.development b/.env.development
@@ -8,7 +8,7 @@ GOVUK_NOTIFY_API_KEY=override-locally
 HOSTING_DOMAIN=http://localhost
 HOSTING_ENVIRONMENT=local
 IDENTITY_API_DOMAIN=override-locally
-IDENTITY_CLIENT_ID=override-locally
+IDENTITY_CLIENT_ID=access-your-teaching-certificates
 IDENTITY_CLIENT_SECRET=override-locally
 QUALIFICATIONS_API_URL=https://preprod-teacher-qualifications-api.education.gov.uk/
 QUALIFICATIONS_API_FIXED_TOKEN=override-locally

diff --git a/.env.test b/.env.test
@@ -1,11 +1,15 @@
-GOVUK_NOTIFY_API_KEY=override-locally
 CHECK_RECORDS_DOMAIN=http://check_records.localhost
+DFE_SIGN_IN_CLIENT_ID=checkrecordteacher
+DFE_SIGN_IN_ISSUER=test
+DFE_SIGN_IN_REDIRECT_URL=test
+DFE_SIGN_IN_SECRET=override-locally
+GOVUK_NOTIFY_API_KEY=override-locally
 HOSTING_DOMAIN=http://qualifications.localhost
 HOSTING_ENVIRONMENT=local
 IDENTITY_API_DOMAIN=override-locally
 IDENTITY_CLIENT_ID=override-locally
 IDENTITY_CLIENT_SECRET=override-locally
-QUALIFICATIONS_API_URL=https://preprod-teacher-qualifications-api.education.gov.uk/
 QUALIFICATIONS_API_FIXED_TOKEN=token
+QUALIFICATIONS_API_URL=https://preprod-teacher-qualifications-api.education.gov.uk/
 SUPPORT_PASSWORD=test
 SUPPORT_USERNAME=test
diff --git a/app/controllers/qualifications/users/omniauth_callbacks_controller.rb b/app/controllers/qualifications/users/omniauth_callbacks_controller.rb
@@ -8,7 +8,7 @@ def identity
         @user = User.from_identity(auth)
         session[:identity_user_id] = @user.id
         session[:identity_user_token] = auth.credentials.token
-        session[:identity_user_token_expiry] = auth.credentials.expires_at
+        session[:identity_user_token_expiry] = auth.credentials.expires_in.seconds.from_now.to_i
 
         log_auth_credentials_in_development(auth)
         redirect_to qualifications_dashboard_path
@@ -19,7 +19,7 @@ def identity
       def log_auth_credentials_in_development(auth)
         if Rails.env.development?
           Rails.logger.debug auth.credentials.token
-          Rails.logger.debug Time.zone.at auth.credentials.expires_at
+          Rails.logger.debug Time.zone.at auth.credentials.expires_in
         end
       end
     end

diff --git a/app/models/user.rb b/app/models/user.rb
@@ -6,11 +6,11 @@ def self.from_identity(auth_data)
     email = auth_data.info.email
     user = find_or_initialize_by(email:)
     user.assign_attributes(
-      date_of_birth: auth_data.info.date_of_birth,
-      family_name: auth_data.info.family_name,
-      given_name: auth_data.info.given_name,
+      date_of_birth: auth_data.extra.raw_info.birthdate,
+      family_name: auth_data.info.last_name,
+      given_name: auth_data.info.first_name,
       name: auth_data.info.name,
-      trn: auth_data.info.trn
+      trn: auth_data.extra.raw_info.trn
     )
     user.tap(&:save!)
   end

diff --git a/app/views/qualifications/qualifications/show.html.erb b/app/views/qualifications/qualifications/show.html.erb
@@ -23,7 +23,7 @@
 
         <p>
           Date of birth<br /> 
-          <strong><%= @user.date_of_birth.to_fs(:long_uk) %></strong>  
+          <strong><%= @user.date_of_birth&.to_fs(:long_uk) %></strong>  
         </p>
 
         <p>

diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb
@@ -1,32 +1,9 @@
 require "check_records/dfe_sign_in"
-require "omniauth/strategies/identity"
 
 OmniAuth.config.logger = Rails.logger
-OmniAuth.config.on_failure = proc { |env| AuthFailuresController.action(:failure).call(env) }
+OmniAuth.config.on_failure =
+  proc { |env| AuthFailuresController.action(:failure).call(env) }
 
-# DSI setup
-dfe_sign_in_identifier = ENV["DFE_SIGN_IN_CLIENT_ID"]
-dfe_sign_in_secret = ENV["DFE_SIGN_IN_SECRET"]
-dfe_sign_in_redirect_uri = ENV["DFE_SIGN_IN_REDIRECT_URL"]
-dfe_sign_in_issuer_uri = ENV["DFE_SIGN_IN_ISSUER"].present? ? URI(ENV["DFE_SIGN_IN_ISSUER"]) : nil
-options = {
-  name: :dfe,
-  discovery: true,
-  response_type: :code,
-  scope: %i[email profile organisation],
-  path_prefix: "/check-records/auth",
-  callback_path: "/check-records/auth/dfe/callback",
-  client_options: {
-    port: dfe_sign_in_issuer_uri&.port,
-    scheme: dfe_sign_in_issuer_uri&.scheme,
-    host: dfe_sign_in_issuer_uri&.host,
-    identifier: dfe_sign_in_identifier,
-    secret: dfe_sign_in_secret,
-    redirect_uri: dfe_sign_in_redirect_uri&.to_s
-  },
-  issuer:
-    ("#{dfe_sign_in_issuer_uri}:#{dfe_sign_in_issuer_uri.port}" if dfe_sign_in_issuer_uri.present?)
-}
 if CheckRecords::DfESignIn.bypass?
   Rails.application.config.middleware.use OmniAuth::Builder do
     provider :developer,
@@ -35,13 +12,47 @@
              path_prefix: "/check-records/auth"
   end
 else
-  Rails.application.config.middleware.use OmniAuth::Strategies::OpenIDConnect, options
+  dfe_sign_in_issuer_uri = URI(ENV["DFE_SIGN_IN_ISSUER"])
+
+  Rails.application.config.middleware.use OmniAuth::Builder do
+    provider :openid_connect,
+             name: :dfe,
+             callback_path: "/check-records/auth/dfe/callback",
+             client_options: {
+               host: dfe_sign_in_issuer_uri&.host,
+               identifier: ENV["DFE_SIGN_IN_CLIENT_ID"],
+               port: dfe_sign_in_issuer_uri&.port,
+               redirect_uri: ENV["DFE_SIGN_IN_REDIRECT_URL"],
+               scheme: dfe_sign_in_issuer_uri&.scheme,
+               secret: ENV.fetch("DFE_SIGN_IN_SECRET", "example")
+             },
+             discovery: true,
+             issuer: "#{dfe_sign_in_issuer_uri}:#{dfe_sign_in_issuer_uri.port}",
+             path_prefix: "/check-records/auth",
+             response_type: :code,
+             scope: %i[email organisation profile]
+  end
 end
 
-# Identity setup
 Rails.application.config.middleware.use OmniAuth::Builder do
-  provider :identity,
-           ENV["IDENTITY_CLIENT_ID"],
-           ENV["IDENTITY_CLIENT_SECRET"],
-           { path_prefix: "/qualifications/users/auth" }
+  provider :openid_connect,
+           name: :identity,
+           callback_path: "/qualifications/users/auth/identity/callback",
+           client_options: {
+             host: URI(ENV["IDENTITY_API_DOMAIN"]).host,
+             identifier: ENV["IDENTITY_CLIENT_ID"],
+             port: 443,
+             redirect_uri:
+               "#{ENV["HOSTING_DOMAIN"]}/qualifications/users/auth/identity/callback",
+             scheme: "https",
+             secret: ENV["IDENTITY_CLIENT_SECRET"]
+           },
+           discovery: true,
+           issuer: ENV["IDENTITY_API_DOMAIN"],
+           path_prefix: "/qualifications/users/auth",
+           pkce: true,
+           post_logout_redirect_uri:
+             "#{ENV["HOSTING_DOMAIN"]}/qualifications/sign-out",
+           response_type: :code,
+           scope: %w[email openid profile dqt:read]
 end
diff --git a/lib/omniauth/strategies/identity.rb b/lib/omniauth/strategies/identity.rb
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
@@ -8,11 +8,10 @@
           OpenStruct.new(
             email: "[email protected]",
             name: "Test User",
-            given_name: "Test",
-            family_name: "User",
-            trn: "123456",
-            date_of_birth: "1986-01-02"
-          )
+            first_name: "Test",
+            last_name: "User"
+          ),
+        extra: OpenStruct.new(raw_info: OpenStruct.new(birthdate: "1986-01-02", trn: "123456"))
       )
     end
 

diff --git a/spec/support/system/qualifications_authentication_steps.rb b/spec/support/system/qualifications_authentication_steps.rb
@@ -15,16 +15,20 @@ def given_identity_auth_is_mocked
       {
         provider: "identity",
         info: {
-          date_of_birth: "1986-01-02",
           email: "[email protected]",
-          family_name: "User",
-          given_name: "Test",
-          name: "Test User",
-          trn: "123456"
+          first_name: "User",
+          last_name: "Test",
+          name: "Test User"
         },
         credentials: {
           token: "token",
-          expires_at: (Time.zone.now + 1.hour).to_i
+          expires_in: 1.hour.to_i
+        },
+        extra: {
+          raw_info: {
+            birthdate: "1986-01-02",
+            trn: "123456"
+          }
         }
       }
     )

diff --git a/spec/system/qualifications/user_token_expires_while_viewing_qualifications_spec.rb b/spec/system/qualifications/user_token_expires_while_viewing_qualifications_spec.rb
@@ -25,7 +25,7 @@ def when_i_visit_the_qualifications_page
   end
 
   def and_my_access_token_expires
-    travel_to Time.zone.now + 61.minutes
+    travel_to 61.minutes.from_now
   end
 
   def and_i_try_to_download_a_certificate

diff --git a/spec/system/qualifications/user_views_their_qualifications_no_itt_spec.rb b/spec/system/qualifications/user_views_their_qualifications_no_itt_spec.rb
@@ -19,16 +19,21 @@ def given_identity_auth_is_mocked
       {
         provider: "identity",
         info: {
-          date_of_birth: "1986-01-02",
           email: "[email protected]",
-          family_name: "Trained",
-          given_name: "Not",
-          name: "Not Trained",
-          trn: "0000000"
+          email_verified: "True",
+          first_name: "Trained",
+          last_name: "Not",
+          name: "Not Trained"
         },
         credentials: {
           token: "no-itt-token",
-          expires_at: (Time.zone.now + 1.hour).to_i
+          expires_in: 1.hour.to_i
+        },
+        extra: {
+          raw_info: {
+            birthdate: "1986-01-02",
+            trn: "0000000"
+          }
         }
       }
     )