✨ [SC-2532] auth: Add OAuth2 PKCE login command#13
Closed
woakas wants to merge 5 commits into
Closed
Conversation
Slice vertical M2 (epic #2517 OAuth2 Third-Party Apps) combinando: - SC-2531 — Extiende ProfileConfigModel con campos OAuth opcionales (oauth_client_id, refresh_token, expires_at, scope, token_type) + AuthHeaderTypeEnum.OAUTH2 = "Authorization". Perfiles legacy TOKEN siguen cargando intactos. Perms 0600 + warning si están más abiertos. - SC-2535 — Centraliza el dispatch de header HTTP en cli/commons/http_auth.py::get_auth_headers. build_endpoint y get_runtimes_from_api delegan. Test de auditoría falla la build si reaparece X-Auth-Token hardcoded fuera del enum / helper / CORS allowlist de UbiFunctions. - SC-2532 — Comando 'ubidots login' con Authorization Code + PKCE (S256, secrets.token_urlsafe). Loopback en 127.0.0.1:53682, timeout 60s, state CSRF, --no-browser, --profile, --scope, --timeout. Resolución de client_id: flag > env var UBIDOTS_OAUTH_CLIENT_ID > profile > settings.OAUTH.DEFAULT_CLIENT_ID. Out of scope (próximos PRs): - SC-2534 — refresh wrapper transparente con filelock - SC-2533 — logout + whoami - SC-2536 — E2E + docs Tests: 43 nuevos en verde, 0 regresiones. Ruff target subido a py314.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements a complete OAuth2 Authorization Code + PKCE flow for CLI authentication. The implementation includes loopback callback handling, secure token exchange, CSRF protection, and profile persistence with hardened file permissions. The login command integrates cleanly with existing profile/configuration infrastructure and is registered as a top-level CLI command. ChangesOAuth2 Authorization Code + PKCE Flow
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginCmd as login command
participant Config
participant LoopbackSrv as LoopbackServer
participant OAuthSrv as OAuth Server
User->>LoginCmd: ubidots cli login [--flags]
LoginCmd->>Config: resolve_active_profile()
LoginCmd->>LoginCmd: resolve_client_id(flag, env, config)
LoginCmd->>LoginCmd: port_available(host, port)
LoginCmd->>LoginCmd: generate_pkce_pair()
LoginCmd->>LoginCmd: generate_state()
LoginCmd->>LoginCmd: build_authorize_url()
LoginCmd->>LoopbackSrv: LoopbackServer.wait_for_callback()
alt --no-browser
LoginCmd-->>User: print authorization URL
else default
LoginCmd->>User: open browser to authorization URL
end
User->>OAuthSrv: authorize application
OAuthSrv->>LoopbackSrv: redirect with code & state
LoopbackSrv->>LoopbackSrv: assert_state_matches()
LoopbackSrv-->>LoginCmd: return LoopbackResult
LoginCmd->>OAuthSrv: exchange_code_for_tokens()
OAuthSrv-->>LoginCmd: TokenSet (tokens, expires_at)
LoginCmd->>LoginCmd: extract_user_label(access_token)
LoginCmd->>Config: save_profile_configuration(oauth_fields)
LoginCmd-->>User: print success message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
cli/commons/tests/test_http_auth.py (1)
61-65: 💤 Low valueConsider using
frozensetforEXCLUDED_FILES.The static analyzer flags this as a mutable class attribute. Since
EXCLUDED_FILESis a constant, usingfrozensetwould make the immutability explicit and silence the warning.♻️ Proposed change
- EXCLUDED_FILES = { + EXCLUDED_FILES = frozenset({ "cli/config/models.py", "cli/commons/http_auth.py", "cli/functions/engines/models.py", - } + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/commons/tests/test_http_auth.py` around lines 61 - 65, EXCLUDED_FILES is defined as a mutable set literal; replace it with an immutable frozenset to make the constant explicit and silence the static analyzer. Locate the EXCLUDED_FILES constant in test_http_auth.py (symbol EXCLUDED_FILES) and change its definition from {"a", "b", ...} to frozenset({...}) (or use frozenset([...])) so the value is immutable; ensure no test mutates it anywhere else.cli/config/helpers.py (1)
162-167: 💤 Low valueConsider using
get_token_auth_headersdirectly.Since
get_runtimes_from_apialways uses token authentication, you could simplify by callingget_token_auth_headers(access_token)instead of constructing aProfileConfigModeljust to pass toget_auth_headers.♻️ Proposed simplification
def get_runtimes_from_api(access_token: str) -> list[dict]: - headers = get_auth_headers( - ProfileConfigModel( - auth_method=AuthHeaderTypeEnum.TOKEN, - access_token=access_token, - ) - ) + from cli.commons.http_auth import get_token_auth_headers + headers = get_token_auth_headers(access_token)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/config/helpers.py` around lines 162 - 167, The code in get_runtimes_from_api constructs a ProfileConfigModel with AuthHeaderTypeEnum.TOKEN only to call get_auth_headers; replace that by calling get_token_auth_headers(access_token) directly to simplify and remove the unnecessary ProfileConfigModel construction (references: get_runtimes_from_api, get_auth_headers, get_token_auth_headers, ProfileConfigModel, AuthHeaderTypeEnum.TOKEN, access_token).cli/auth/commands.py (1)
55-67: ⚡ Quick winConsider narrowing exception handling for clarity.
The bare
except Exception:on line 66 is functionally correct (JWT parsing is non-critical; fallback is safe), but narrowing to specific expected exceptions would improve readability and catch genuine bugs.♻️ Proposed refinement
claims = json.loads(base64.urlsafe_b64decode(payload_b64).decode("utf-8")) return claims.get("email") or claims.get("preferred_username") or claims.get("sub") or fallback - except Exception: + except (ValueError, json.JSONDecodeError, UnicodeDecodeError, IndexError, KeyError): return fallback🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/auth/commands.py` around lines 55 - 67, The broad except in _extract_user_label should be narrowed to the specific errors expected during JWT parsing (e.g., IndexError, AttributeError, ValueError, json.JSONDecodeError, and binascii.Error) so genuine bugs aren't swallowed; update the except clause on the block that reads token_set.access_token, splits the JWT, base64-decodes the payload, and parses JSON to catch and handle only those exceptions and still return fallback on failures.cli/auth/oauth_client.py (1)
99-107: ⚡ Quick winConsider defensive handling for missing
access_tokenin successful response.Line 102 directly accesses
data["access_token"], which will raiseKeyErrorif the OAuth server returns a 200 OK response without the required field. While this indicates a broken OAuth server (protocol violation), wrapping this in a try-except and raising aTokenExchangeErrorwould provide a clearer error message.🛡️ Proposed defensive handling
data = response.json() expires_in = int(data.get("expires_in", 0)) + try: - return TokenSet( - access_token=data["access_token"], - refresh_token=data.get("refresh_token", ""), - token_type=data.get("token_type", "Bearer"), - expires_at=int(time.time()) + expires_in, - scope=data.get("scope", ""), - ) + return TokenSet( + access_token=data["access_token"], + refresh_token=data.get("refresh_token", ""), + token_type=data.get("token_type", "Bearer"), + expires_at=int(time.time()) + expires_in, + scope=data.get("scope", ""), + ) + except KeyError as exc: + raise TokenExchangeError(detail=f"Server response missing required field: {exc}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/auth/oauth_client.py` around lines 99 - 107, The code in oauth_client.py that builds and returns TokenSet directly indexes data["access_token"], which can raise KeyError on malformed but 200 responses; update the token-exchange logic (the function that returns TokenSet) to defensively validate that "access_token" exists (e.g., check "access_token" in data or try/except KeyError) and if missing raise a TokenExchangeError with a clear message including the response body/status for debugging instead of letting a raw KeyError propagate; ensure refresh_token, token_type, expires_at and scope are still populated as before when access_token is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/auth/oauth_client.py`:
- Around line 95-96: The error check in oauth_client.py is too broad because it
raises UnknownOAuthClientError whenever "client" appears in detail; narrow this
by matching specific OAuth error codes/phrases (e.g., "invalid_client",
"unauthorized_client", or exact error_code fields) or use a precise
regex/whole-word match for known client-related messages rather than any
substring "client". Update the conditional that currently inspects the variable
detail (the branch that raises UnknownOAuthClientError) to only trigger for
those explicit patterns or mapped error_code values so generic messages like
"client-side validation failed" won't be misclassified.
In `@pyproject.toml`:
- Line 72: The Ruff config's target-version ("target-version = \"py314\"") is
incorrect for your declared Python range; update the target-version to "py39" so
it matches tool.poetry.dependencies' python = ">=3.9,<3.15" and avoids
suggesting unreleased/unsupported syntax—edit the target-version line in
pyproject.toml accordingly.
---
Nitpick comments:
In `@cli/auth/commands.py`:
- Around line 55-67: The broad except in _extract_user_label should be narrowed
to the specific errors expected during JWT parsing (e.g., IndexError,
AttributeError, ValueError, json.JSONDecodeError, and binascii.Error) so genuine
bugs aren't swallowed; update the except clause on the block that reads
token_set.access_token, splits the JWT, base64-decodes the payload, and parses
JSON to catch and handle only those exceptions and still return fallback on
failures.
In `@cli/auth/oauth_client.py`:
- Around line 99-107: The code in oauth_client.py that builds and returns
TokenSet directly indexes data["access_token"], which can raise KeyError on
malformed but 200 responses; update the token-exchange logic (the function that
returns TokenSet) to defensively validate that "access_token" exists (e.g.,
check "access_token" in data or try/except KeyError) and if missing raise a
TokenExchangeError with a clear message including the response body/status for
debugging instead of letting a raw KeyError propagate; ensure refresh_token,
token_type, expires_at and scope are still populated as before when access_token
is present.
In `@cli/commons/tests/test_http_auth.py`:
- Around line 61-65: EXCLUDED_FILES is defined as a mutable set literal; replace
it with an immutable frozenset to make the constant explicit and silence the
static analyzer. Locate the EXCLUDED_FILES constant in test_http_auth.py (symbol
EXCLUDED_FILES) and change its definition from {"a", "b", ...} to
frozenset({...}) (or use frozenset([...])) so the value is immutable; ensure no
test mutates it anywhere else.
In `@cli/config/helpers.py`:
- Around line 162-167: The code in get_runtimes_from_api constructs a
ProfileConfigModel with AuthHeaderTypeEnum.TOKEN only to call get_auth_headers;
replace that by calling get_token_auth_headers(access_token) directly to
simplify and remove the unnecessary ProfileConfigModel construction (references:
get_runtimes_from_api, get_auth_headers, get_token_auth_headers,
ProfileConfigModel, AuthHeaderTypeEnum.TOKEN, access_token).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba009eb1-75d2-449d-90f2-e4a0a1fad4ec
📒 Files selected for processing (18)
cli/auth/__init__.pycli/auth/commands.pycli/auth/loopback_server.pycli/auth/oauth_client.pycli/auth/tests/__init__.pycli/auth/tests/test_commands.pycli/auth/tests/test_loopback_server.pycli/auth/tests/test_oauth_client.pycli/commons/exceptions.pycli/commons/http_auth.pycli/commons/tests/test_http_auth.pycli/commons/utils.pycli/config/helpers.pycli/config/models.pycli/config/tests/test_oauth_profile_model.pycli/main.pycli/settings.pypyproject.toml
|
|
||
| [tool.ruff] | ||
| target-version = "py313" | ||
| target-version = "py314" |
There was a problem hiding this comment.
Ruff target-version does not match supported Python versions.
The target-version is set to "py314" (Python 3.14), but tool.poetry.dependencies declares support for python = ">=3.9,<3.15". Python 3.14 has not been released yet (as of March 2025), and targeting an unreleased version may cause Ruff to suggest syntax or features incompatible with the actual minimum supported version (3.9).
Set target-version = "py39" to align with the minimum supported Python version.
🔧 Proposed fix
-target-version = "py314"
+target-version = "py39"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| target-version = "py314" | |
| target-version = "py39" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` at line 72, The Ruff config's target-version ("target-version
= \"py314\"") is incorrect for your declared Python range; update the
target-version to "py39" so it matches tool.poetry.dependencies' python =
">=3.9,<3.15" and avoids suggesting unreleased/unsupported syntax—edit the
target-version line in pyproject.toml accordingly.
Permite que el comando login funcione contra entornos alternativos sin tener que pre-configurar un profile completo. - --port / UBIDOTS_OAUTH_LOOPBACK_PORT: override del loopback (default 53682). Útil si 53682 está ocupado o si el customer self-hosted registró otro redirect_uri en su core. El AS sigue siendo source of truth — DOT exige exact-match del redirect_uri, así que el port custom debe estar registrado en la OAuth Application. - --api-domain / -a / UBIDOTS_API_DOMAIN: override del host del Authorization Server (ej. https://cs.ubidots.site). El api_domain elegido se persiste en el profile YAML al guardar los tokens. - Mensaje de "port in use" ahora es accionable: incluye el comando lsof para encontrar el proceso, cómo usar --port, y la nota de registro del redirect_uri. 7 tests nuevos cubren la cascada de resolución de ambos flags + el mensaje diagnóstico de port-in-use.
- oauth_client.py: narrow the UnknownOAuthClientError mapping to the exact OAuth2 code `invalid_client` (RFC 6749 §5.2). The previous `"client" in detail.lower()` would misclassify generic errors like "Client-side validation failed". Added a regression test. - pyproject.toml: revert ruff target-version from py314 → py313 to match the repo's pre-existing target (and the actual minimum Python where this CLI's code patterns work in practice). The mismatch between python = ">=3.9" in dependencies and py313 target is pre-existing repo debt; out of scope for this PR.
…irmation Bugs encontrados al validar el flow con múltiples profiles: - BUG 1 — Cuando `--profile` no se pasa, el comando leía el profile activo correctamente pero guardaba los tokens siempre en `default.yaml`. Ahora resuelve el active profile real desde `config.yaml::profile` y persiste en ese archivo. Cubierto por `test_no_profile_flag_writes_to_active_profile_not_default`. - BUG 2 — `--profile X` con X inexistente fallaba con "File not found" porque `get_configuration` hace exit cuando el YAML no existe. Ahora `_resolve_active_config` detecta el profile inexistente y arranca con un `ProfileConfigModel` vacío; al final el archivo se crea en `<profiles_path>/X.yaml`. Cubierto por `test_explicit_profile_flag_creates_new_profile_if_missing`. Mejoras UX: - Mensaje al inicio: `Logging into profile '<name>' at <api_domain> (client_id=...)` para que sea obvio qué profile va a tocar antes de abrir el browser. - Si el profile target ya tiene sesión OAuth activa (auth_method=OAUTH2, access_token, refresh_token, expires_at > now), pedir confirmación con el email del usuario actual antes de sobrescribir. Skip con `--yes` / `-y` para CI/scripts. - Mensaje de éxito ahora incluye el profile: `Login successful as <email> (profile: <name>)`. 5 tests nuevos; suite total 56 nuevos en verde.
DevOps registrará la OAuth Application en core con `client_id=ubidots-cli` (decisión ya documentada en el design del epic 2517). Hardcodear el default acá deja `ubidots login` listo para usar sin flags ni env vars — mismo UX que `gh auth login`, `gcloud auth login`, etc. Override sigue disponible vía: - `--client-id <id>` (mayor precedencia, override por invocación) - `UBIDOTS_OAUTH_CLIENT_ID=<id>` (env var, útil para self-hosted) - `oauth_client_id` ya guardado en el profile (reuso post-login) Tests no requieren cambios: los casos que validan el missing-client escenario hacen `monkeypatch.setattr(settings.OAUTH, "DEFAULT_CLIENT_ID", "")` explícito.
This was referenced May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resumen
Slice vertical M2 del epic #2517 OAuth2 Third-Party Apps, combinando tres tickets de Shortcut en un solo PR para que el flow
ubidots loginquede validable end-to-end:ProfileConfigModelcon campos OAuth opcionales (oauth_client_id,refresh_token,expires_at,scope,token_type) yAuthHeaderTypeEnum.OAUTH2 = "Authorization". Perfiles legacy conauth_method=X-Auth-Tokensiguen cargando intactos. Archivos de profile se persisten con0600y se tightenean al leer si están más abiertos.cli/commons/http_auth.py::get_auth_headers.build_endpointyget_runtimes_from_apidelegan en el helper. Test de auditoría falla la build si reapareceX-Auth-Tokenhardcoded fuera del enum, el helper, o la CORS allowlist de UbiFunctions (que no es auth header del CLI).ubidots logincon Authorization Code + PKCE (S256, verifier 64B, state 32B). Loopback en127.0.0.1:53682/callback, timeout 60s configurable, state CSRF chequeado, flags--client-id,--profile,--no-browser,--scope,--timeout. Resolución declient_idcon cascada: flag > env varUBIDOTS_OAUTH_CLIENT_ID>oauth_client_iddel profile >settings.OAUTH.DEFAULT_CLIENT_ID.Fuera de alcance (próximos PRs)
filelockubidots logout+whoamidocs/authentication.md+ CHANGELOGPendiente de DevOps antes de mergear
DevOps tiene que registrar la OAuth Application en core (
ubidots) con:python manage.py create_oauth_application \ --client-id ubidots-cli \ --name "Ubidots CLI" \ --client-type public \ --grant-type authorization-code \ --redirect-uris http://127.0.0.1:53682/callbackCuando esté creada, hardcodear el
client_idresultante encli/settings.py::OAuthSettings.DEFAULT_CLIENT_ID(hoy vacío). Mientras tanto,ubidots loginnecesita--client-id <id>oUBIDOTS_OAUTH_CLIENT_ID=<id>exportado.Test plan
cli/pages/tests/test_cloud_commands.py, no introducidas por este PR)ruff checkcontarget-version = "py314"limpio en todos los archivos del cambioubidots --helplistaloginyubidots login --helpmuestra los flags correctosubidots devices listcon perfil OAuth contra core stagingNotas técnicas
client_secret: cliente público (RFC 8252 §8.5) — PKCE reemplaza al secret.authlib, pero conhttpx(ya dep) +http.server(stdlib) basta para el alcance de este PR. Re-evaluar cuando llegue SC-2534.ruff target-version = "py314",from __future__ import annotationsdonde aplica para que las forward refs sean lazy en runtime actual (3.12) y bajo PEP 649 en 3.14.assert no_oauth_command_breaks_token_profileno es una regla externa — está cubierta por la suite (TestProfileConfigModelLegacyLoad,TestValidateProfileConfigBackwardCompat,TestGetAuthHeaders).Summary by CodeRabbit
Release Notes
New Features
ubidots auth logincommand supporting OAuth2 authentication with browser-based sign-in or manual URL flow.--no-browserflag for environments without browser access.Security