Strict credential authentication in tests #2368
Conversation
erikbern
force-pushed
the
erikbern/test-auth
branch
from
608d00c to
b0dfbca
Compare
erikbern
force-pushed
the
erikbern/test-auth
branch
from
fa837c4 to
352f606
Compare
erikbern
changed the title
There was a problem hiding this comment.
One thing that isn't obvious to me here is when we need the token_env fixture.
Open question: should we create a random token for each test? That wouldn't be significantly harder and maybe it's an even stronger test.
Yeah — this feels like it might be worth doing? We've had weird state-spillover issues with the mocking in the past that have caused and/or masked issues.
Mostly when we have subprocesses (since they inherit env from the parent) but arguably it would be cleaner to make that explicit
Let me see if it's easy to do. There's a risk it's hard but I'll try in a separate PR. |
|
Thanks @mwaskom |
* Check credentials in unit tests * Fix e2e_test * Fix container_app_test * Fix container_test * Better event handler * Fix token flow tests * Fix client_test * Fix grpc_utils_test * Fix live_reload_test * Fix cli_test * Fix mounted_files_test * Fix config_test * Fix shutdown_test * Fix image_test * Fix? fork_test * Use enums instead of hardcoded integers * typo
….-prefixed ancestor dirs (#2366) * Fix mounting of packages from .venv not working due to over-eager module_mount_condition * Adds test * Fixes and debug logging * Add test to deploy from within container (#2365) * [auto-commit] [skip ci] Bump the build number * Miscellaneous cleanup of `@app.cls` code (#2362) * [auto-commit] [skip ci] Bump the build number * Add _experimental_spawn function (#2354) * Add _experimental_spawn function * Add type to signature * Address PR comments * [auto-commit] [skip ci] Bump the build number * Move the authentication check during running/deploy earlier (#2370) * [auto-commit] [skip ci] Bump the build number * Strict credential authentication in tests (#2368) * Check credentials in unit tests * Fix e2e_test * Fix container_app_test * Fix container_test * Better event handler * Fix token flow tests * Fix client_test * Fix grpc_utils_test * Fix live_reload_test * Fix cli_test * Fix mounted_files_test * Fix config_test * Fix shutdown_test * Fix image_test * Fix? fork_test * Use enums instead of hardcoded integers * typo * [auto-commit] [skip ci] Bump the build number * Use unique server credentials in every unit test (#2372) Use per-test credentials * [auto-commit] [skip ci] Bump the build number * use a container client for test_container_debug_snapshot (#2373) * [auto-commit] [skip ci] Bump the build number * Fix container app tests (part 2) – use the right client type (#2376) Fix container app tests * [auto-commit] [skip ci] Bump the build number * Extend test --------- Co-authored-by: Erik Bernhardsson <[email protected]> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Deven Navani <[email protected]> Co-authored-by: Ryan Culbertson <[email protected]> Co-authored-by: Erik Bernhardsson <[email protected]>
This makes the credential authentication in tests a lot more strict – it checks the token and secret on every request. Basically what the real production API already does.
This change will make it easier to remove
ClientHellowhile retain tests for most/all of the logic (since we're going to rely on grpc errors from other routes instead).Broken out of #2361 which conflated a bunch of different changes. Better to make tests stricter first.
Ended up being a much bigger change than I anticipated (lots of test that relied on no authentication).
Open question: should we create a random token for each test? That wouldn't be significantly harder and maybe it's an even stronger test.