[NPQ-3490] - store long lived auth token by spqr-praetoria · Pull Request #3317 · DFE-Digital/npq-registration

spqr-praetoria · 2026-05-05T12:16:58Z

Context

Ticket: https://dfedigital.atlassian.net/browse/NPQ-3490

Changes proposed in this pull request

github-actions · 2026-05-05T12:27:45Z

Review app deployed to https://npq-registration-review-3317-web.test.teacherservices.cloud/

github-actions · 2026-05-19T10:42:19Z

Review app for PR 3317 was deleted

jebw

Looks good - I've a few comments on changes - can you review them then ping me when you're ready for a catch up, thanks

jebw · 2026-05-19T12:16:12Z

-    let(:older_statement_output_fee_true) { create(:statement, cohort: older_cohort, lead_provider:, output_fee: true, for_date: 3.months.ago) }
-    let(:older_statement_output_fee_false) { create(:statement, cohort: older_cohort, lead_provider:, output_fee: false, for_date: 2.months.ago) }
-    let(:latest_statement_output_fee_true) { create(:statement, cohort: older_cohort, lead_provider:, output_fee: true, for_date: 1.month.ago) }
-    let(:latest_statement_output_fee_false) { create(:statement, cohort: older_cohort, lead_provider:, output_fee: false, for_date: 0.months.ago) }
-    let(:future_statement_output_fee_true) { create(:statement, cohort: older_cohort, lead_provider:, output_fee: true, for_date: 1.month.from_now) }


This shouldn't be here - this reverts Alkesh' recent fix for a flaky spec

Will update

jebw · 2026-05-19T12:19:04Z

+      it "does not enqueue a RefreshUserTokenJob" do
+        expect { subject }.not_to have_enqueued_job(RefreshUserTokenJob)
+      end
+    end


Can you add a spec here to check sentry was alerted that we didn't get a refresh token back for some reason

Think you'll also need to change it to alert sentry

jebw · 2026-05-19T12:23:51Z

+
+      new_refresh_token_assigned = false
+      user = nil
+      Rails.logger.info("[HELLO4] Create user with provider data")


Removed leftover logging

jebw · 2026-05-19T12:26:11Z

+        headers: { "Content-Type" => "application/x-www-form-urlencoded" },
+      )
+
+      response.parsed_response["refresh_token"] || @refresh_token


This makes no sense - if the API returned a success resposnse, then @refresh_token is now used up and cannot be resused again so returning it will lead to the application incorrectly updating with an expired token

jebw · 2026-05-19T12:27:55Z

-      application.update!(lead_provider_approval_status: "rejected", reason_for_rejection:)
+      ApplicationRecord.transaction do
+        application.update!(lead_provider_approval_status: "rejected", reason_for_rejection:)
+        clear_refresh_token_if_orphaned


Orphaned isn't a term we're using so may lead to confusion for other developers - how about clear_refresh_token_if_no_remaining_applications!

jebw · 2026-05-19T12:42:52Z

+  class RefreshAccessToken
+    include HTTParty
+
+    default_timeout 5.seconds


Given this will be getting called from a Job I think we can give this longer to respond - say 15.seconds

jebw · 2026-05-19T12:44:40Z

+          new_refresh_token_assigned = persist_refresh_token(user_matched_using_trn, provider_data)
+        end
+
+        schedule_refresh(user_matched_using_trn) if new_refresh_token_assigned


This shouldn't be needed if we're using an hourly job

jebw · 2026-05-19T12:45:55Z

+    def schedule_refresh(user)
+      RefreshUserTokenJob.set(wait: 7.days).perform_later(user.id)
+    end


Cron job supercedes this

jebw · 2026-05-19T12:58:05Z

@@ -3,6 +3,7 @@
 module Users
  class FindOrCreateFromTeacherAuth


The call method in this class are getting very complicated and its because there are multiple places all trying to do the same 'after sorting user out' action of persisting the token.

I can see 2 approaches

Move the persist the token out to the omniauth controller (with some helper code on the token class) - this logic is orthogonal to signing the user in.

@user = Users::FindOrCreateFromTeacherAuth if @user @user.set_refresh_token(provider_data.credentials&.fetch(:token)) if @user.trn.blank? # .... end

Or alternatively, add a level of indirection within this class - move the existing #call method to be a differently named private method, eg sign_in_user or similar since that always returns a user class. Then add a new #call method with something like the following

def call sign_in_user(...).tap do |user| set_refresh_token_on_user(user) if user && user.trn.blank? end end

jebw · 2026-05-19T12:59:04Z

+    create_table :user_refresh_tokens do |t|
+      t.references :user, null: false, foreign_key: true, index: { unique: true }
+      t.text :refresh_token, null: false
+      t.datetime :refresh_token_updated_at, null: false
+      t.timestamps
+    end


Could do with a chat around the schema on this

Meant to comment not approve

jebw

See responses in earlier approval (I'd meant to just comment rather than approve so I've dismissed that one)

jebw · 2026-05-19T13:26:05Z

+    token_record.update!(refresh_token: new_refresh_token, refresh_token_updated_at: Time.current)
+  rescue StandardError => e
+    Sentry.capture_exception(e, extra: { user_id: })
+    raise


This shouldn't re-raise otherwise you'll end up with a thundering herd problem when you chain jobs

9am - Enqueue job kicks of 1000 api jobs

they fail because endpoint is unavailable

those 1000 jobs reschedule themselves

and keep rescheduling them selves with increasing backoff

meanwhile at 10am - Enqueue job kicks off the same 1000 api jobs for a second time

and so the loop continues

sonarqubecloud · 2026-05-19T14:57:30Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.0% Coverage on New Code
7.8% Duplication on New Code

See analysis details on SonarQube Cloud

spqr-praetoria requested a review from a team as a code owner May 5, 2026 12:16

spqr-praetoria changed the title ~~[NPQ-3490] - store long live auth token~~ [NPQ-3490] - store long lived auth token May 5, 2026

spqr-praetoria temporarily deployed to review May 5, 2026 12:20 — with GitHub Actions Inactive

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch from fd8ba6c to 01efa4e Compare May 5, 2026 13:38

spqr-praetoria temporarily deployed to review May 5, 2026 13:41 — with GitHub Actions Inactive

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch 2 times, most recently from 895d923 to 0ef27e0 Compare May 5, 2026 15:55

spqr-praetoria temporarily deployed to review May 5, 2026 16:07 — with GitHub Actions Inactive

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch from 0ef27e0 to 345820b Compare May 6, 2026 07:07

spqr-praetoria temporarily deployed to review May 6, 2026 07:14 — with GitHub Actions Inactive

spqr-praetoria temporarily deployed to staging May 6, 2026 07:16 — with GitHub Actions Inactive

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch 3 times, most recently from 03ae5b4 to ed6c714 Compare May 6, 2026 07:47

spqr-praetoria temporarily deployed to review May 6, 2026 07:56 — with GitHub Actions Inactive

spqr-praetoria temporarily deployed to staging May 11, 2026 13:03 — with GitHub Actions Inactive

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch 3 times, most recently from 82ceb3c to 65339ad Compare May 18, 2026 11:27

spqr-praetoria had a problem deploying to review May 18, 2026 11:29 — with GitHub Actions Failure

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch 3 times, most recently from 2c04143 to 8050cbe Compare May 19, 2026 08:47

spqr-praetoria had a problem deploying to review May 19, 2026 08:50 — with GitHub Actions Failure

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch from 8050cbe to c4768b8 Compare May 19, 2026 09:01

spqr-praetoria had a problem deploying to review May 19, 2026 09:04 — with GitHub Actions Failure

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch from c4768b8 to a94bdb2 Compare May 19, 2026 10:24

spqr-praetoria had a problem deploying to review May 19, 2026 10:27 — with GitHub Actions Failure

spqr-praetoria temporarily deployed to staging May 19, 2026 10:29 — with GitHub Actions Inactive

spqr-praetoria closed this May 19, 2026

spqr-praetoria temporarily deployed to review May 19, 2026 10:40 — with GitHub Actions Inactive

spqr-praetoria reopened this May 19, 2026

spqr-praetoria temporarily deployed to review May 19, 2026 10:44 — with GitHub Actions Inactive

spqr-praetoria temporarily deployed to staging May 19, 2026 10:49 — with GitHub Actions Inactive

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch from a94bdb2 to 187968b Compare May 19, 2026 11:24

spqr-praetoria temporarily deployed to review May 19, 2026 11:27 — with GitHub Actions Inactive

spqr-praetoria temporarily deployed to staging May 19, 2026 11:29 — with GitHub Actions Inactive

jebw previously approved these changes May 19, 2026

View reviewed changes

jebw reviewed May 19, 2026

View reviewed changes

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch from 187968b to 900ec36 Compare May 19, 2026 14:01

spqr-praetoria temporarily deployed to staging May 19, 2026 14:06 — with GitHub Actions Inactive

spqr-praetoria temporarily deployed to review May 19, 2026 14:08 — with GitHub Actions Inactive

[NPQ-3490] - store long live auth token

618c5a0

spqr-praetoria force-pushed the NPQ-3490-store-long-lived-auth-token branch from 900ec36 to 618c5a0 Compare May 19, 2026 14:50

spqr-praetoria temporarily deployed to staging May 19, 2026 14:55 — with GitHub Actions Inactive

spqr-praetoria temporarily deployed to review May 19, 2026 15:06 — with GitHub Actions Inactive

		@@ -3,6 +3,7 @@
		module Users
		class FindOrCreateFromTeacherAuth

Conversation

spqr-praetoria commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes proposed in this pull request

Uh oh!

github-actions Bot commented May 5, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

jebw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jebw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented May 19, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

spqr-praetoria commented May 5, 2026 •

edited

Loading