Update openid-client to v6 #1394
Conversation
v6 is a complete rewrite, with API changes: https://github.com/panva/openid-client/releases/tag/v6.0.0 Closes #1230
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Looks good to me!
Nothing to change here but I'd like mention that I am getting Support for loading ES Module in require() is an experimental feature and might change at any time warning. Maybe this feature will get matured or we may consider migrating to ESM 🤷♂️
| const { // eslint-disable-line object-curly-newline | ||
| authorizationCodeGrant, | ||
| buildAuthorizationUrl, | ||
| calculatePKCECodeChallenge, | ||
| fetchUserInfo, | ||
| randomPKCECodeVerifier, | ||
| randomState, | ||
| } = require('openid-client'); // eslint-disable-line object-curly-newline |
There was a problem hiding this comment.
this is a nit / preference: we can const client = require('openid-client'); then the rest of the code in this file will be easier to read and understand, specially knowing which functions are coming from the external library.
Good spot - I'll take a look into this.
I think we should avoid that migration as long as possible. |
Current status:
which means
I'll hold of merging for now. |
openid-clientv6 is a complete rewrite, with API changes: https://github.com/panva/openid-client/releases/tag/v6.0.0API reference: https://github.com/panva/openid-client/blob/v6.1.7/docs/README.md
Closes getodk/central#1225
What has been done to verify that this works as intended?
Existing tests; new tests at #1392
Why is this the best possible solution? Were any other approaches considered?
Staying up-to-date with security-focussed libraries seems like a sensible approach. An alternative might be to fork the
openid-clientlibrary, but this seems like high-risk, unnecessary work for no obvious benefit.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
It should not affect users.
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
No.
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass OR confirm CircleCI build passes