feat(auth): Add support for Firebase Authentication Emulator #289
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
Thanks @floppydisken for the PR. Can you please address the CLA issue and test failures reported by GitHub Actions? We can take a look then. |
|
@googlebot I signed it! |
Tests are now passing. However, I haven't done any tests against the emulator. Do you guys recommend an approach for doing so? |
|
I think at some point we can set up integration tests to run against the emulator. There's similar work happening in our other repos right now. But in the meantime we'll have to do with a combination of unit tests and other manual testing. |
There was a problem hiding this comment.
Thanks for the PR @floppydisken. The highlevel approach is correct. Just needs a bit of cleanup and some degree of unit test coverage.
FirebaseAdmin/FirebaseAdmin.Tests/Auth/IdToolkitHostResolverTest.cs
Outdated
Show resolved
Hide resolved
FirebaseAdmin/FirebaseAdmin.Tests/Auth/IdToolkitHostResolverTest.cs
Outdated
Show resolved
Hide resolved
FirebaseAdmin/FirebaseAdmin.Tests/Auth/IdToolkitHostResolverTest.cs
Outdated
Show resolved
Hide resolved
Add environment variable test that piggybacks on the already established FirebaseUserManagerTest. Also, to make it easier to access and set environment variables a convenience class has been made to help out. This class utilizes the neat builtin get and set mechanism provided by the C# language. By using this class the code somewhat documents itself and prevents stupid spelling mistakes that might occur when running code. Each environment variable can conveniently be documented inside the class.
The TestConfig was not being disposed of when expected resulting in the env variable being set in other tests as well and causing them to fail.
There was a problem hiding this comment.
Thanks @floppydisken. Just a few more things to address. But overall this is looking great!
FirebaseAdmin/FirebaseAdmin.Tests/Auth/Users/FirebaseUserManagerTest.cs
Outdated
Show resolved
Hide resolved
FirebaseAdmin/FirebaseAdmin.Tests/Auth/Users/FirebaseUserManagerTest.cs
Outdated
Show resolved
Hide resolved
| /// Gets or sets environment variable for connecting to the Firebase Authentication Emulator. | ||
| /// </summary> | ||
| /// <value>string in the form <host>:<port>, e.g. localhost:9099. Beware: No validation is done.</value> | ||
| internal static string FirebaseAuthEmulatorHost |
There was a problem hiding this comment.
This is a pretty neat trick, and I do appreciate the power it brings. It basically puts an env variable lookup behind a typesafe constraint that can be checked by the compiler.
However, this is a bit too exposed right now. So:
- let's move this constant into the new
Utilsclass (and remove this class entirely), so it's scoped to the Auth namespace, and - remove the setter (it's potentially dangerous to have an API that updates env variables lying around)
There was a problem hiding this comment.
let's move this constant into the new Utils class (and remove this class entirely), so it's scoped to the Auth namespace, and
I was actually thinking the class to also provide access to the "GOOGLE_CLOUD_HOST" and "GCLOUD_HOST" environment variables as well as other variables. I am quite unaware of the amount of env variables used for the Firebase client libraries, so I don't know if it's a good idea. However, it would make sense that this particular environment variable is scoped to the Auth namespace.
(and remove this class entirely)
Removing the class itself, would result in naming like FirebaseAuthHostEnvironmentVariable instead of EnvironmentVariable.FirebaseAuthHost, which I see no advantage to. Am I misunderstanding you perhaps?
There was a problem hiding this comment.
Proposed change in 85ee525
There was a problem hiding this comment.
Removing the class itself, would result in naming like FirebaseAuthHostEnvironmentVariable instead of EnvironmentVariable.FirebaseAuthHost, which I see no advantage to. Am I misunderstanding you perhaps?
My reasoning is that this class will end up being a wrapper for a single constant. Therefore it's just simpler to use the Utils class itself as the wrapper, and rename the constant to something like EmulatorHostEnvironmentVar.
There was a problem hiding this comment.
FirebaseAdmin/FirebaseAdmin.Tests/Auth/Users/FirebaseUserManagerTest.cs
Outdated
Show resolved
Hide resolved
FirebaseAdmin/FirebaseAdmin.Tests/Auth/Users/FirebaseUserManagerTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
We seem to have overlooked a critical element. I checked out your branch, and tried to run some of the integration tests against the emulator, and immediately ran into a few issues. The main problem is the credentials. Whenever we operate in the emulator mode, we should ditch the credentials injected via FirebaseApp, and use a fake credential. The fake credential should always return the fixed access token string "owner".
In C# it's pretty simple to construct this fake credential:
GoogleCredential.FromAccessToken("owner");You just need to read the env variable, and swap the credentials with the above in FirebaseUserManager. We can also add another helper function to Utils to handle this:
internal static GoogleCredential ResolveCredentials(GoogleCredential original)
{
if (emulator_env_var_set)
{
return GoogleCredential.FromAccessToken("owner");
}
return original;
}
Can we incorporate this change into this PR?
|
Hi @yuchenshi. I was trying to run some of our integration tests against the emulator, and encountered some errors (10 out of 30 in It seems the emulator has set the LastSignInTimestamp on some user accounts that were freshly created. Also do you know whether the emulator supports the following APIs? private const string EmailLinkSignInUrl =
"https://www.googleapis.com/identitytoolkit/v3/relyingparty/emailLinkSignin";
private const string ResetPasswordUrl =
"https://www.googleapis.com/identitytoolkit/v3/relyingparty/resetPassword";
private const string VerifyCustomTokenUrl =
"https://www.googleapis.com/identitytoolkit/v3/relyingparty/verifyCustomToken";
private const string VerifyPasswordUrl =
"https://www.googleapis.com/identitytoolkit/v3/relyingparty/verifyPassword";We call these APIs during integration tests, and in the emulator mode we'd want those calls directed to the emulator. |
|
I don't have bandwidth right now to look at test failures, but here's some responses to the questions and hopefully that helps.
I've noticed this as well but I cannot figure out what the right behavior should be. I'd appreciate it if you can help to pin this down in the form of a decision tree on when this should be set/updated or some test cases (even in pseudo code).
All of these are supported under the emulator prefix, e.g. |
That's probably a question for somebody like @bojeil-google. The only suggestion I can offer (purely based on intuition) is that if a user has never signed in, the emulator shouldn't set the lastSignInTimestamp on the user record. This is especially true for freshly created user accounts.
That's awesome! We just need to make some minor modifications to the tests in that case. |
@hiranya911 suggestion is a good rule of thumb. |
|
Adding the latest integration test report (with emulator) for future reference: At a high-level all the traffic during tests is directed to the emulator, which is great. The test failures, as far as I can tell are due to one of three reasons:
Note that (2) is an action item for this repo, and something we should get addressed before going into a release. |
There was a problem hiding this comment.
Thanks @floppydisken. This looks great, and my last integration test run with the emulator was mostly positive (see my previous comment). I just have one more comment for you, and once that's addressed I will merge this PR into a separate development branch. Then we can continue to develop the emulator support for CreateCustomToken and VerifyIdToken APIs on that branch, before merging the whole thing into the main branch.
|
Thanks @floppydisken for patiently working through all the comments. We will continue to develop this feature on the |
* feat(auth): Add support for Firebase Authentication Emulator (#289) * Add IdToolkitHostResolver with tests * Add resolution for versions of the API with tests * Simplify and reduce IdToolkitHostResolver to a Util method * Add TestConfig for testing against FirebaseEmulatorHost Add environment variable test that piggybacks on the already established FirebaseUserManagerTest. Also, to make it easier to access and set environment variables a convenience class has been made to help out. This class utilizes the neat builtin get and set mechanism provided by the C# language. By using this class the code somewhat documents itself and prevents stupid spelling mistakes that might occur when running code. Each environment variable can conveniently be documented inside the class. * Add license for new Utils class file * Add documentation for ResolveIdToolkitHost with examples * Use the EnvironmentVariable class to access env variables * Fix formatting * Use inheritance instead of TestConfig to set FirebaseUserManager tests The TestConfig was not being disposed of when expected resulting in the env variable being set in other tests as well and causing them to fail. * Cleanup and rename variable to more meaningful name * Move Utils and EnvironmentVariable class to Auth namespace * Move emulatorHostEnvVar to where it's used for readability * Rename UtilTest to more appropriate name * Rename expected...Host to expected...Url * Use the FirebaseAuthEmulatorHostName const instead of string * Add missing license head * Add missing util import and fix stylecop complaints * Rename AuthUtil to Util and move to Auth test folder * Simplify emulator host environment variable to be a constant in Utils * Use string literal for emulator host in FirebaseUserManagerTest In case a constant values is changed such as the used environment variable, this will cause the tests to fail, which is good to know. * Remove unnecessary tests from FirebaseAuthTest * Use string literal to ensure test fails if constant is changed * Remove the environment simplification since there is only one callsite * Cleanup GetIdToolkitHost documentation * Correct typo in file name. Util -> Utils. * Rename test cass to UtilsTest * Add tenant resolution to GetIdToolkitHost * Add function for resolving GoogleCredentials when in emulator mode * Add test for tenant url resolution * Use if statement instead of ternary for tenantIdPath * Better formatting for GetIdToolkitHost * Add periods to the end of documentation strings * Remove nullable and provide empty string instead * Change to only have one single line at end * Add convenience funcs for getting and checking emulator host env variable * Simplify GetEmulatorHost name * fix(auth): Setting EmulatorHost programatically (#290) * fix(auth): Setting EmulatorHost programatically * fix: Cleaned up tests * fix: Cleaned up user manager tests * feat(auth): Supporting VerifyIdToken() API with emulator (#291) * feat(auth): Emulator support for CreateCustomTokenAsync() API (#293) * feat(auth): Emulator support for CreateCustomTokenAsync() API * fix: Cleaned up unit tests * fix: Removed extra punctuation * feat(auth): Adding emulator support to TenantManager API (#294) * feat(auth): Adding emulator support to TenantManager API * fix: Updated test to check for credentials Co-authored-by: Bjarke <work.bjarke@gmail.com>
Adds support for Firebase Authentication Emulator.
Heavily inspired by the Java version https://github.com/firebase/firebase-admin-java/pull/510/files
I really don't know the scope of adding emulation support for all features but I thought Authentication would be a start (and I need it for my project).