perarnborg / vuex-oidc

Vuejs (with vuex) wrapper for open id authentication
MIT License
232 stars 64 forks source link

Replace oidc-client with oidc-client-ts #187

Closed mmizutani closed 1 year ago

mmizutani commented 2 years ago

With big thanks to the maintainer of oidc-client, brockallen, this replaces the oidc-client library with its successor, oidc-client-ts. oidc-client is no longer maintained, and it is not quite desirable to continue relying on abandoned libraries, especially for security sensitive authn/authz software.

The reasons that the migration the dependency from oidc-client to oidc-client-ts can be justified are as follow:

A downside is that oidc-client-ts does not support the OAuth 2.0 Implicit Flow. oidc-client-ts suports the Implicit Flow and the Authorization Code Flow with PKCE, while oidc-client-ts only supports the Authorization Code Flow with PKCE and Refresh Token Grant. Existing users of vuex-oidc might need to tweak some configurations of their identity providers to change the auth flow type. However, OIDC service providers such as Microsoft and Auth0 nowadays recommend against the use of the Implicit Flow and encourage switching to the Authorization Code Flow with PKCE (or some other alternatives) even for SPAs. Microsoft Authentication Library for JavaScript (MSAL.js) v2, for example, has dropped support for the OAuth 2.0 Implicit Grant Flow.

In one of my Vue.js SPAs at work, I have experimentally switched from vuex-oidc with oidc-client to vuex-oidc with oidc-client-ts. The migration was quite smooth; I only needed to enable refresh tokens in AzureAD and slightly change the configurations for vuex-oidc as follows:

import { vuexOidcCreateStoreModule } from 'vuex-oidc'
- import Oidc from 'oidc-client'
+ import { Logger, Log } from 'oidc-client-ts'

- Oidc.Log.logger = window.console
- Oidc.Log.level = process.env.NODE_ENV === 'development' ? Oidc.Log.DEBUG : Oidc.Log.WARN
+ Log.setLogger(window.console)
+ if (process.env.NODE_ENV === 'development') {
+   Log.setLevel(Log.DEBUG)
+ } else {
+   Log.setLevel(Log.WARN)
+ }

export const oidcSettings = {
  authority: `https://${process.env.oidcHost}/v2.0`,
  clientId: process.env.clientId,
  redirectUri: process.env.redirectUri,
- responseType: 'id_token token',
+ responseType: 'code',
- scope: 'openid profile',
+ scope: 'openid profile offline_access',
  automaticSilentRenew: true,
  validateSubOnSilentRenew: true,
  automaticSilentSignin: true,
  includeIdTokenInSilentRenew: true,
- revokeAccessTokenOnSignout: true
+ revokeTokensOnSignout: true
}

This has been working well in an Authorization Code Flow with PKCE and access token auto-refresh setup.

To help migration by the existing users of the vuex-oidc, I have also prepared some changes to the wiki of this repository:

https://github.com/mmizutani/vuex-oidc/wiki/Home/_compare/d9bb6cb0855e83464435bbf2042b7eac48f5b48c...d6da010b8af3a8ac2b1a2a8156f05972624f50e2

If the backward-incompatible changes introduced by this PR is not acceptable, we might consider adding a user-configurable parameter to select which of the OIDC client libraries to use to vuex-oidc.

Resolve #177

mmizutani commented 2 years ago

@perarnborg I don't want to bother you, but could you please let us know what you think about this change to the core dependency of vuex-oidc?

dlamande commented 2 years ago

@perarnborg can u have a look please ? I'm interested in this change. Indeed, the current version of oidc-client uses CryptoJS 3.1.2 with vulnerability. With oidc-client-ts we will be up to date !

perarnborg commented 2 years ago

Thank you for your time and effort put into this @mmizutani!

This does introduce some breaking changes. That could be possible in a major version bump though, so I am not completely against it. The added complexity of providing the option to choose dependency library (oidc-client / oidc-client-ts) does not appeal to me as a maintainer.

For me personaly this is a low priority, since I am part of a team building an alternative client lib to oidc-client-ts. The motivation of doing this is that there are some things inherited from oidc-client that I want to change for my projects. This library will also be publicly available, and will come with it's own wrappers for different frameworks like Vue. I am not planning on moving this project to that client, due to backward incompatibilities.

@mmizutani An alternative that I see is that we fork this into a different project (vuex-oidc-ts?). If you (or someone else in the community) would like to do that, feel free to grab the source code from this library if you want! Since my main focus is somewhat elsewhere for now, that could be an idea.

However, as I said I am also open to merging this and publish it as v4 as you suggest. The changes that where needed were indeed minimal. I am not completely ruling out the route of providing the option to choose dependency lib, it just does not appeal to me at first glance... I will need to think about this for a little while, please provide your feedback below! ❤️

For those interested, here is the other oauth/oidc client project that I am working on: https://github.com/Aventyret/lionel-oauth-client It is a work in progress.

mmizutani commented 2 years ago

@perarnborg Thank you for sharing your thoughts on this proposal and the recent progress in your new work in https://github.com/Aventyret/lionel-oauth-client, a possible successor of the oidc-client-js library.

Indeed, the option to choose dependency library (oidc-client(-js) / oidc-client-ts) would introduce too much complication and would be an overkill.

Publishing the changes I am going to introduce as v4 would be a more desirable option. However, it nevertheless increases the maintenance burden of this library in the sense that the repo maintainer would still have to make changes to the current v3 branch to polish existing features and address requests from existing users of v3 and at the same time porting those changes to oidc-client-ts based v4 for the time being. Since an ideal replacement of oidc-client(-js) is not oidc-client-ts but ionel-oauth-client (once completed) from the view point of the maintainer, I do not want to force this route either.

A possible middle ground I would suggest is not merging my PR to the master branch and releasing it as v4 but merging my PR to a new separate, experimental, protected branch named, say, experimental-oidc-client-ts without actually releasing as v4. This way, the maintainer is not responsible for officially supporting this variant of the library where the oidc-client(-js) dependency is replaced by oidc-client-ts, whereas users of this library can git clone the new experimental branch via npm/yarn/pnpm and try the new unofficial version not relying on the now archived core dependency (at their own risk, thought) if necessary.

If this sounds feasible to those who are watching this PR, I will change the base branch of this PR and submit another PR to the master branch to add some note to guide interested library users to try the new experimental branch until lionel-oauth-client graduates the beta phase and gets ready for integration with other libraries like vuex-oidc.

mediafreakch commented 1 year ago

I tested this PR against https://github.com/perarnborg/vuex-oidc-example .

Bottom line is that the Authorization Code flow with PKCE for Google requires you to send a clientSecret, as outlined in this comment. In Single Page Applications this seems not to be a good idea, as the secret can be read by anyone. Whether it is correct for Google to require the secret in PKCE flow is another topic.

I however successfully got the example working with a different provider, Keycloak in my case. The only change necessary was to disable the implicit flow in the server-side client settings, and enable the standard flow instead. For keycloak, no clientSecret was required.

Here's the diff to make it work with oidc-client-ts (To make it work with Google, you would need an additional clientSecret which I did not know)

.env

-VUE_APP_OIDC_CONFIG={"authority": "https://accounts.google.com", "clientId": "459300396575-3ruj8l8jn69pcgst8rgkqnk6g43gbc78.apps.googleusercontent.com", "redirectUri": "http://localhost:5002/oidc-callback", "popupRedirectUri": "http://localhost:5002/oidc-popup-callback", "responseType": "id_token token", "scope": "openid email", "automaticSilentRenew": true, "automaticSilentSignin": false, "silentRedirectUri": "http://localhost:5002/silent-renew-oidc.html"}
+VUE_APP_OIDC_CONFIG={"authority": "https://accounts.google.com", "clientId": "459300396575-3ruj8l8jn69pcgst8rgkqnk6g43gbc78.apps.googleusercontent.com", "redirectUri": "http://localhost:5002/oidc-callback", "popupRedirectUri": "http://localhost:5002/oidc-popup-callback", "responseType": "code", "scope": "openid email", "automaticSilentRenew": true, "automaticSilentSignin": false, "silentRedirectUri": "http://localhost:5002/silent-renew-oidc.html"}

package.json

     "uselocaldep": "cp ../vuex-oidc/dist/vuex-oidc.esm.js ./node_modules/vuex-oidc/dist/vuex-oidc.esm.js"
   },
   "dependencies": {
-    "oidc-client": "^1.10.1",
+    "eslint-plugin-vue": "^9.7.0",
+    "oidc-client-ts": "^2.1.0",
     "vue": "^2.5.17",
     "vue-router": "^3.0.1",
     "vuex": "^3.0.1", 
     "vuex-oidc": "^3.9.8" // I manually replaced vuex-oidc-esm.js by the version built using this PR

Imho it's still a win to go ahead and merge this PR and release vuex-oidc@4 with it, despite the drawback when using Google as OIDC Provider. Maybe a hint in the wiki can help people to see that when using Google, it might make sense to stay on vuex-oidc@3 with the implicit flow.

perarnborg commented 1 year ago

@mediafreakch Good point about Google, this should be noted in the docs after v4 is published.

Did you (or anyone else here) test authenticateOidcSilent? It throws an error for me in the example application.

I am looking in to it, but I would appreciate any help given!

perarnborg commented 1 year ago

...okay, I realized there was another breaking change. vuexOidcProcessSilentSignInCallback now needs the oidcSettings as an argument. This is fine, I will add that to the docs as well.

I will merge these changes in the next couple of days and publish to npm as v4.

mmizutani commented 1 year ago

Bottom line is that the Authorization Code flow with PKCE for Google requires you to send a clientSecret To make it work with Google, you would need an additional clientSecret which I did not know

Did you (or anyone else here) test authenticateOidcSilent? It throws an error for me in the example application.

Sorry, I forgot to pass the clientSecret and client_secret from the oidcConfig to the UserManager. This is fixed in https://github.com/perarnborg/vuex-oidc/pull/187/commits/b7dfa86b85fb8533f9c2545bbc5998a262e3769a.

Here is a working example adapted from https://github.com/perarnborg/vuex-oidc-example that uses oidc-client-ts and this PR branch: https://github.com/mmizutani/vuex-oidc-example/tree/switch-to-oidc-client-ts Since example app now requires not only the OAuth client ID but also the client secret when using the Authorization Code flow with PKCE with Google as the IdP, I have swapped the OAuth client credential with my own issued in my Google Cloud project as per the official documentation.

![image](https://user-images.githubusercontent.com/4973774/208224234-8fc932d3-a458-4acf-bcf0-8b75a3c93bb7.png)
perarnborg commented 1 year ago

@mmizutani Thank you for moving so fast on this! Your changes look great, I plan to pubslish v4 on Monday after I have looked through everything one last time and after I have updated the docs.

mmizutani commented 1 year ago

Thank you for the discussion and review.