Skip to content
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

Remove dependency on openid-client@4.9 #151

Closed
Tracked by #102
paulius-valiunas opened this issue Apr 7, 2023 · 5 comments
Closed
Tracked by #102

Remove dependency on openid-client@4.9 #151

paulius-valiunas opened this issue Apr 7, 2023 · 5 comments
Assignees

Comments

@paulius-valiunas
Copy link
Collaborator

paulius-valiunas commented Apr 7, 2023

This version of the package doesn't work well with ES Modules and will potentially have security vulnerabilities as it's not actively maintained. There are two potential solutions:

  • Update to version 5, which switched from got to node:http and removed retry support - we would have to add our own retry logic and nodejs documentation is not helpful at all for understanding the kind of errors we should handle
  • Remove the dependency altogether and just send http request using whatever library we want. I'm almost inclined to do this rather than the above option because there's only a couple requests we would have to construct - we don't use most of the features of that package.
@paulius-valiunas paulius-valiunas changed the title Update openid-client to 5.0 Remove dependency on openid-client@4.9 Apr 7, 2023
@paulius-valiunas
Copy link
Collaborator Author

@calebmshafer @wgoehrig @ben-polinsky any preference as to which way to proceed?

@ben-polinsky
Copy link
Collaborator

It'd be nice if the library allowed us to provide our own agent. Found your discussion thread with the maintainer :) You're right the error handling is tough, but if we have confidence in what got is doing, can't we upgrade openid-client and then copy got's retry implementation, or at least use its error handling as a guide?

@paulius-valiunas
Copy link
Collaborator Author

It'd be nice if the library allowed us to provide our own agent. Found your discussion thread with the maintainer :) You're right the error handling is tough, but if we have confidence in what got is doing, can't we upgrade openid-client and then copy got's retry implementation, or at least use its error handling as a guide?

we could, but the file you linked only contains the error classes that are constructed and thrown from all over their code base, and I don't think untangling all of that would be much easier than handling node:http errors. If I were to implement this logic, I would rather attach a debugger, try to reproduce a few different errors and then use https://github.com/sindresorhus/got/blob/main/documentation/7-retry.md as a guide. The source code of got would still be useful because I could look up specific node.js error codes when I'm unsure which one to use.

@ben-polinsky
Copy link
Collaborator

I think I also lean toward option two - getting rid of the package.

@paulius-valiunas paulius-valiunas self-assigned this Apr 18, 2023
@ben-polinsky
Copy link
Collaborator

Released in @itwin/oidc-signin-tool_v4.0.0 and @itwin/service-authorization_v1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants