cc @brianrodri who may be interested in this.

@yuchenshi At the time I started work on this I thought the Google people were going to rework how those are handled. Now that the firebase CLI is supposed to work OK with session cookies, I'll try to include it in integration tests (which I'd held off on) and see what else needs to be fixed here.

@yuchenshi At the time I started work on this I thought the Google people were going to rework how those are handled. Now that the firebase CLI is supposed to work OK with session cookies, I'll try to include it in integration tests (which I'd held off on) and see what else needs to be fixed here.

I really appreciate it. firebase/firebase-admin-node#1148 has some integration tests that you can draw inspiration from and it shows the desired behavior for verifying ID tokens.

@muru sorry for disturbance, but could you please take a look? I really wait for that MR =)

@Keramblock thanks for the nudge. I have fixed the issues noted by @hiranya911 , but haven't gotten the integration tests to work. I'll try those again over the weekend.

@muru the last set of commits have broken the CI. I'd recommend reverting the breaking commits, and doing the bare minimal necessary to address the earlier code review feedback. Any other new capabilities (e.g. emulator based testing), can be implemented in separate future PRs.

@hiranya911 porting the token verification code from firebase/firebase-admin-node#1148 breaks a few unit tests. Since that should probably be included with this PR, would it be fine if I marked those tests for skipping when tested with the emulator? And while I am it, I could do the same for integration tests too.

Ideally we shouldn't have to skip any unit tests.

We can live without emulator-based integration tests for now (that work is on hold in Node.js at firebase/firebase-admin-node#1155 due to some problems).

@hiranya911 what you say is correct, but shouldn't matter here, since emulation is set in the second/final iteration (https://github.com/firebase/firebase-admin-python/pull/531/files#diff-81622636c5d10b6ab40396c608887473086a43b94621195117193ab81a4b5cdaR166 - emulated is False first, then True).

These tests broke due to the changes made to token validation to fit the emulator (aacd27f) - they were supposed to raise an exception but didn't. Note that all tests ran perfectly fine before that.

I'm debugging the tests in your branch and I can clearly see that tests that shouldn't be running in the emulator mode, picking up the emulator mode from another fixture. The test case test_noncert_credential[user_mgt_app0] should pass by raising the expected error, but it doesn't because it sees the env variable set in a different fixture (i.e. the auth_app fixture), and goes into the emulator mode. The order of events observed is something like:

auth_app fixture initialized with emulated = False
  all tests that depend on auth_app runs
auth_app fixture cleaned up
auth_app fixture initialized with emulated = True
  all tests that depend on auth_app runs
user_mgt_app fixture initialized with emulated = False
  all tests that depend on user_mgt_app runs (These tests see the env variable set by auth_app, since it's not cleaned up yet)
user_mgt_app fixture cleaned up
user_mgt_app fixture initialized with emulated = True
  all tests that depend on user_mgt_app runs
user_mgt_app fixture cleaned up
auth_app fixture cleaned up

Basically we can no longer reason about when a test case may or may not see the env variable, since other fixtures may be running in the background, waiting to be cleaned up at the end of the module.

Ah, I hadn't noticed that, thanks! I'll look into how that can be fixed - maybe have an existing value of the environment variable override the fixture parameters.

I think the easiest solution would be to change the fixture scope to function (which is also pytest default). That will cause a new app and a monkeypatch instance to be created per test function, and they will get cleaned up immediately after. App initialization and clean up are pretty cheap operations, so it should be ok in terms of speed.

With function-scoped fixtures (plus a few other minor modifications), these are all the remaining test failures:

FAILED tests/test_token_gen.py::TestCreateCustomToken::test_noncert_credential[user_mgt_app1] - Failed: DID NOT RAISE <class 'ValueError'>
FAILED tests/test_token_gen.py::TestVerifyIdToken::test_revoked_token_check_revoked[user_mgt_app1-BinaryToken] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.Revoke...
FAILED tests/test_token_gen.py::TestVerifyIdToken::test_revoked_token_check_revoked[user_mgt_app1-TextToken] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.RevokedI...
FAILED tests/test_token_gen.py::TestVerifyIdToken::test_invalid_token[user_mgt_app1-NoKid] - Failed: DID NOT RAISE <class 'firebase_admin._auth_utils.InvalidIdTokenError'>
FAILED tests/test_token_gen.py::TestVerifyIdToken::test_invalid_token[user_mgt_app1-WrongKid] - Failed: DID NOT RAISE <class 'firebase_admin._auth_utils.InvalidIdTokenError'>
FAILED tests/test_token_gen.py::TestVerifyIdToken::test_invalid_token[user_mgt_app1-FutureToken] - Failed: DID NOT RAISE <class 'firebase_admin._auth_utils.InvalidIdTokenError'>
FAILED tests/test_token_gen.py::TestVerifyIdToken::test_invalid_token[user_mgt_app1-ExpiredToken] - Failed: DID NOT RAISE <class 'firebase_admin._auth_utils.InvalidIdTokenError'>
FAILED tests/test_token_gen.py::TestVerifyIdToken::test_expired_token[user_mgt_app1] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.ExpiredIdTokenError'>
FAILED tests/test_token_gen.py::TestVerifyIdToken::test_certificate_request_failure[user_mgt_app1] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.CertificateFetchEr...
FAILED tests/test_token_gen.py::TestVerifySessionCookie::test_invalid_cookie[user_mgt_app1-NoKid] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.InvalidSessionCooki...
FAILED tests/test_token_gen.py::TestVerifySessionCookie::test_invalid_cookie[user_mgt_app1-WrongKid] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.InvalidSessionCo...
FAILED tests/test_token_gen.py::TestVerifySessionCookie::test_invalid_cookie[user_mgt_app1-FutureCookie] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.InvalidSessi...
FAILED tests/test_token_gen.py::TestVerifySessionCookie::test_invalid_cookie[user_mgt_app1-ExpiredCookie] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.InvalidSess...
FAILED tests/test_token_gen.py::TestVerifySessionCookie::test_expired_cookie[user_mgt_app1] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.ExpiredSessionCookieError'>
FAILED tests/test_token_gen.py::TestVerifySessionCookie::test_certificate_request_failure[user_mgt_app1] - Failed: DID NOT RAISE <class 'firebase_admin._token_gen.CertificateF...
FAILED tests/test_token_gen.py::TestCertificateCaching::test_certificate_caching[user_mgt_app1] - assert 0 == 1

It's unfortunate our implementation doesn't handle expired emulated tokens correctly (this is because we bypass the JWT verification step entirely for emulated tokens). But this might be ok. I'm fine with skipping the above tests if we must.

@hiranya911 I removed the integration test and modified the fixtures to use the function scope, and where possible, I modified the tests to account for the current behaviour of the emulator mode instead of skipping them. If the emulator mode is improved, that might help prevent tests from being overlooked due to being skipped.

Also, I opted against using a module-level variable for _auth_utils.is_emulated() as it behaves like module fixture when a variable, causing test failures. Is that fine?