-
Notifications
You must be signed in to change notification settings - Fork 477
[xdl] Add EXPO_TOKEN authentication method #2415
Conversation
b84d20f
to
bf6f8f5
Compare
@fson I also added some unit tests for the changes I made. It's a combination of the old existing integration tests (with mocked API) and new tests for the token handling. I can remove this if it's too much. But I'd figure adding some tests for this would be good. Plus, I fixed the simulator integration test (still skipped though) if someone wants to test that again. Edit: I'm not sure why the E2E tests from |
The failing test (https://app.circleci.com/pipelines/github/expo/expo-cli/3974/workflows/6dd053cc-c228-4e6b-b0c7-65b862553911/jobs/50658) runs |
We might be able to improve some performance if we ship a lockfile and cache the |
Ah, there we go! It works 😄 |
003fc09
to
a2bc122
Compare
I refactored the Also slightly increased the timeout for the failing XDL publish test. It's a bit flaky as you mentioned for me, hope this will make it succeed. |
1d63a37
to
fb959ef
Compare
What I did
This integrates the
EXPO_TOKEN
authentication method, implemented as aBearer
authorization token.Added the token retrieval in
UserSettings
I think this is the proper place for that. Eventually, it's an environmental setting but still set by the user. We could also move it to the
UserManager
itself, but I think using it fromUserSettings
makes more sense.Added token in the
getSessionAsync
methodWhen integrating the robots, I noticed this method is used in other areas to see if people are authenticated. With this, the DevTools also uses the proper username and other authentication stuff (e.g. username).
Added to API v1 and v2I can't find traces of API v1 being used somewhere, but I added it using almost identical implementation. Again, the token is fetched fromsee section above.UserSettings
by theUserManager
and passed through within the (current) user. This should work fine forApiV2Client.clientForUser(user)
calls. For API v1, I added a call toUserSettings
to fetch the token.Token has priority over existing authentication session
E.g. when a user runs this from his or her computer, we probably want to prioritize the
EXPO_TOKEN
value. When the user is already authenticated, this token is used over any existing session.User session data from token isn't stored through
UserSettings
I think this is important to mention as well, I don't think any data fetched with the token should be stored. If users provide the token inline, this data should only be "available" during that same execution call (e.g.
EXPO_TOKEN=xxx expo publish
). It also doesn't interfere with existing sessions, falling back right to that when the token isn't set anymore.What needs to be done
Add (unit) tests- Done, see commentDouble-check the analytics calls for EXPO_TOKEN sessionsDoneRight now, when
ensureLoggedInAsync
is called, it fetches the user. If no current user is loaded yet, it will fetch the info and send out the login analytics event. I'm not sure if this is still valid when using the token. Every call using that token and usingensureLoggedInAsync
will send out the login event.Consider if we need to fail when using EXPO_TOKEN andDoneexpo logout
Logging out isn't that relevant here, I don't think we can even do that. As long as that token is defined in the environment, the user will still be authenticated using that token.
See it in action
$ expo whoami
with inline and exported token$ expo build:android
with inline token$ expo build:ios
with exported token