Skip to content
This repository has been archived by the owner on Jan 18, 2024. It is now read-only.

[xdl] Add EXPO_TOKEN authentication method #2415

Merged
merged 13 commits into from
Aug 25, 2020
Merged

Conversation

byCedric
Copy link
Member

@byCedric byCedric commented Jul 31, 2020

What I did

This integrates the EXPO_TOKEN authentication method, implemented as a Bearer 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 from UserSettings makes more sense.

  • Added token in the getSessionAsync method
    When 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 v2
    I can't find traces of API v1 being used somewhere, but I added it using almost identical implementation. Again, the token is fetched from UserSettings by the UserManager and passed through within the (current) user. This should work fine for ApiV2Client.clientForUser(user) calls. For API v1, I added a call to UserSettings to fetch the token. see section above.

  • 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 comment

  • Double-check the analytics calls for EXPO_TOKEN sessions Done
    Right 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 using ensureLoggedInAsync will send out the login event.

  • Consider if we need to fail when using EXPO_TOKEN and expo logout Done
    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.

As TC mentioned, this also applies to logout, login, and register

See it in action

$ expo whoami with inline and exported token whoami example
$ expo build:android with inline token build android example
$ expo build:ios with exported token build ios example

packages/xdl/src/ApiV2.ts Outdated Show resolved Hide resolved
packages/xdl/src/UserSettings.ts Outdated Show resolved Hide resolved
packages/xdl/src/User.ts Outdated Show resolved Hide resolved
@byCedric
Copy link
Member Author

byCedric commented Aug 3, 2020

@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 expo-cli are timing out now. I don't think it's related to the changes I made in XDL?

Screenshot 2020-08-03 at 17 43 44

@fson
Copy link
Contributor

fson commented Aug 4, 2020

The failing test (https://app.circleci.com/pipelines/github/expo/expo-cli/3974/workflows/6dd053cc-c228-4e6b-b0c7-65b862553911/jobs/50658) runs yarn install inside a project, so I think it might be occasionally failing due to that taking too long... Not sure if there's a quick solution to this problem (increase the timeout? 💀), but if it this happens often we should definitely fix it (flaky tests will erode the teams' confidence in tests).

@byCedric
Copy link
Member Author

byCedric commented Aug 4, 2020

We might be able to improve some performance if we ship a lockfile and cache the node_modules from the system (e.g. yarn cache dir)? That should at least speed it up for a bit, but not always.

@byCedric
Copy link
Member Author

byCedric commented Aug 4, 2020

Ah, there we go! It works 😄

@byCedric
Copy link
Member Author

byCedric commented Aug 4, 2020

I refactored the getSessionAsync and getCurrentUsernameAsync methods so they work better with the devtools. I also noticed in some code getSessionAsync is also used to check if people are authenticated or not, which works better with this change.

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.

@byCedric byCedric merged commit 10a4d1d into master Aug 25, 2020
@byCedric byCedric deleted the @bycedric/feat/expo-token branch August 25, 2020 19:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants