-
Notifications
You must be signed in to change notification settings - Fork 1
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
Better ORCID authorization #176
Conversation
This will provide a better user experience. It also supports our switch to three-legged OAuth2 authentication. **Implications** - Old bookmarks (except to the homepage) will break. They will take users to the homepage. - The serving application will need to send index.html in response to all requests for unknown pages. Vite does this in development mode, but, for instance, CloudFront or any webserver like Apache or Nginx need to be configured to do this.
This change replaces the vue-oidc-client and oidc-client-ts packages with custom authentication code. The new logic mainly resides in three files: - lib/orcid.ts (which replaces those packages and much of lib/auth.js) - lib/auth.ts (authorization utilities, replacing part of lib/auth.js) - store/modules/auth.ts (authorization/role management, replacing store/modules/auth.js) In addition, there are two new Vue components implementing callback screens: components/screens/OidcCallback.vue and components/screens/OidcCallbackError.vue. The rationale for this change is described in lib/orcid.ts: "Three-legged OAuth" is used here. We originally tried using third-party libaries -- primarily vuex-oidc, which in turn uses oidc-client or oidc-client-ts for the actual OAuth2/OIDC authentication process. We ran into issues with all OAuth2 and OIDC authentication flows: - With OIDC's implicit flow, the client does not need to manage a client secret; this is good for single-page applications whose source code is visible. This flow is supported by vuex-oidc v3 via oidc-client, though it is no no longer supported by the supported branches of these projects. However, the access token has a limited lifespan, and ORCID does not seem to support token renewal. (It does not return a renewal token, and its renew endpoint has an X-Frame-Options header that prohibits use in an iframe, so in any case we cannot renew silently.) So the user is logged out after 10 minutes. - The preferred option would be OIDC's authorization code flow with PKCE, which is more secure than the implicit flow and still does not require a client secret. However, ORCID does not support PKCE. Plans to support it seem to have stalled. (See ORCID/ORCID-Source#5977.) - So we have opted to use three-legged authentication, which requires participation of the MaveDB API. In this flow, the browser client first obtains a code when the user signs into ORCID and grants permissions. It sends this code to the MaveDB API, which makes a second request to ORCID, this time exchanging the code and MaveDB's client secret for an access token and, in addition, the ID token (JWT) that we will subsequently use for authorization. These have a very long lifespan (which ORCID's OAuth2 service returns in the expires_in parameter, measured in seconds). Three-leggged authentication is not supported by oidc-client or oidc-client-ts, so the present module implements it.
The sign-out callback is unused, and the sign-in callback is now replaced by components/screens/OidcCallback.vue.
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.
importing my comments from other PR
* The user's authentication state, profile and roles are exposed here, as are functions for logging in and out, so Vue | ||
* components should not need to interact with the | ||
*/ | ||
import _ from 'lodash' |
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.
doesn't look like this file uses lodash
* We send the following parameters to ORCID: | ||
* - client_id: MaveDB's OAuth2/OIDC client ID for ORCID, which is not a secret | ||
* - redirect_uri: The URI to which the user should be redirected after sign-in | ||
* - scope=openid: A valie indicating that we want an OpenID ID token |
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.
s/valie/value
const appUrl = `${window.location.origin}/` | ||
|
||
interface AuthState { | ||
/** A JWT identifying the current user. User details can be obtained by decoding it. */ |
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.
why a multi-line comment here?
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.
Rephrased, but I do want to leave the note about the token being decodable to get things like the user's name (i.e. not an opaque token).
src/lib/orcid.ts
Outdated
} | ||
|
||
/* | ||
// This is what authentication looks like using vuex-oidc v3 and the implicit flow. |
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.
do we need to preserve this? is it possible this library will ever work for us?
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.
We'll probably never want to go back to the implicit flow. We might return to using vuex-oidc if ORCID implements PKCE, and I sort of wanted to preserve this code (which is a little different from the vue-oidc code used previously). But I think you're right; I'll remove it now that it's in the commit history.
sub: string | ||
} | ||
|
||
// Notice the final '/'. |
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.
not sure I understand the comment here - is this used other than with oidc-callback appended on?
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 went ahead and removed this unused code.
@ashsny This change involves switching the Vue router mode, so that for instance instead of https://mavedb.org/#/score-sets/{urn}, the URL will be https://mavedb.org/score-sets/{urn}. I think we all prefer this style anyway, and the move to AWS facilitated it. Do you think that at this stage in adoption it's worth addressing the problem of broken bookmarks? One possibility is to detect a URL fragment that's a valid path in the application, navigate accordingly, and display a pop-up suggesting that the user update their bookmarks. |
import config from '@/config' | ||
import { oidc } from '@/lib/auth' |
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.
This import config from '@/config'
isn't needed here (duplicated from line 184), and I believe the import { oidc } from '@/lib/auth'
references a method removed by these changes. Both of these throw compiler errors when running npm run dev
.
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.
Thanks, not sure how that got into the checked-in version. I've removed it.
If ORCID implements PKCE (see ORCID/ORCID-Source#5977), we will be able to switch from our custom auth code back to vuex-oidc. In that case we may dig this code up and modify it to switch from implicit flow to PKCE.
Jeremy's original PR description follows:
Addresses #135.
The code for managing ORCID login sessions has been replaced in order to address three issues:
The rationale for this change is described in lib/orcid.ts:
This change replaces the vue-oidc-client and oidc-client-ts packages with custom authentication code. The new logic mainly resides in three files:
In addition, there are two new Vue components implementing callback screens: components/screens/OidcCallback.vue and components/screens/OidcCallbackError.vue.
Release notes