Use Admin AuthTokenProvider when targeting Emulator #3228

schmidt-sebastian · 2020-06-17T18:17:33Z

Fixes #3144

google-oss-bot · 2020-06-17T18:27:51Z

Binary Size Report

Affected SDKs

@firebase/database

Type Base (b07f822) Head (aa5ee6c) Diff

browser 268 kB 269 kB +848 B (+0.3%)

esm2017 235 kB 236 kB +523 B (+0.2%)

main 269 kB 270 kB +848 B (+0.3%)

module 267 kB 268 kB +843 B (+0.3%)
firebase

Type Base (b07f822) Head (aa5ee6c) Diff

firebase-database.js 187 kB 187 kB +18 B (+0.0%)

firebase.js 819 kB 819 kB +18 B (+0.0%)

Test Logs

Base (b07f822b): https://github.com/firebase/firebase-js-sdk/actions/runs/164843010
Head (aa5ee6c9): https://github.com/firebase/firebase-js-sdk/actions/runs/167658411

yuchenshi · 2020-06-17T19:59:19Z

I'll be brief here. I believe this will break two use cases:

Admin SDK authenticating as Admin (where Authorization: Bearer owner must be specified, instead of empty.)
Developers using production Firebase Auth to login a production user and then write to local RTDB emulator using the production JWT.

Feel free to correct me if I'm wrong, but I may be overly cautious since we had a similar change before that broke admin and firebase-testing. If you can correct me by testing admin, production auth, and @firebase/testing, that will be even better. Bonus points if those tests can be automated.

schmidt-sebastian · 2020-06-18T19:23:53Z

@yuchenshi Thanks for your comment. I am kind of stuck here now, but will try to come up with a way to address your concerns. I hope we can do more than just silence the logs.

filipesilva · 2020-06-26T16:23:33Z

@schmidt-sebastian did you perhaps come across a workaround that could be used meanwhile? It looks like this fix ended up being a bit more complicated than it looked like. I'm happy enough working around it on my side for the time being, if that's an option.

schmidt-sebastian · 2020-06-26T17:00:55Z

I haven't been able to come up with a way to solve this - as far as I can tell, we might just have to disable the log output. Does anything actually break on your end, or are you mostly (and rightfully) concerned about the log output?

filipesilva · 2020-06-26T17:21:29Z

Anything that involves using recent emulator version with RTDB and without having GOOGLE_APPLICATION_CREDENTIALS set will fail. It's not logging, it's that RTDB doesn't work at all.

In the original repro in #3144, the cloud fn times out with RTDB, but not with Firestore, because of this problem. On our setup we're stuck on firebase-tools@7.12.1 and can't update because of it. For some reason older emulator versions are ok.

samtstern · 2020-07-07T17:24:28Z

@schmidt-sebastian do we have any way to detect if we're in an Admin or Client context? If so the behavior matrix looks like this:

In client context...
- Host is emulator: use Firebase Auth
- Host is prod: use Firebase Auth
In admin context...
- Host is emulator: use EmptyTokenProvider that always returns exactly owner
- Host is prod: use standard cloud auth

changeset-bot · 2020-07-07T22:22:41Z

🦋 Changeset is good to go

Latest commit: 004f58d

We got this.

This PR includes changesets to release 8 packages

Name	Type
firebase	Patch
@firebase/database	Patch
@firebase/testing	Patch
firebase-browserify-test	Patch
firebase-package-typings-test	Patch
firebase-messaging-selenium-test	Patch
firebase-typescript-test	Patch
firebase-webpack-test	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

schmidt-sebastian

Sam - this might be ready for another review. It at least solves the reported problem. I hope it causes not too many others.

samtstern

This looks 99% correct now but I have one more question.

samtstern · 2020-07-08T12:36:45Z

packages/database/src/core/RepoManager.ts

@@ -116,6 +123,11 @@ export class RepoManager {
      dbUrl = `http://${dbEmulatorHost}?ns=${repoInfo.namespace}`;


Does this branch if (dbEmulatorHost) { } trigger only if the emulator connection is made via env var? What about if the user does this:

firebase.initializeApp({ databaseURL: 'http://localhost:9000?ns=namespace' })

You might consider using the EmulatorAdminTokenProvider any time Constants.NODE_ADMIN is true and the database is http:// (not https://). That's what we do in the Firebase CLI, it catches more cases and it also has the nice side benefit of not allowing someone's admin credentials to ever go to an insecure host.

yuchenshi

Code LGTM this time and please address Sam's latest comment.

However, I'm still concerned about test coverage. Are the cases (and the awesome matrix Sam has put together) manually tested, unit tested, or integration tested?

…e-js-sdk into mrschmidt/tokenprovider

samtstern · 2020-07-09T19:29:19Z

packages/database/src/core/RepoManager.ts

      dbUrl = `http://${dbEmulatorHost}?ns=${repoInfo.namespace}`;
      parsedUrl = parseRepoInfo(dbUrl);
      repoInfo = parsedUrl.repoInfo;
+    } else {
+      isEmulator =
+        parsedUrl.repoInfo.host === 'localhost' && !parsedUrl.repoInfo.secure;


We do see people running their emulator on non-localhost hosts. Like 0.0.0.0. I think just anything non-secure (http://) is the right condition.

Done. I will verify this manually on Monday, which should allow us to release this next week.

samtstern

One final comment about non-localhost emulators.

filipesilva · 2020-07-13T08:22:08Z

@schmidt-sebastian if there's a snapshot build somewhere that I can pull in via npm I can test on my setup too.

schmidt-sebastian · 2020-07-13T18:31:14Z

I manually verified using Sam's testing matrix.

@filipesilva We will publish a prerelease version on NPM, likely tomorrow.

samtstern · 2020-07-15T13:34:43Z

So it looks like firebase-admin-node 9.0.0 went out yesterday ... does that mean this bugfix missed the last 8.x release?

schmidt-sebastian · 2020-07-15T18:13:44Z

@samtstern firebase-admin-node 8.x should pick this up on re-install since we don't pin versions.

Use empty auth token provider when talking to Emulator

9d69fd9

schmidt-sebastian requested a review from jsdt as a code owner June 17, 2020 18:17

schmidt-sebastian changed the title ~~Use empty auth token provider when talking to Emulator~~ Use empty AuthTokenProvider when targeting Emulator Jun 17, 2020

schmidt-sebastian assigned yuchenshi and jsdt Jun 17, 2020

schmidt-sebastian mentioned this pull request Jun 18, 2020

Unified emulator settings for Database firebase/firebase-android-sdk#1672

Merged

1 task

schmidt-sebastian unassigned jsdt and yuchenshi Jun 18, 2020

samtstern requested a review from yuchenshi July 7, 2020 16:48

Another attempt

88e8f22

schmidt-sebastian force-pushed the mrschmidt/tokenprovider branch 2 times, most recently from 191af58 to d67005b Compare July 8, 2020 03:58

Cleanup + Sam's constants

84a9b29

schmidt-sebastian force-pushed the mrschmidt/tokenprovider branch from d67005b to 84a9b29 Compare July 8, 2020 04:02

schmidt-sebastian changed the title ~~Use empty AuthTokenProvider when targeting Emulator~~ Use Admin AuthTokenProvider when targeting Emulator Jul 8, 2020

schmidt-sebastian commented Jul 8, 2020

View reviewed changes

schmidt-sebastian added 3 commits July 7, 2020 21:08

Create smooth-poets-leave.md

f891b19

Update smooth-poets-leave.md

7a19477

Update smooth-poets-leave.md

617dd05

schmidt-sebastian assigned samtstern Jul 8, 2020

samtstern reviewed Jul 8, 2020

View reviewed changes

yuchenshi approved these changes Jul 8, 2020

View reviewed changes

samtstern mentioned this pull request Jul 9, 2020

Migrate testing to rules-unit-testing #3378

Merged

5 tasks

schmidt-sebastian added 2 commits July 9, 2020 11:54

Also support localhost

8a978d8

Merge branch 'mrschmidt/tokenprovider' of github.com:firebase/firebas…

54c19f1

…e-js-sdk into mrschmidt/tokenprovider

samtstern reviewed Jul 9, 2020

View reviewed changes

samtstern approved these changes Jul 9, 2020

View reviewed changes

schmidt-sebastian added 2 commits July 10, 2020 15:47

Simplify

f0ea3f0

Merge branch 'master' into mrschmidt/tokenprovider

6ba08b7

Lint

004f58d

schmidt-sebastian assigned jsdt and unassigned samtstern Jul 13, 2020

jsdt approved these changes Jul 14, 2020

View reviewed changes

hsubox76 self-requested a review July 14, 2020 20:53

hsubox76 approved these changes Jul 14, 2020

View reviewed changes

hsubox76 merged commit c2b737b into master Jul 14, 2020

google-oss-bot mentioned this pull request Jul 15, 2020

Version Packages #3411

Merged

Feiyang1 mentioned this pull request Jul 16, 2020

Use custom changelog generator #3415

Merged

google-oss-bot mentioned this pull request Jul 16, 2020

Version Packages #3421

Merged

schmidt-sebastian deleted the mrschmidt/tokenprovider branch July 17, 2020 01:40

firebase locked and limited conversation to collaborators Aug 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Admin AuthTokenProvider when targeting Emulator #3228

Use Admin AuthTokenProvider when targeting Emulator #3228

schmidt-sebastian commented Jun 17, 2020

google-oss-bot commented Jun 17, 2020 •

edited

Loading

`@firebase/database`

`firebase`

yuchenshi commented Jun 17, 2020

schmidt-sebastian commented Jun 18, 2020

filipesilva commented Jun 26, 2020

schmidt-sebastian commented Jun 26, 2020

filipesilva commented Jun 26, 2020 •

edited

Loading

samtstern commented Jul 7, 2020

changeset-bot bot commented Jul 7, 2020 •

edited

Loading

schmidt-sebastian left a comment

samtstern left a comment

samtstern Jul 8, 2020

yuchenshi left a comment

samtstern Jul 9, 2020

schmidt-sebastian Jul 10, 2020

samtstern left a comment

filipesilva commented Jul 13, 2020

schmidt-sebastian commented Jul 13, 2020

samtstern commented Jul 15, 2020

schmidt-sebastian commented Jul 15, 2020

		@@ -116,6 +123,11 @@ export class RepoManager {
		dbUrl = `http://${dbEmulatorHost}?ns=${repoInfo.namespace}`;

Use Admin AuthTokenProvider when targeting Emulator #3228

Use Admin AuthTokenProvider when targeting Emulator #3228

Conversation

schmidt-sebastian commented Jun 17, 2020

google-oss-bot commented Jun 17, 2020 • edited Loading

Binary Size Report

Affected SDKs

@firebase/database

firebase

Test Logs

yuchenshi commented Jun 17, 2020

schmidt-sebastian commented Jun 18, 2020

filipesilva commented Jun 26, 2020

schmidt-sebastian commented Jun 26, 2020

filipesilva commented Jun 26, 2020 • edited Loading

samtstern commented Jul 7, 2020

changeset-bot bot commented Jul 7, 2020 • edited Loading

🦋 Changeset is good to go

schmidt-sebastian left a comment

Choose a reason for hiding this comment

samtstern left a comment

Choose a reason for hiding this comment

samtstern Jul 8, 2020

Choose a reason for hiding this comment

yuchenshi left a comment

Choose a reason for hiding this comment

samtstern Jul 9, 2020

Choose a reason for hiding this comment

schmidt-sebastian Jul 10, 2020

Choose a reason for hiding this comment

samtstern left a comment

Choose a reason for hiding this comment

filipesilva commented Jul 13, 2020

schmidt-sebastian commented Jul 13, 2020

samtstern commented Jul 15, 2020

schmidt-sebastian commented Jul 15, 2020

google-oss-bot commented Jun 17, 2020 •

edited

Loading

`@firebase/database`

`firebase`

filipesilva commented Jun 26, 2020 •

edited

Loading

changeset-bot bot commented Jul 7, 2020 •

edited

Loading