🐛 Fix FASTAPI_CLOUD_TOKEN always overrides user token#174
🐛 Fix FASTAPI_CLOUD_TOKEN always overrides user token#174YuriiMotov wants to merge 18 commits intomainfrom
FASTAPI_CLOUD_TOKEN always overrides user token#174Conversation
ae3e143 to
092f407
Compare
|
|
||
| def is_expired(self) -> bool: | ||
| if not self.token: | ||
| if self._auth_mode != "user": # pragma: no cover # Should never happen |
There was a problem hiding this comment.
We can add unit tests for this class and cover this case there
deploy commandFASTAPI_CLOUD_TOKEN always overrides user token
| identity = Identity() | ||
| # Duplicate context initialization here to make `fastapi deploy` command work | ||
| # (callback doesn't take effect in this case) | ||
| ctx.initialize(prefer_auth_mode="token") |
There was a problem hiding this comment.
For fastapi cloud ... commands context will be initialized by callback, but it doesn't work for fastapi ... commands (can't use callback when app is added without name).
To fix this we need to duplicate this initialization in commands that are used on top level (fastapi login and fastapi deploy).
Not sure if we can handle this in a better way...
| def init_sentry() -> None: | ||
| """Initialize Sentry error tracking only if user is logged in.""" | ||
| identity = Identity() | ||
| identity = Identity( |
There was a problem hiding this comment.
Here we can't use ctx.get_identity() as this function is called at import time
| @pytest.fixture(autouse=True) | ||
| def unset_env_vars(monkeypatch: pytest.MonkeyPatch) -> Generator[None, None, None]: | ||
| """Fixture to unset environment variables that might interfere with tests.""" | ||
| monkeypatch.delenv("FASTAPI_CLOUD_TOKEN", raising=False) | ||
| yield |
There was a problem hiding this comment.
That was very annoying that tests stopped working when I had FASTAPI_CLOUD_TOKEN set on my machine.. Took me time to understand what is happening..
| @pytest.mark.parametrize( | ||
| "command", | ||
| [ | ||
| ["deploy"], | ||
| ["cloud", "deploy"], | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Should we configure all tests for fastapi deploy and fastapi login commands to also run with fastapi cloud deploy and fastapi cloud login?
We need at least this one for coverage (to trigger condition in the callback)
| def test_init_sentry_when_deployment_token( | ||
| logged_out_cli: Path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| monkeypatch.setenv("FASTAPI_CLOUD_TOKEN", "deployment-token") | ||
| with patch("fastapi_cloud_cli.utils.sentry.sentry_sdk.init") as mock_init: | ||
| init_sentry() | ||
|
|
||
| mock_init.assert_called_once_with( | ||
| dsn=SENTRY_DSN, | ||
| integrations=[ANY], # TyperIntegration instance | ||
| send_default_pii=False, | ||
| ) |
There was a problem hiding this comment.
This is not necessary for this PR, I just noticed that this use case wasn't covered by tests
There was a problem hiding this comment.
Pull request overview
This PR fixes authentication precedence so that FASTAPI_CLOUD_TOKEN (deployment token) is only preferred for deploy operations, while other commands continue to use the user token from the auth config. It introduces a global CLI context (ctx) to control Identity behavior per command context and updates multiple commands/tests accordingly.
Changes:
- Introduce
fastapi_cloud_cli.context.ctxto initialize per-command auth preference and accessIdentityviactx.get_identity(). - Update
Identityto track bothuser_tokenanddeploy_token, selecting the effective token based onprefer_auth_mode. - Expand CLI/test behavior: warnings for
login, informational output fordeploy/whoami, and add fixtures/tests around env token behavior.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sentry.py | Adds coverage for Sentry init when a deployment token is set. |
| tests/test_cli_login.py | Adds login test ensuring deploy token env var doesn’t override user login, and warning is printed. |
| tests/test_cli_deploy.py | Parameterizes deploy test to cover both fastapi deploy and fastapi cloud deploy, and asserts env-token info output. |
| tests/test_api_client.py | Initializes ctx for API client tests (currently only in this file). |
| tests/conftest.py | Adds autouse fixture to unset FASTAPI_CLOUD_TOKEN during tests. |
| src/fastapi_cloud_cli/utils/sentry.py | Uses Identity(prefer_auth_mode="token") for import-time Sentry init behavior. |
| src/fastapi_cloud_cli/utils/cli.py | Switches unauthorized handling to use ctx.get_identity() and removes direct Identity() usage. |
| src/fastapi_cloud_cli/utils/auth.py | Refactors Identity to support user vs deploy token selection without always overriding. |
| src/fastapi_cloud_cli/utils/api.py | Uses ctx.get_identity() for API client auth header creation. |
| src/fastapi_cloud_cli/context.py | Adds new Context singleton to store prefer_auth_mode and construct Identity. |
| src/fastapi_cloud_cli/commands/whoami.py | Updates whoami to work with context/dual-token output messaging. |
| src/fastapi_cloud_cli/commands/setup_ci.py | Uses context-provided identity. |
| src/fastapi_cloud_cli/commands/logs.py | Uses context-provided identity. |
| src/fastapi_cloud_cli/commands/login.py | Initializes context for root login command and warns if deploy token env var is set. |
| src/fastapi_cloud_cli/commands/link.py | Uses context-provided identity. |
| src/fastapi_cloud_cli/commands/env.py | Uses context-provided identity across env subcommands. |
| src/fastapi_cloud_cli/commands/deploy.py | Initializes context with prefer_auth_mode="token" for deploy and prints env-token info. |
| src/fastapi_cloud_cli/cli.py | Adds cloud_app callback to initialize context based on invoked subcommand. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
For now, if you have
FASTAPI_CLOUD_TOKENset and try to use any command, it will always use deploy token (see sources)This PR makes it only use
FASTAPI_CLOUD_TOKENfordeploycommand.For this we need to introduce a way for
Identityto know in which context it's used (whether it's adeploycommand or not).Each command can set a context (for now just
prefer_auth_mode) and we should always accessIdentityviactx.get_identity()(except the case with Sentry init as it's run on import time before setting the context).Now if user runs
fastapi loginandFASTAPI_CLOUD_TOKENis set, it will receive a warning:fastapi loginAnd on
fastapi deploycommand, we also notify that deployment token is being used:fastapi deployAs a side effect I updated the output of
whoamicommand.Now it checks both user token and deployment token:
Details
Not logged in, but deployment token is set:
Logged in, and deployment token is set:
Without deployment token:
Additionally added a
unset_env_varsfixture that ensuresFASTAPI_CLOUD_TOKENset on host machine doesn't break tests (currently on master tests will fail if you haveFASTAPI_CLOUD_TOKENset)