n4bb12 / verdaccio-github-oauth-ui

📦🔐 GitHub OAuth plugin for Verdaccio
https://verdaccio.org
MIT License
74 stars 45 forks source link

Username token mapping is not cached #183

Closed thatsmydoing closed 1 year ago

thatsmydoing commented 1 year ago

Bug Report

Checking the username of a token was added in https://github.com/n4bb12/verdaccio-github-oauth-ui/commit/a166cf0ec80ae2353110a3a47ca110d33350bd6a 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

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.

n4bb12 commented 1 year ago

Thanks for reporting.

n4bb12 commented 1 year ago

There are a few things here that tell me caching this doesn't make sense:

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.

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.

This function is only called during authentication, not during each API request. So on a per-user basis, these requests are low frequency already.

thatsmydoing commented 1 year ago

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

https://github.com/verdaccio/verdaccio/blob/a69978755deacb5034fae8a8a35291ed7ad19473/src/lib/auth.ts#L435

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.

n4bb12 commented 1 year ago

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?

thatsmydoing commented 1 year ago

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.