Skip to content
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

Support multiple accounts #3192

Merged
merged 27 commits into from
Apr 6, 2021
Merged

Support multiple accounts #3192

merged 27 commits into from
Apr 6, 2021

Conversation

samtstern
Copy link
Contributor

@samtstern samtstern commented Mar 5, 2021

Description

Implement multiple authentication (see Internal API proposal)

Fixes #1906
Fixes #181

Scenarios Tested

login:add

$ firebase login:add

Visit this URL on this device to log in:
https://accounts.google.com/o/oauth2/auth?client_id=563584335869-fgrhgmd47bqnekij5i8b5pr03ho849e6.apps.googleusercontent.com&scope=email%20openid%20https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fcloudplatformprojects.readonly%20https%3A%2F%2Fwww.googleapis.com%2Fauth%2Ffirebase%20https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fcloud-platform&response_type=code&state=75254219&redirect_uri=http%3A%2F%2Flocalhost%3A9005

Waiting for authentication...

✔  Success! Added account [email protected]

login:list

$ firebase login:list
Logged in as [email protected]

Other available accounts (switch with login:use)
 - [email protected]
 - [email protected]

login:use

$ firebase login:use [email protected]
✔  Set default account [email protected] for current project directory.

# ...

$ firebase login:list
Logged in as [email protected]

Other available accounts (switch with login:use)
 - [email protected]
 - [email protected]

logout

$ firebase logout
✔  Logged out from [email protected]
✔  Logged out from [email protected]
✔  Logged out from [email protected]

logout (primary account

$ firebase logout [email protected]
? You are logging out of your default account, which account should become the 
new default? [email protected]
✔  Logged out from [email protected]
✔  Setting default account to "[email protected]"

# ...

$ firebase login:list
Logged in as [email protected]

Other available accounts (switch with "firebase login:use")
 - [email protected]

init

$ firebase init database

     ######## #### ########  ######## ########     ###     ######  ########
     ##        ##  ##     ## ##       ##     ##  ##   ##  ##       ##
     ######    ##  ########  ######   ########  #########  ######  ######
     ##        ##  ##    ##  ##       ##     ## ##     ##       ## ##
     ##       #### ##     ## ######## ########  ##     ##  ######  ########

You're about to initialize a Firebase project in this directory:

  /private/var/folders/xl/6lkrzp7j07581mw8_4dlt3b000643s/T/tmp.2OgG9jvU

Before we get started, keep in mind:

  * You are currently outside your home directory


=== Account Setup

Which account do you want to use for this project? Choose an account or add a new one now

? Please select an option: [email protected]

✔  Using account: [email protected]

=== Project Setup

First, let's associate this project directory with a Firebase project.
You can create multiple project aliases by running firebase use --add, 
but for now we'll just set up a default project.

? Please select an option: Use an existing project
? Select a default Firebase project for this directory: fir-dumpster-ota (fir-du
mpster-ota)
i  Using project fir-dumpster-ota (fir-dumpster-ota)

=== Database Setup
i  database: ensuring required API firebasedatabase.googleapis.com is enabled...
⚠  database: missing required API firebasedatabase.googleapis.com. Enabling now...
✔  database: required API firebasedatabase.googleapis.com is enabled

? It seems like you haven’t initialized Realtime Database in your project yet. D
o you want to set it up? No

Firebase Realtime Database Security Rules allow you to define how your data should be
structured and when your data can be read from and written to.

? What file should be used for Realtime Database Security Rules? database.rules.
json
✔  Default rules have been written to database.rules.json.
Future modifications to database.rules.json will update Realtime Database Security Rules when you run
firebase deploy.

i  Writing configuration info to firebase.json...
i  Writing project information to .firebaserc...
i  Writing gitignore file to .gitignore...

✔  Firebase initialization complete!

--account flag (denied)

$ firebase [email protected] --project=fir-dumpster-ota functions:config:get

Error: HTTP Error: 403, The caller does not have permission

--account flag (succeed)

$ firebase [email protected] --project=fir-dumpster-ota functions:config:get
{
  "foo": {
    "baz": "qux"
  }
}

Sample Commands

Internal API proposal

@google-cla google-cla bot added the cla: yes Manual indication that this has passed CLA. label Mar 5, 2021
@@ -1,50 +0,0 @@
"use strict";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-written in TS

@samtstern samtstern changed the title [WIP] Support multiple accounts Support multiple accounts Mar 9, 2021
@samtstern samtstern marked this pull request as ready for review March 9, 2021 16:05
@joehan
Copy link
Contributor

joehan commented Mar 9, 2021

Did some testing, and noticed that firebase init still prompts me to choose an account if I use the --account flag.
Based on the API proposal, I think this should just use the account specified in the flag

@samtstern
Copy link
Contributor Author

Did some testing, and noticed that firebase init still prompts me to choose an account if I use the --account flag.
Based on the API proposal, I think this should just use the account specified in the flag

@joehan thanks! I just pushed a fix for this, I always forget about "inherited options"

Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some cleanup, but most of the core logic seems good to me

src/auth.ts Outdated Show resolved Hide resolved
src/auth.ts Outdated

// Clear any matching project defaults
const activeAccounts = configstore.get("activeAccounts") || {};
for (const projectDir of Object.keys(activeAccounts)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer using for.. in here instead of Object.keys, unless theres something reason not to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it but then eslint yelled at me about guarding it, which I think would be uglier:
https://eslint.org/docs/rules/guard-for-in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, yeah, I ran into that same rule recently. Maybe we should get rid of the rule, but I agree this is less ugly than adding an eslint-ignore or guarding it

configstore.set("tokens", lastAccessToken);
const account = findAccountByRefreshToken(refreshToken);
if (account && lastAccessToken) {
account.tokens = lastAccessToken;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Why is this account.tokens if it only has one token?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to keep terminology similar where I can. The "tokens" key has been used in configstore to refer to the object of type Tokens which contains access and refresh tokens. This is the same type.

src/commands/login-add.ts Outdated Show resolved Hide resolved
src/commands/login-add.ts Outdated Show resolved Hide resolved
src/commands/login-use.ts Outdated Show resolved Hide resolved
src/commands/login-use.ts Show resolved Hide resolved
src/commands/logout.ts Outdated Show resolved Hide resolved
src/commands/logout.ts Outdated Show resolved Hide resolved
src/init/features/account.ts Outdated Show resolved Hide resolved
@samtstern samtstern requested a review from joehan March 16, 2021 10:49
src/commands/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once you fix the typo in commands/index.js

Probably deserves a second set of eyes though given the scope.

src/defaultCredentials.ts Outdated Show resolved Hide resolved
src/management/projects.ts Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
- Adds support for multiple accounts via new commands `login:use`, `login:add` and `login:list`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a feature that could do with (a) documentation in the README and (b) documentation in FireSite

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @rachelsaunders I added a README section, can you take a look?

scripts/emulator-tests/functionsEmulator.spec.ts Outdated Show resolved Hide resolved
* Get the default account associated with a project directory, or the global default.
* @param projectDir the Firebase project directory.
*/
export function getProjectDefaultAccount(projectDir?: string | null): Account | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can projectDir really be null? What does that mean rather than just an empty string?

Copy link
Contributor Author

@samtstern samtstern Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from detectProjectRoot which sets options.projectRoot to string | null ... it's a nasty type but I want the code to be honest about what options.projectRoot (which is typed as any) can be and because detectProjectRoot is not always called both undefined and null are possible.

/**
* Get all authenticated accounts _besides_ the default account.
*/
export function getAdditionalAccounts(): Account[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm noticing that all these new functions are exported - is that necessary? Some of these seem like helpers...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're all used in at least one other file.

src/auth.ts Outdated Show resolved Hide resolved
src/commands/login-list.ts Outdated Show resolved Hide resolved
src/commands/login-list.ts Outdated Show resolved Hide resolved
src/commands/login-use.ts Outdated Show resolved Hide resolved
);
}

const projectDir = options.projectRoot as string | null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just options.projectRoot as string || ""? (I'm not a fan of using null if I can help it, as you may have guessed by now)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual type based on detectProjectRoot, the as is just because I don't want to use any.

src/test/multiAuth.spec.ts Outdated Show resolved Hide resolved
@samtstern samtstern requested a review from bkendall March 23, 2021 12:20
Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't hammer every corner, I'm sure, but I pulled this down and played with it a bit, init-ing and deploying things. Looks good to me!

@samtstern samtstern added cla: yes Manual indication that this has passed CLA. and removed cla: yes Manual indication that this has passed CLA. labels Apr 6, 2021
@samtstern samtstern merged commit 0dd344d into master Apr 6, 2021
@gavinsharp gavinsharp mentioned this pull request Apr 7, 2021
@bkendall bkendall deleted the ss-multi-auth branch August 4, 2021 19:27
devpeerapong pushed a commit to devpeerapong/firebase-tools that referenced this pull request Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
3 participants