-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add OIDC strategy for Identity #234
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, thanks for picking up 👍🏼 . A few comments left inline.
f6653b4
to
084f094
Compare
This is strange because I get the opposite error on the main branch.
On July 11, 2023, "codeclimate[bot]" ***@***.***> wrote:
@malcolmbaig commented on this pull request.
In
app/controllers/qualifications/users/omniauth_callbacks_controller.rb
<https://github.com/DFE-Digital/access-your-teaching-
qualifications/pull/234#discussion_r1259662861>:
> @@ -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
Testing this locally (using preprod Identity), I still get expires_at
back, leading to an exception at this line. Any idea why?
—
Reply to this email directly, view it on GitHub <https://github.com/DFE-
Digital/access-your-teaching-qualifications/pull/234#pullrequestreview-
1524153723>, or unsubscribe
<https://github.com/notifications/unsubscribe-
auth/AAAAYNRV74OZGSQVDGS7XU3XPVBIFANCNFSM6AAAAAA2AC2U7I>.
You are receiving this because you authored the thread.Message ID: <DFE-
Digital/access-your-teaching-
***@***.***>
|
Sorry, ignore! Removed my original comment because I realised I hadn't tested it properly. Needed a server restart. All good as far as this attribute is concerned. |
084f094
to
b7662bf
Compare
b7662bf
to
d19e227
Compare
config/initializers/omniauth.rb
Outdated
callback_path: "/check-records/auth/dfe/callback", | ||
client_options: { | ||
host: dfe_sign_in_issuer_uri&.host, | ||
identifier: ENV.fetch("DFE_SIGN_IN_CLIENT_ID", "example"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it'd be better to set a precedent and drop the default "example' value in these options? Useful for the app to crash early (during an attempt at starting the server in the dev environment, for example) so that we can pick up any misconfiguration before getting to prod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the suggested change as separate PR as it isn't directly related to this change.
In a [discussion on a different PR](#234 (comment)), there was a suggestion to remove the default values from the OIDC configuration to trigger misconfiguration messages sooner, eg. on app startup. The idea is that this would make development better as any misconfiguration error would be clearer.
In a [discussion on a different PR](#234 (comment)), there was a suggestion to remove the default values from the OIDC configuration to trigger misconfiguration messages sooner, eg. on app startup. The idea is that this would make development better as any misconfiguration error would be clearer.
d77e5dd
to
cb09c8d
Compare
In a [discussion on a different PR](#234 (comment)), there was a suggestion to remove the default values from the OIDC configuration to trigger misconfiguration messages sooner, eg. on app startup. The idea is that this would make development better as any misconfiguration error would be clearer.
cb09c8d
to
136e9a8
Compare
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.
136e9a8
to
c99759f
Compare
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.
Link to Trello card
https://trello.com/c/85JumIaw/1090-switch-quals-to-use-an-oidc-strategy-for-identity
Checklist