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

feat(auth): Recognize FIREBASE_AUTH_EMULATOR_HOST environment variable #414

Merged
merged 8 commits into from
Jan 15, 2021
Merged

feat(auth): Recognize FIREBASE_AUTH_EMULATOR_HOST environment variable #414

merged 8 commits into from
Jan 15, 2021

Conversation

maku693
Copy link
Contributor

@maku693 maku693 commented Dec 18, 2020

If FIREBASE_AUTH_EMULATOR_HOST is set, auth API request will be sent to the host according to the environment variable.

Discussion

#409

Reflecting the comment on the issue, the behavior of signers and token verifiers are unchanged.

Now the signer is replaced to emulatedSigner, and token verifiers panic when the environment variable is set. #414 (comment) #414 (comment)

RELEASE NOTE: The Admin SDK now supports the Firebase emulator. Developers can configure the SDK to run against the emulator by setting the FIREBASE_AUTH_EMULATOR_HOST environment variable.

option.WithoutAuthentication(),
},
}
client, err := NewClient(context.Background(), conf)
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'm not sure using no-auth config is appropriate for this test.

}
if umc.providerConfigEndpoint != providerConfigEndpoint {
return fmt.Errorf("providerConfigEndpoint = %q; want = %q", umc.providerConfigEndpoint, providerConfigEndpoint)
if baseClient.tenantMgtEndpoint != defaultIDToolkitV2Beta1Endpoint {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the check for tenantMgtEndpoint.

@maku693 maku693 marked this pull request as ready for review December 18, 2020 17:39
@samtstern
Copy link
Contributor

samtstern commented Dec 21, 2020

@maky693 thank you for the contribution! General approach LGTM but assigning to @hiranya911 for Go / Admin SDK review.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM. Only question is should we release this without support for emulating token verification?

auth/auth.go Outdated Show resolved Hide resolved
@dgpc
Copy link

dgpc commented Jan 5, 2021

@hiranya911 - as a user waiting for this feature, my 2c worth is that although token verification emulation is necessary for complete end-to-end testing, making it possible to talk to the emulator at all in this manner is still useful to release as-is, since it unblocks testing of a variety of workflows

@Michaelhobo
Copy link

@hiranya911 - as a user waiting for this feature, my 2c worth is that although token verification emulation is necessary for complete end-to-end testing, making it possible to talk to the emulator at all in this manner is still useful to release as-is, since it unblocks testing of a variety of workflows

Agreed, this would be a huge win for me.

@yuchenshi
Copy link
Member

yuchenshi commented Jan 5, 2021

I'd propose a middle ground where we release a version first, where everything works but token verification panics with "not implemented" for now when the env var is set. This should ensure that SDK don't call production APIs in emulator mode and also minimize any confusion caused by the SDK verifying signatures using production public keys. (This idea can also apply to the Python SDK, using thrown exceptions instead of panics.)

@samtstern
Copy link
Contributor

@yuchenshi I like that idea! SGTM

@hiranya911
Copy link
Contributor

Yeah, I like that too. Any chance we can get this PR updated to incorporate that change?

@yuchenshi yuchenshi assigned maku693 and unassigned samtstern Jan 6, 2021
@maku693
Copy link
Contributor Author

maku693 commented Jan 7, 2021

Thank you for review! I'll update this PR this weekend.

@@ -100,9 +100,10 @@ type tokenVerifier struct {
expiredTokenCode string
keySource keySource
clock internal.Clock
isEmulator bool
Copy link
Contributor Author

@maku693 maku693 Jan 10, 2021

Choose a reason for hiding this comment

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

This might be too simple. It's better to implement custom keySource for emulator mode.

But I don't sure custom keySource is necessary because we didn't decide how to verify tokens in the emulator mode yet.

@maku693
Copy link
Contributor Author

maku693 commented Jan 10, 2021

@yuchenshi Is the point of the change to prevent calling production APIs in emulator mode?
If so, not just the token verifier but also the signer and the tenant manager should panic at API calls, correct?

@yuchenshi
Copy link
Member

For createCustomToken etc., when FIREBASE_AUTH_EMULATOR_HOST is set, the SDK should create unsigned tokens (i.e. the signature part being an empty string), which are in turn accepted by the Auth Emulator. In other words, signer should be short-circuited and there should not be any outgoing network call at all (no signBlob etc.).

Auth Emulator doesn't support multi-tenancy (at least for now) and I'm indifferent whether the SDK should panic or forward them to the Auth Emulator which rejects such requests. I'm fine as long as they won't reach production.

@maku693
Copy link
Contributor Author

maku693 commented Jan 12, 2021

Ah, I understand. I'll update not to sign tokens.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

@yuchenshi wdyt?

@hiranya911 hiranya911 merged commit 8cd5093 into firebase:dev Jan 15, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Feb 4, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Feb 5, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Feb 5, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Feb 24, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Feb 24, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Feb 28, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Mar 5, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Mar 16, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Mar 16, 2021
muru added a commit to muru/firebase-admin-python that referenced this pull request Mar 20, 2021
hiranya911 pushed a commit to firebase/firebase-admin-python that referenced this pull request Mar 23, 2021
…HOST environment variable. (#531)

* Support auth emulator via FIREBASE_AUTH_EMULATOR_HOST

Modeled on firebase/firebase-admin-go#414

* Tests for emulator support in auth, user mgmt and token gen

To minimize modification of tests, the app fixture and instrumentation
have been modified to use a global dict of URLs, which are then
monkey-patched based on fixture parameters. Essentially, all tests using
the app fixture are run twice, once with the emulated endpoint and once
without.

* fallback for monkeypatch in python 3.5

* Token verification for the auth emulator

* Accommodate auth emulator behaviour in tests.

Where possible, tests are modified to account for the current behaviour
in emulator mode (e.g., invalid or expired tokens or cookies still
work).

Fixtures were changed to function scope to avoid problems caused by
overlap when some fixtures being in emulator mode and some in normal
mode concurrently.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants