Skip to content

Allow asynchronous bearer token #377

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

Merged
merged 6 commits into from
Oct 13, 2020

Conversation

nandorojo
Copy link
Contributor

This PR closes #376.

Sometimes, you need to run an asynchronous function to get your token from your auth provider. In order to always have the most up-to-date token, this function should be called at the time of request.

It seems like the tests pass for this, I'm testing it in my project now too. Will report back.

@nandorojo
Copy link
Contributor Author

I updated a few types and fixed it up. It's working well in my app.

@ferdikoomen
Copy link
Owner

Hi @nandorojo Although im not sure if this async token handler is a very good idea (seems like logic that the application should handle), but the change is pretty small :-) This does mean that for each call the OpenAPI client will get / wait for a new bearer token.., that seems a bit weird. The idea with these bearer tokens is that they expire after some time and you need to re-fetch them at that point. Normally you would have some kind of client (like the OpenID client) that would have some kind of event that gives you a new token, just before the last one expires:

const userManager = new UserManager({...});
userManager.events.addUserLoaded(user => {
    OpenAPI.TOKEN = user.access_token;
});

This is an example from the OpenID client: https://github.com/IdentityModel/oidc-client-js/wiki. I assume that Firebase auth also has a very similar concept (onTokenRefresh): https://firebase.google.com/docs/reference/js/firebase.messaging.Messaging#ontokenrefresh

@nandorojo
Copy link
Contributor Author

@ferdikoomen thanks for getting back to me!

I understand what you mean, but without doing this, requests won't reliably use the correct token in each call. And it feels like a much simpler approach than a long work-around: just get the token before making the call.

In the case of Firebase, the function is asynchronous, but if the token hasn't expired, it returns the current token "synchronously". Firebase only makes an asynchronous request if the token is past the expiration time to refresh it. This means that in almost every request, the token getter function is simply returning the current token without any async requests. But in the off chance that it needs to refresh, I wouldn't want to send a request with a stale token.

The alternative is, I refactor my entire app and use something like Redux to track the token every time it changes, and conditionally render pages based on this token. But this could lead to flashes of empty content when a token refreshes. I would rather implement this logic in my network requests, and I agree that this library should handle it. Plus, the behavior is fully opt-in, and not breaking at all :)

I've been using it in my app, and it works really well.

@nandorojo
Copy link
Contributor Author

nandorojo commented Oct 6, 2020

Just to follow up, Firebase does have this functionality: https://firebase.google.com/docs/reference/js/firebase.auth.Auth

However, the user it returns doesn't directly return a token field, but rather an async getIdToken() function.

onIdTokenChanged(async user => {
  const token = await user.getIdToken() // ✅ this is how you get it
  const token = user.token              // 🚨 this sadly does not exist
})

@nandorojo
Copy link
Contributor Author

@ferdikoomen Just wanted to see if you're open to merging this. If not, I'll sadly have to work off my own fork. Would you be open to at the very least merging it undocumented?

To reiterate what I mentioned before: my token getter function is async, but almost every time it's called, it's returning a cached value synchronously. However, in order to keep it perfectly up-to-date with refresh tokens, I need to check for the token before each network request. The best solution for this is to allow the TOKEN to be a function that returns a string.

Thanks again for the great work on this library. We're only a team of 2 (one front-end and one back-end) and it's made our workflow way easier.

@ferdikoomen
Copy link
Owner

@nandorojo I added some changes to the PR, with support for the XHR flow, some added documentation and the E2E tests. If the tests pass then ill merge the PR!

@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #377 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #377   +/-   ##
=======================================
  Coverage   93.34%   93.35%           
=======================================
  Files         104      104           
  Lines        1263     1264    +1     
  Branches      225      225           
=======================================
+ Hits         1179     1180    +1     
  Misses         84       84           
Impacted Files Coverage Δ
src/utils/registerHandlebarTemplates.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84099ee...a5a8c0b. Read the comment docs.

@ferdikoomen ferdikoomen merged commit b79618f into ferdikoomen:master Oct 13, 2020
@nandorojo
Copy link
Contributor Author

@ferdikoomen thanks so much!

NicoVogel pushed a commit to NicoVogel/openapi-typescript-codegen that referenced this pull request Dec 19, 2020
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

Successfully merging this pull request may close these issues.

Fire promise globally before any request
2 participants