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

use fake "owner" credential when rtdb emulator env var is set #2029

Merged
merged 15 commits into from
Aug 9, 2019

Conversation

jmwski
Copy link
Contributor

@jmwski jmwski commented Jul 27, 2019

Today, the RTDB emulator accepts any production access token. We would like to support fully-offline emulator interaction that doesn't require an external auth provider. Some questions to follow up on:

  • Should we store a bit describing whether the SDK is talking to an emulator in the RepoInfo class?
  • Should we only infer that we're talking to an emulator if the environment variable is set? Can we infer it in the additional situation in which an ns query parameter is needed (RepoInfo#needsQueryParam)?

@mikelehen
Copy link
Contributor

Hrm... I guess I have several interrelated questions...

  1. What is the overall user experience we're trying to build here? I'm surprised that we want to always communicate with the emulator as "owner". Don't we want to be able to test security rules, etc. which will require authenticating with different credentials?
  2. There are a couple of existing "hooks" to replace auth already. FirebaseAuth hooks into FirebaseApp like (this)[https://github.com/firebase/firebase-js-sdk/blob/8271edfb49a17082c379bae160a0ebcbfbe215cb/packages/auth/src/exports_auth.js#L618] to provide auth functionality. And RTDB wraps FirebaseApp's token fetching capabilities in its own abstraction:
    * Abstraction around FirebaseApp's token fetching capabilities.
    . Would it make sense to modify / replace this AuthTokenProvider to incorporate whatever emulator functionality we're aiming for?
  3. The Cloud Firestore SDK already has emulator support. Are we doing something similar there? Or is RTDB different for some reason?

@jmwski
Copy link
Contributor Author

jmwski commented Jul 29, 2019

Hrm... I guess I have several interrelated questions...

  1. What is the overall user experience we're trying to build here? I'm surprised that we want to always communicate with the emulator as "owner". Don't we want to be able to test security rules, etc. which will require authenticating with different credentials?
  2. There are a couple of existing "hooks" to replace auth already. FirebaseAuth hooks into FirebaseApp like (this)[https://github.com/firebase/firebase-js-sdk/blob/8271edfb49a17082c379bae160a0ebcbfbe215cb/packages/auth/src/exports_auth.js#L618] to provide auth functionality. And RTDB wraps FirebaseApp's token fetching capabilities in its own abstraction:
    * Abstraction around FirebaseApp's token fetching capabilities.

    . Would it make sense to modify / replace this AuthTokenProvider to incorporate whatever emulator functionality we're aiming for?
  3. The Cloud Firestore SDK already has emulator support. Are we doing something similar there? Or is RTDB different for some reason?

The use case we want to support here is to enable the emulators in environments where we can't get a production access token (e.g., CI). The issue I'm trying to work around in this PR is that we're using a single global application credential for all emulators.

I just talked offline with @ryanpbrewster and @yuchenshi and I think one path forward is to enforce the following policy in the admin sdk: if either FIRESTORE_EMULATOR_HOST or FIREBASE_DATABASE_EMULATOR_HOST is set, we default to using a single "fake" credential globally. We are still discussing this, but if we decide to move forward, I will restore this PR in the admin sdk and get rid of this one.

Apologies for the back-and-forth on this.

@mikelehen
Copy link
Contributor

Per chat discussion, if we move forward with this approach, I think the special handling for RTDB should probably live in a new EmulatorAuthTokenProvider class akin to

export class AuthTokenProvider {
that just returns "owner" or whatever and swap it in when the env var is set.

@jmwski jmwski changed the title omit access token exchange if we're talking to an emulator use fake "owner" credential when rtdb emulator env var is set Aug 1, 2019
@mikelehen
Copy link
Contributor

When this is ready for another review, can you please assign it to me?

@jmwski jmwski assigned mikelehen and unassigned jmwski and yuchenshi Aug 6, 2019
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

I think this mostly LGTM but a couple minor suggestions. Let me know what you think.

packages/database/src/core/Constants.ts Outdated Show resolved Hide resolved
packages/database/src/core/Constants.ts Outdated Show resolved Hide resolved
packages/database/src/core/AuthTokenProvider.ts Outdated Show resolved Hide resolved
@mikelehen mikelehen assigned jmwski and unassigned mikelehen Aug 6, 2019
@jmwski jmwski assigned mikelehen and unassigned jmwski Aug 7, 2019
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

LGTM with one final nit. Thanks!

packages/database/src/core/ReadonlyRestClient.ts Outdated Show resolved Hide resolved
@mikelehen mikelehen removed their assignment Aug 7, 2019
@jmwski jmwski merged commit 4ebedd0 into master Aug 9, 2019
mikelehen pushed a commit that referenced this pull request Aug 12, 2019
* omit access token exchange if talking to an emulator

* [AUTOMATED]: Prettier Code Styling

* create TokenProvider interface and add EmulatorAuthTokenProvider

* [AUTOMATED]: Prettier Code Styling

* Update packages/database/src/core/EmulatorAuthTokenProvider.ts

Co-Authored-By: Yuchen Shi <yuchenshi@google.com>

* break circular dependency by extracting env var constant

* [AUTOMATED]: Prettier Code Styling

* rename AuthTokenProvider -> FirebaseAuthTokenProvider etc.

* [AUTOMATED]: Prettier Code Styling

* fix ReadOnlyRestClient constructor comment

* poke travis CI

* remove unnecessary js docs
@firebase firebase locked and limited conversation to collaborators Oct 9, 2019
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.

4 participants