Username token mapping is not cached
thatsmydoing opened this issue · 5 comments
Bug Report
Checking the username of a token was added in a166cf0 to fix a security issue. Since this isn't cached, each request requires an API call to github.
Versions
| Version | |
|---|---|
| Verdaccio | v5.24.1 |
| This plugin | v6.0.2 |
| Node | v18.16.0 |
Environment
| Name | Version | |
|---|---|---|
| Package manager | yarn | 3.5.1 |
| Browser | Firefox | 113 |
| Operating system | macos | 13.4 |
Observed behavior
Sometimes too many calls are made to github and it hits the rate limit.
Expected behavior
It should not hit the rate limit.
Steps to reproduce
- Do a yarn install on a big enough project and clear the cache
- Repeat
- Installation hangs
Additional context
Once the limit is hit, verdaccio repeatedly outputs
Request quota exhausted for request GET /user
This comes from https://github.com/octokit/octokit.js/blob/fb11ed709ba636868974ac8ef344099ab35825a6/src/octokit.ts#L25
I don't think this is very likely to be hit, I was testing the new yarn, benchmarking etc. Though I think making an API call per request does also have a performance impact.
Thanks for reporting.
There are a few things here that tell me caching this doesn't make sense:
- What should be the cache key?
The only argument to getting the username from GitHub is the token. Keeping a bunch of valid tokens in memory is probably not a good idea.
- These tokens are never seen twice
Every GitHub OAuth code flow yields a new token. If the input changes each time, I don't see how this is cacheable, even if we wanted to.
- Why even cache this
This function is only called during authentication, not during each API request. So on a per-user basis, these requests are low frequency already.
This function is only called during authentication, not during each API request. So on a per-user basis, these requests are low frequency already.
I think that's only true if you're using jwt authentication. For legacy API auth, it does get called for every API request
And now I find the documentation stating that legacy is not supported by this plugin, but it still actually works for the most part which is the cause for confusion.
We've always had it on legacy and I think it only really started being an issue with the recent changes.
Yes, unfortunately. The workarounds, deviating implementations, and things to consider in order to support both options are consuming too much time.
May I ask, if there is a reason you're not switching?
We started using it with legacy since it was the default and we didn't want to switch as it would be disruptive since everyone would have to update their tokens.
How jwt auth works is also a bit unclear since none of it is explained. With legacy auth, we knew that the token would last "forever" (unless you never used it and github expires it). Removing a user from the organization also instantly revokes the token so we don't have to do anything extra.
IIUC, jwt auth necessarily expires and developers would have to periodically login. It's also not clear what this looks like. Can developers just do yarn npm login or will they have to go to the verdaccio page and copy-paste some commands. It's also not clear what happens if a user is removed from the organization. Is their jwt also revoked? Can it be even revoked? Do we have to set the expiry very low so it doesn't last too long?
There's too many questions and potential downsides that we didn't feel it was worth switching.