Catch OIDC error #13078

dacook · 2025-01-15T06:04:58Z

Catches the OAuth exception listed here: https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/66e89c5fdf0c335670df0dd3?filters%5Bevent.since%5D=all (note that the Faraday::ServerError is already handled, although I haven't added a spec for it.)

What? Why?

May close [DFC Orders] Improve OIDC Error Handling #12987

What should we test?

It occurs when the session dies for some reason. I'm not sure if there's a way to trigger it.

Manually triggered within a spec:

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

mkllnk · 2025-01-16T00:31:35Z

It occurs when the session dies for some reason. I'm not sure if there's a way to trigger it.

I think that you invalidate tokens in Keycloak. So that's possible for manual testing but not automated testing. There we just have to mock the response.

mkllnk · 2025-01-16T00:33:01Z

app/controllers/admin/dfc_product_imports_controller.rb

-           ActionController::ParameterMissing => e
+           ActionController::ParameterMissing,
+           Rack::OAuth2::Client::Error => e
      flash[:error] = e.message
      redirect_to admin_product_import_path


Does the flash message persist or disappear automatically?

In my testing I confirmed that error messages persist.
I remember this was intentional as part of the BUU update (other messages auto dismiss, but errors need to be dismissed by the user).

dacook · 2025-01-16T01:19:40Z

I think that you invalidate tokens in Keycloak.

Thanks, can you describe how I would do that? I just tried logging in to Les Communs with the testdfc@protonmail.com details in BitWarden, but it only showed my current browser session:
https://login.lescommuns.org/auth/realms/data-food-consortium/account/#/security/device-activity

Is that the right account?

Hmm, maybe it's because the session has ended already! I'll stage this PR and see what happens.

dacook · 2025-01-16T01:27:47Z

Oh it's currently only connected on uk-staging. It shows the session is active, with no error: https://staging.openfoodnetwork.org.uk/admin/oidc_settings

So I can see the token is valid, but don't know how to invalidate it still.

dacook · 2025-01-16T05:32:23Z

Tried to test in spec/system/admin/dfc_product_import_spec.rb with:

   allow(DfcRequest).to receive(:call).and_raise(Rack::OAuth2::Client::Error, "session problem")

But no luck for some reason.

So I tried to manually replicate this in dev. With an invalid refresh_token, the import works as usual. I guess that's only needed for refreshing. With an invalid token, a 403 error message appears (existing functionality):

 OidcAccount.first.update! token: "blah"

So I don't know how to reliably replicate this issue. @mkllnk do you think it would be ok to merge it as-is and see how we go? Or wait until it occurs on the staging environment, stage it, and confirm it then?

mkllnk · 2025-01-16T06:18:58Z

allow(DfcRequest).to receive(:call).and_raise(Rack::OAuth2::Client::Error, "session problem")

It's not a class method, it's an instance method. You'll need allow_any_instance_of.

dacook · 2025-01-16T06:40:25Z

It's not a class method, it's an instance method. You'll need allow_any_instance_of.

Oops, thanks you're absolutely right. I did try that before, but now I see that I was testing it wrong that time. Will try again next week.

I'm not sure if this can be tested easily, or needs to be.

mkllnk

Nice catch! But I think that we can improve this if you have time.

spec/system/admin/dfc_product_import_spec.rb

I forgot how hard it is to do view stuff in controllers. I tried using the short lazy-lookup key '.oauth_error_html', which is supposed to work in controllers. But perhaps it doesn't work within a rescue?

mkllnk

Nice to have the link. What about updating the OIDC account and delete the token so that it can be refreshed more easily?

app/controllers/admin/dfc_product_imports_controller.rb

dacook · 2025-01-21T06:16:18Z

What about updating the OIDC account and delete the token so that it can be refreshed more easily?

Ah, I misunderstood your initial message and didn't read that as a suggested change 😅

todo: add spec

dacook · 2025-01-22T00:02:55Z

What about updating the OIDC account and delete the token so that it can be refreshed more easily?

@mkllnk I made a start on this, but have to admit I'm not really sure and it might not be worth following up. Am I on the right track, or should we just stop here (at "[fixup] Sanitise content from external source") and move on?

app/controllers/admin/dfc_product_imports_controller.rb

mkllnk · 2025-01-22T02:59:21Z

engines/dfc_provider/app/services/dfc_request.rb

+    rescue Rack::OAuth2::Client::Error => e
+      @user.oidc_account.update!(
+        token: nil,
+        refresh_token: nil
+      )
+      throw e


Yes, that's what I imagined. I also imagined a custom error class but that's just nice to have.

Thanks, that crossed my mind too but I figured it could be added later if needed.

engines/dfc_provider/app/services/dfc_request.rb

Co-authored-by: Maikel <maikel@email.org.au>

But the test session isn't active! [skip ci]

dacook · 2025-01-22T03:04:30Z

engines/dfc_provider/app/services/dfc_request.rb

+    rescue Rack::OAuth2::Client::Error => e
+      @user.oidc_account.update!(
+        token: nil,
+        refresh_token: nil
+      )
+      throw e


Thanks, that crossed my mind too but I figured it could be added later if needed.

dacook · 2025-01-22T04:02:19Z

engines/dfc_provider/spec/services/dfc_request_spec.rb

@@ -88,5 +88,33 @@
      products = graph.select { |s| s.semanticType == "dfc-b:SuppliedProduct" }
      expect(products).to be_present
    end
+


@mkllnk I've had a go at speccing this, but in order to test that it clears the tokens, we need to set tokens first.

I've copied secret vars from Bitwarden and am able to make these requests, but the test session isn't active and results in the error "invalid_grant :: Session not active" (the exact error I'm trying to test!)

Am I doing things the long way round? Let me know if you see a better way.

Otherwise, can you please help me to refresh the session? Then hopefully the below spec will work..

I wouldn't use VCR for this because the spec then depends on the state of your session. The one in Bitwarden is probably outdated. Use webmock. And you got the right error response to fill your example already.

Thanks for the feedback. I had another go, but I can't seem to set up the right context for this case to work.

Maybe it's not worth trying to make the spec. What if we merge this without a spec, and see if it solves the errors?

dacook requested a review from mkllnk January 15, 2025 06:05

github-actions bot assigned dacook Jan 15, 2025

mkllnk reviewed Jan 16, 2025

View reviewed changes

dacook added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jan 16, 2025

Catch OIDC error

4ce2730

I'm not sure if this can be tested easily, or needs to be.

dacook force-pushed the catch-oidc-error-12987 branch from aee70df to 4ce2730 Compare January 21, 2025 00:58

dacook marked this pull request as ready for review January 21, 2025 01:10

dacook requested a review from mkllnk January 21, 2025 01:10

mkllnk approved these changes Jan 21, 2025

View reviewed changes

spec/system/admin/dfc_product_import_spec.rb Show resolved Hide resolved

Show more specific message for oauth errors

4c84fdf

I forgot how hard it is to do view stuff in controllers. I tried using the short lazy-lookup key '.oauth_error_html', which is supposed to work in controllers. But perhaps it doesn't work within a rescue?

dacook requested a review from mkllnk January 21, 2025 05:20

mkllnk reviewed Jan 21, 2025

View reviewed changes

app/controllers/admin/dfc_product_imports_controller.rb Outdated Show resolved Hide resolved

dacook added 2 commits January 22, 2025 10:54

[fixup] Sanitise content from external source

a3f916f

[wip] Delete tokens when authentication fails

00b78f4

todo: add spec

mkllnk reviewed Jan 22, 2025

View reviewed changes

dacook and others added 2 commits January 22, 2025 14:05

Apply suggestions from code review

ff3af2f

Co-authored-by: Maikel <maikel@email.org.au>

Refactor

9497e77

dacook force-pushed the catch-oidc-error-12987 branch from 5bc0335 to 4e15a6e Compare January 22, 2025 03:49

[wip] spec for clearing tokens

ec85822

But the test session isn't active! [skip ci]

dacook force-pushed the catch-oidc-error-12987 branch from 4e15a6e to ec85822 Compare January 22, 2025 04:00

dacook commented Jan 22, 2025

View reviewed changes

dacook added 2 commits February 3, 2025 14:10

try stubbing instead

c2dd1b2

still no luck

46ef03a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch OIDC error #13078

Catch OIDC error #13078

dacook commented Jan 15, 2025 •

edited

Loading

mkllnk commented Jan 16, 2025

mkllnk Jan 16, 2025

dacook Jan 16, 2025

dacook commented Jan 16, 2025

dacook commented Jan 16, 2025

dacook commented Jan 16, 2025

mkllnk commented Jan 16, 2025

dacook commented Jan 16, 2025

mkllnk left a comment

mkllnk left a comment

dacook commented Jan 21, 2025

dacook commented Jan 22, 2025

mkllnk Jan 22, 2025

dacook Jan 22, 2025

dacook Jan 22, 2025

dacook Jan 22, 2025

mkllnk Jan 22, 2025

dacook Feb 3, 2025

Catch OIDC error #13078

Are you sure you want to change the base?

Catch OIDC error #13078

Conversation

dacook commented Jan 15, 2025 • edited Loading

What? Why?

What should we test?

Release notes

mkllnk commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook commented Jan 16, 2025

dacook commented Jan 16, 2025

dacook commented Jan 16, 2025

mkllnk commented Jan 16, 2025

dacook commented Jan 16, 2025

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

dacook commented Jan 21, 2025

dacook commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook commented Jan 15, 2025 •

edited

Loading