feat: Add shared OAuth manager for plugin credential coordination - #251
feat: Add shared OAuth manager for plugin credential coordination#251r-pedraza wants to merge 31 commits into
Conversation
…ntials before legacy fallback
| or not token_set.access_token.strip() | ||
| ): | ||
| raise error_cls(f"OAuth {action} returned an empty access token.") | ||
| if not token_set.is_valid( |
There was a problem hiding this comment.
This validates a freshly obtained token against the proactive-refresh margin (default 300s). Any provider that issues access tokens with a lifetime shorter than the margin will fail every refresh and every interactive login with "expires too soon", discard the usable token, and never persist it — an unrecoverable loop. Validation here should check that the token is not already expired (refresh_margin_seconds=0), and leave the margin to the proactive-refresh decision in _resolve_without_provider.
| return OAuthTokenSet( | ||
| access_token=refreshed.access_token, | ||
| refresh_token=refreshed.refresh_token or previous.refresh_token, | ||
| expires_at=refreshed.expires_at, |
There was a problem hiding this comment.
refresh_token, scopes and metadata are preserved when the provider omits them, but expires_at is not. Because is_valid() treats expires_at is None as "never expires", a provider that omits expiry on refresh yields a token that is cached and served indefinitely and is never refreshed again — the previous expiry guarantee is lost. Either carry the previous expiry forward, or reject a refresh result with no expires_at when the previous set had one.
| stored_token_set.scope if stored_token_set else "user" | ||
| ) | ||
| if stored_token_set and stored_token_set.token_set.refresh_token: | ||
| storage_scope = stored_token_set.scope |
There was a problem hiding this comment.
read_with_scope() can return scope="env", and SecretManager.set(scope="env") only mutates os.environ for the current process. Refreshing an env-sourced token therefore never persists the new (possibly rotated) refresh token, and delete(scope="env") cannot really remove the credential even though oauth.refresh.stale_deleted is emitted. Consider mapping an env read scope onto a durable write scope (user) for write/delete, or refusing to refresh env-sourced token sets.
| if env_credential: | ||
| return env_credential | ||
|
|
||
| stored_token_set = self.token_store.read(request) |
There was a problem hiding this comment.
token_store.read() raises OAuthStorageError on an unparseable stored blob, and this call precedes both the legacy-secret fallback and the provider branch. One corrupted secret therefore makes get_credential fail unrecoverably — no oauth.* failure event is emitted, the configured legacy keys are never consulted, and interactive re-auth never runs. Worth catching OAuthStorageError here, emitting a storage-failure event, and treating the blob as absent (ideally deleting it) so the remaining resolution order still applies.
| except asyncio.CancelledError: | ||
| cancel_event.set() | ||
| try: | ||
| held_lock = await asyncio.shield(worker) |
There was a problem hiding this comment.
The cleanup path here can leak the lock forever. await asyncio.shield(worker) inside the except asyncio.CancelledError block is itself cancellable: if a second cancellation arrives (double task.cancel(), an enclosing asyncio.timeout, or a TaskGroup unwinding), CancelledError propagates out of this block before held_lock.release() runs, while the worker thread goes on to acquire the lock. Since self._locks entries are never evicted, that key's threading.Lock stays held for the rest of the process lifetime and every later refresh/login on that credential deadlocks until timeout.
Suggest making the cleanup uncancellable, e.g. add a done-callback that releases the result regardless of who is awaiting:
except asyncio.CancelledError:
cancel_event.set()
def _release_if_acquired(task: asyncio.Task) -> None:
if task.cancelled():
return
if task.exception() is None:
task.result().release()
worker.add_done_callback(_release_if_acquired)
raiseThe same concern applies to finally: await asyncio.to_thread(held_lock.release) in lock() — a cancellation delivered during unwinding leaves the lock held.
| """Write a token set and return the SecretManager key used.""" | ||
| secret_key = self.build_secret_key(request) | ||
| try: | ||
| scope = _validate_scope(scope) |
There was a problem hiding this comment.
_validate_scope runs inside the try, so an invalid scope is re-raised as OAuthStorageError("... could not be written.") instead of the ValueError that names the allowed scopes — an input-validation bug gets reported as a storage failure. Moving the _validate_scope(scope) call above the try in both write and delete keeps the useful message.
| if _is_sensitive_metadata_key(key_label): | ||
| safe_metadata[key_label] = REDACTED_METADATA_VALUE | ||
| continue | ||
| return MappingProxyType(safe_metadata) |
There was a problem hiding this comment.
Keys that are neither whitelisted nor sensitive-looking are dropped here with no trace. OAuthManager._safe_metadata happily forwards arbitrary keys, so any new metadata added later disappears silently and looks like it was never set. Consider mapping unknown keys to <redacted> (or recording a dropped-key count) so the loss is observable.
|
|
||
| def _is_safe_secret_key_value(value: Any) -> bool: | ||
| """Return whether a stored secret key label is safe to expose.""" | ||
| return isinstance(value, str) and value.startswith("oauth_") and _is_safe_identifier( |
There was a problem hiding this comment.
This hardcodes the oauth_ prefix, but OAuthTokenStore.secret_prefix is configurable (defaults to "oauth"), so a store built with any other prefix will have its secret_key metadata redacted even though the key is a non-secret label. The _is_safe_identifier check alone already guarantees the value is structurally safe — the prefix check adds no security and only loses information.
| manager = _manager(secrets, tmp_path) | ||
| request = _request() | ||
|
|
||
| secret_key = manager.save_token_set_blocking( |
There was a problem hiding this comment.
get_credential_blocking isn't exercised anywhere, and neither blocking helper's "cannot run inside an active event loop" guard is tested. Two cheap additions: one sync test resolving a credential via get_credential_blocking, and one async def test (or an asyncio.run wrapper) asserting both blocking helpers raise OAuthError when a loop is already running.
There was a problem hiding this comment.
Segunda pasada sobre 25c34b1b, con el detalle inline en cada línea.
Los seis comentarios marcados MEGA-BLOCKING son el bloque que decide si esto entra: reintroducen en la superficie pública lo que #261 acaba de quitar. Dos cosas van comprobadas ejecutando, no leyendo, y se citan en su sitio.
Nota menor: la descripción del PR menciona get_credential_sync e invalidate_sync; no existe ningún invalidate en la rama (son get_credential_blocking / save_token_set_blocking). También he cerrado los hilos que el merge de master dejó obsoletos; los 24 restantes siguen aplicando.
| class OAuthCredential: | ||
| """Resolved credential returned to callers.""" | ||
|
|
||
| access_token: str |
There was a problem hiding this comment.
MEGA-BLOCKING — rompe el contrato de secrets de #261.
OAuthCredential es la salida pública de get_credential(), así que cada plugin y cada step que use OAuth recibe un token vivo en un str plano. La regla de .claude/docs/security.md es literal: no Titan API ever returns a secret string.
Tenemos ya las tres piezas exportadas desde core.security, elige la que encaje:
SecretRef+create_authenticated_session(ref, scheme)para cualquier cosa HTTP,broker.create_client(key, builder)para SDKs que necesitan el token en el constructor (es como Slack monta hoy suWebClient),SensitiveValue+.reveal()lo más tarde posible, como último recurso.
Mientras esto sea str, el resto de las protecciones (redacción, escaneo de metadata) trabajan a ciegas.
| ): | ||
| raise error_cls(f"OAuth {action} returned a token that expires too soon.") | ||
|
|
||
| def _credential_from_token_set( |
There was a problem hiding this comment.
MEGA-BLOCKING — el token que sale de aquí no está registrado para redacción.
_credential_from_token_set devuelve token_set.access_token sin llamar a register_secret(). El único registro que ocurre es el del blob JSON completo en _vault.set(), y el access_token de dentro nunca se registra por separado. Resultado: un token que viene de provider.refresh() / authorize() es invisible tanto para el filtro de redacción de logs como para el escaneo de metadata de Success/Skip/Exit.
Lo he reproducido:
source: oauth-login | token: BRAND-NEW-LOGIN-TOKEN-xyz
access_token registered for redaction? False
>>> Success(metadata={"slack_token": cred.access_token}) accepted: LEAK NOT DETECTED
Es decir: un step puede publicar el token OAuth en la metadata de un resultado y nada lo detecta. register_secret() tiene que llamarse sobre la cadena exacta que cruza el boundary, en el momento en que lo cruza.
| register_secret(value) | ||
| return value.strip() | ||
|
|
||
| def set( |
There was a problem hiding this comment.
Complemento del comentario en _credential_from_token_set: set() delega en _vault.set(), que registra el blob JSON para redacción. El access_token de dentro no queda registrado como cadena independiente, así que redact() (que casa por substring exacto) nunca lo enmascara cuando aparece solo en un log o en un header.
Si el registro se hace aquí sobre el access_token además del blob, el problema desaparece para el camino de storage; para el camino de provider hace falta también el registro en el manager.
| self, | ||
| secrets: object | None = None, | ||
| *, | ||
| namespace: str = "titan", |
There was a problem hiding this comment.
MEGA-BLOCKING — esto salta el namespacing del broker.
Todo el diseño de SecretBroker es que el llamante no puede elegir su namespace: derive_namespace() lo deriva de la identidad que el executor lee de la definición del workflow, dentro del boundary. Aquí el namespace es un parámetro con default "titan", así que todos los blobs OAuth caen en el namespace global sea quien sea el que pregunta, y se pierde el aislamiento por plugin del keyring.
El namespace debería venir del broker que se recibe, no ser un argumento de este constructor.
| project_path: Path | None = None, | ||
| vault: SecretManager | None = None, | ||
| ) -> None: | ||
| self._vault = vault or SecretManager(project_path=project_path) |
There was a problem hiding this comment.
MEGA-BLOCKING — segundo SecretManager, y debería ser el broker.
create_oauth_secret_store() construye su propio vault mientras SecretBrokerFactory mantiene el compartido. Son dos snapshots independientes en memoria de .titan/secrets.env: una escritura con scope project por un lado es invisible por el otro hasta reinstanciar.
Sobre el argumento de "es core, no un plugin, así que el broker no aplica" — aplica igual, y no es una zona gris:
derive_namespace()reserva explícitamente la identidad"core"→titan.core, y su propio docstring lo dice: "the engine builder usesfor_plugin(\"core\")for app-level consumers".engine/builder.py:107ya lo hace en producción:create_broker_factory().for_plugin("core")para el broker del AI executor.plugin_registry._reject_reserved_plugin_nameimpide que un plugin reclamecore/project/user, precisamente para que esas identidades sean de Titan.
Así que el manager debería recibir el SecretBroker del plugin en cuyo nombre resuelve (o for_plugin("core") para uso app-level), no fabricarse un vault. Ser core-side es la razón para usar el broker, no para saltárselo.
| resolved = self.get_with_scope(key, namespace=namespace) | ||
| return resolved.value if resolved else None | ||
|
|
||
| def get_with_scope( |
There was a problem hiding this comment.
Causa raíz del AttributeError de storage.py:151, y hay que arreglarlo junto con el bug.
FakeSecretManager implementa get, get_with_scope y get_from_scope, y no resolve — o sea, la imagen espejo del adaptador de producción, que tiene resolve y ninguno de los otros. Como _get_secret_with_scope prueba resolve() primero, los 74 tests corren el branch de compatibilidad y el de producción tiene cero cobertura. Por eso esto está verde en CI con un crash en el camino normal.
Además su get_from_scope toma namespace posicional donde el real es keyword-only.
Pediría: añadir resolve() al fake (dejando un test aparte para el branch de compat), y al menos un test que construya OAuthTokenStore() sin argumentos y lea una clave ausente.
| source=request.access_token_env_var, | ||
| ) | ||
|
|
||
| def _credential_from_legacy_secret( |
There was a problem hiding this comment.
El camino legacy tiene un agujero de migración justo para el primer consumidor previsto, Slack.
Slack guarda hoy tres claves separadas ({project}_slack_user_token, _slack_refresh_token, _slack_token_expires_at); esto guarda un único blob JSON bajo oauth_<sha256>. Y aquí el token legacy se devuelve sin comprobar expiración y sin ninguna vía de refresh, y se consulta antes de authorize().
Los user tokens de Slack caducan en ~12h y rotan el refresh token, así que con la clave legacy todavía en el keyring servaríamos un access token muerto indefinidamente, sin forma de salir del bucle.
Dos salidas: ignorar las entradas legacy cuando se conoce una expiración, o permitir que el refresh token legacy arranque el primer refresh y se persista ya como blob.
| f"OAuth credential for '{request.connection_id}' is not available." | ||
| ) | ||
|
|
||
| def get_credential_blocking( |
There was a problem hiding this comment.
Aviso de interacción con #265 (lazy plugin loading), que está en vuelo.
Este guard lanza OAuthError cuando ya hay un event loop corriendo — confirmado ejecutándolo. Y #265 mueve el initialize() de los plugins a primer uso, incluido un ensure_all_initialized() que se llama directamente desde plugin_management._load_plugins() en el hilo del event loop de Textual.
O sea: el helper bloqueante funciona desde steps de workflow (esos corren en run_worker(thread=True)) y lanza desde la pantalla de plugins, donde Slack aparecería como "failed" sin estarlo.
Lo que consuma esto necesita un camino async para llamantes del hilo de UI, y interactive=True no debería ser alcanzable nunca desde initialize() — si no, el primer uso abre un navegador en medio de un workflow.
| request: OAuthRequest, | ||
| credential_key: str, | ||
| *, | ||
| include_legacy: bool = True, |
There was a problem hiding this comment.
Nit: include_legacy sólo se llama con False en los dos call sites, así que la rama por defecto (True) es código muerto. O se usa, o fuera del parámetro.
En la misma línea: _safe_metadata duplica el saneado que ya hace events._freeze_metadata.
| @@ -0,0 +1,47 @@ | |||
| # OAuth Manager | |||
There was a problem hiding this comment.
Esta página son 47 líneas y no está referenciada desde mkdocs.yml, así que no se renderiza.
Y lo que le falta es justo lo que necesita quien vaya a escribir un provider: cómo se relaciona con el boundary de secrets, de dónde sale el namespace, qué scopes de storage existen y qué se devuelve al step (que, según el primer comentario de esta revisión, debería dejar de ser un str).
finxo
left a comment
finxo
left a comment
There was a problem hiding this comment.
Tercera tanda: hallazgos del análisis que aún no estaban en ningún hilo, para que no se pierdan. 4 importantes + 3 nits + 3 matices sobre hilos ya abiertos. El primero es del mismo bloque de secrets que los MEGA-BLOCKING de la revisión anterior.
| value = os.environ.get(key.upper()) | ||
| if not value or not value.strip(): | ||
| return None | ||
| register_secret(value) |
There was a problem hiding this comment.
MEGA-BLOCKING — el valor que se registra para redacción no es el que se devuelve.
register_secret(value) guarda la cadena cruda del entorno, pero al llamante se le entrega value.strip(). Como redact() enmascara por substring exacto (if value in text), cuando la env var trae un \n o un espacio al final — habitual en credenciales inyectadas por CI o con $(cat file) — el token que de verdad va a los headers Authorization y a los logs nunca casa con el patrón registrado y no se redacta jamás.
Es la misma familia que el comentario de _credential_from_token_set: hay que registrar exactamente la cadena que cruza el boundary.
stripped = value.strip()
if not stripped:
return None
register_secret(stripped)
return stripped(registrar ambas formas también vale, pero registrar sólo lo que devuelves es el invariante seguro).
| if scope == "project": | ||
| value = self._vault._project_secrets.get(key.upper()) | ||
| elif scope == "user": | ||
| value = keyring.get_password(namespace, key) |
There was a problem hiding this comment.
keyring.get_password sin guardas, a diferencia de _vault.resolve(), que lo envuelve en try/except Exception precisamente porque lanza cuando no hay backend de keyring disponible.
Como este método sólo existe para OAuthTokenStore._verify_secret_deleted(), y SecretManager.delete(scope="user") ya se traga los errores de keyring (except Exception: pass), en una máquina sin backend la secuencia es: el delete "funciona" → esta lectura lanza → OAuthTokenStore.delete() lo envuelve como OAuthStorageError("could not be deleted") → el camino de recuperación tras un refresh fallido aborta la resolución entera. Un problema de disponibilidad de keyring se reporta como fallo de borrado.
try:
value = keyring.get_password(namespace, key)
except Exception:
value = None| raise | ||
| if storage_scope is not None: | ||
| reauthorize_storage_scope = storage_scope | ||
| except Exception as exc: |
There was a problem hiding this comment.
Un fallo transitorio de refresh fuerza un re-login completo y encima se pierde la causa.
En modo interactivo, tanto este except Exception como el except OAuthError de arriba se limitan a fijar reauthorize_storage_scope y caen al branch de authorize(), sin re-lanzar ni envolver. Eso hace indistinguible un invalid_grant de un timeout/DNS/5xx del endpoint de token, o de un bug en el adapter: el refresh token sigue siendo válido y de hecho no se borra aquí, pero al usuario se le empuja a un login interactivo completo.
Sólo OAuthTokenInvalidError significa de verdad que el grant está muerto, y ese caso ya tiene su propio branch con borrado de la credencial rancia. Además, en el camino interactivo la excepción original se descarta por completo (el no-interactivo al menos hace raise OAuthTokenRefreshError(str(exc)) from exc), así que la causa no llega ni al llamante ni al sink.
Propuesta: limitar el fallback automático a re-autorización al caso invalid-grant, y propagar el resto como OAuthTokenRefreshError envolviendo exc sea o no interactivo — que lo reintentable siga siendo reintentable.
| operation_id = uuid.uuid4().hex | ||
| credential_key = build_oauth_credential_key(request) | ||
|
|
||
| async with self.lock_manager.lock(credential_key): |
There was a problem hiding this comment.
get_credential mantiene este mismo lock de credencial mientras hace await provider.refresh(...) / await provider.authorize(...), y OAuthLockManager usa un threading.Lock que no es reentrante.
Así que un adapter que persista su propio resultado con await manager.save_token_set(...) durante su authorize/refresh — algo natural, y es la única API pública de persistencia — se autobloquea hasta agotar los 60 s de presupuesto y sale con un OAuthLockTimeout que no tiene nada que ver con lo que pasó.
O se documenta en el OAuthProvider que los adapters no deben llamar de vuelta al manager (sólo devolver el token set), o se expone un camino de escritura interno que ya asuma el lock tomado.
| expires_at=refreshed.expires_at, | ||
| token_type=refreshed.token_type, | ||
| scopes=refreshed.scopes or previous.scopes, | ||
| metadata={**previous.metadata, **refreshed.metadata}, |
There was a problem hiding this comment.
Nit: con este merge el provider puede añadir o sobrescribir claves, pero nunca borrar una. Cualquier valor ligado al grant que el provider deje de devolver (un id_token, una pista de cuenta/sub, un scope anterior) sobrevive a todos los refreshes siguientes, así que quien lea token_set.metadata puede recibir material que caducó con el grant original.
Si la intención es sólo preservar el conjunto concreto de campos que los providers pueden omitir legítimamente — igual que se hace justo arriba con refresh_token y scopes — mejor una allow-list explícita que fusionar el mapping entero.
| "expires_at": self.expires_at, | ||
| "token_type": self.token_type, | ||
| "scopes": list(self.scopes), | ||
| "metadata": dict(self.metadata), |
There was a problem hiding this comment.
Nit: metadata sólo se copia en superficie, nunca se valida como serializable a JSON, pero acaba aquí dentro del payload que OAuthTokenStore.write() pasa a json.dumps.
Un provider que devuelva un datetime, un enum o un Path en metadata produce un TypeError envuelto como OAuthStorageError("... could not be written.") después de un intercambio/refresh exitoso: se pierde un token recién obtenido y el error nombra la clave del secreto en vez del campo culpable.
Validar la serializabilidad en __post_init__ (p. ej. un round-trip json.dumps dentro de _normalize_mapping) atribuye el fallo a la metadata del provider en el momento de construirla.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class OAuthCredential: |
There was a problem hiding this comment.
Nit, complementario al comentario sobre access_token de más abajo: OAuthCredential es el único modelo del módulo sin __post_init__, así que ni normaliza ni valida nada.
Con el store real el riesgo es bajo (OAuthSecretStore.resolve_env ya hace .strip()), pero _credential_from_env pasa el valor tal cual mientras _credential_from_legacy_secret sí hace .strip() — dos caminos con reglas distintas para el mismo campo. Un access_token de sólo espacios se acepta como credencial válida, porque el if not token de arriba sólo descarta la cadena vacía.
Cuando cambies el tipo del campo, vale la pena añadir de paso la misma normalización que hace OAuthTokenSet.
| held_lock = await asyncio.shield(worker) | ||
| except _OAuthLockAcquisitionCancelled: | ||
| pass | ||
| except OAuthLockTimeout: |
There was a problem hiding this comment.
Matiz sobre el hilo que ya hay abierto en este bloque (el del lock filtrado por doble cancelación): además de eso, aquí sólo se toleran _OAuthLockAcquisitionCancelled y OAuthLockTimeout.
Cualquier otro fallo del worker — el OSError que _FileLock.acquire() re-lanza para errores que no son contención, un OSError al crear/abrir el fichero de lock, o el RuntimeError de _try_acquire_once() sin handle — escapa de este bloque except asyncio.CancelledError y el raise final nunca se ejecuta. La task se traga su propia cancelación: el llamante recibe un OSError por una petición que en realidad fue cancelada, y los caminos que dependen de observar CancelledError (asyncio.wait_for, un TaskGroup desenrollándose, el shutdown) no se enteran.
Con capturar ancho aquí (except Exception: pass, logueando si quieres) el CancelledError se re-lanza siempre después de drenar el worker.
| refresh_token="refresh-token", | ||
| expires_at=int(time.time()) + 120, | ||
| ), | ||
| id="expires-inside-refresh-margin", |
There was a problem hiding this comment.
Ojo con estos dos parámetros (aquí y en la parametrización de autorización, línea ~1219) al arreglar el hilo del margen: fijan como comportamiento esperado que un token recién emitido por el provider con 120 s de vida sea un fallo duro, porque _validate_provider_token_set aplica el margen de refresh proactivo (300 s) a resultados frescos.
O sea: un provider que emita access tokens más cortos que el margen no puede completar nunca ni un refresh ni un login. Si el chequeo de margen se relaja para que sólo gobierne la reutilización de tokens cacheados — que es lo correcto — estos dos casos tienen que irse con el arreglo. Tal como están, cualquiera que lea el test lo interpretará como la especificación deseada y bloqueará el fix.
|
|
||
| assert credential.access_token == "fresh-token" | ||
| assert credential.source == "oauth-refresh" | ||
| assert secrets.set_calls == [] |
There was a problem hiding this comment.
Este assert fija que un blob de origen env se refresca y se descarta en silencio. Con providers que rotan el refresh token en cada uso, el rotado se tira y el que hay en la env var ya está consumido: cada ejecución posterior reintenta con un token muerto.
Es el fondo del hilo de scope="env" que dejé arriba (GitHub lo marcó outdated tras el merge, así que puede que no lo veas en el diff): la premisa vieja ya no aplica — _VALID_STORAGE_SCOPES impide escribir a env — pero la consecuencia sigue viva.
Si "los blobs de env son read-only" es el contrato deseado, el test debería exigir también una señal observable (un evento tipo oauth.storage.skipped) para que el llamante pueda enterarse. Como está, se blinda un no-persist silencioso sin forma de detectarlo.
finxo
left a comment
finxo
left a comment
There was a problem hiding this comment.
No mergear hasta que se cierre el bloque de secrets
Marco REQUEST_CHANGES explícito para que esto no entre por error: tal como está, el PR rompe dos invariantes que ya están ratificados en el harness del repo.
1. El invariante de secrets_hardening (entregado en #261). Objetivo textual del dominio: "ninguna API de Titan devuelva jamás un secreto en claro a nadie: ni a steps, ni a plugins, ni a clientes, ni a pantallas TUI. SecretManager puede usarse donde sea necesario, pero solo dentro de una frontera de confianza única; fuera de ella solo existen handles opacos y primitivas de uso." Y entre lo que se protege estructuralmente: "filtrado accidental por logs, UI, metadata o serialización" y "reutilización insegura de secrets entre plugins/steps (namespaces derivados, no elegibles)".
Ahora mismo OAuthCredential.access_token es un str crudo que sale de la frontera, el token del provider no se registra para redacción (comprobado: pasa el guard de Success(metadata=...)), y el namespace es un argumento con default "titan" en vez de derivarse. Los tres son exactamente los puntos que #261 cerró.
2. La decisión de almacenamiento de Slack. En el harness del dominio slack: "Slack configuration for a repository should live in .titan/config.toml […] The personal Slack token remains in keyring", con la razón explícita de "preserve the personal secret boundary". El camino scope="project" de OAuthTokenStore.write() escribe el blob de tokens a .titan/secrets.env — fichero compartido del equipo, dentro del checkout — y el refresh reutiliza ese scope, así que cada refresh token rotado se sigue escribiendo ahí. Eso degrada silenciosamente lo que Slack hace hoy con broker.store().
Aparte del bloque de secrets, el AttributeError de storage.py:151 hace que el store por defecto reviente en el primer arranque (verificado ejecutándolo), y el camino legacy no puede refrescar los tokens de Slack, que caducan en ~12h rotando el refresh token.
El detalle está en los comentarios inline. La dirección no la discuto: un OAuth manager compartido en core es lo correcto y core/security/oauth_tokens.py es el sitio adecuado para el manejo crudo. Lo que falta es que la salida del manager sea un handle opaco (SecretRef + create_authenticated_session, broker.create_client, o SensitiveValue) y que el namespace y el vault vengan del broker — que, para código core, es for_plugin("core"), tal y como ya hace engine/builder.py:107.
2 participants
Pull Request
📝 Summary
Introduces a centralized OAuth manager in
titan_cli.core.oauththat provides provider-neutral credential resolution for plugins. The manager handles token storage, refresh coordination with credential-scoped locks, and lifecycle events through an event sink abstraction, enabling plugins to request OAuth credentials without coupling to specific OAuth providers.🔧 Changes Made
OAuthManagerwith async API for credential resolution from environment variables, token store, legacy secrets, or registered providersOAuthTokenStorefor persisting token sets as JSON blobs via SecretManagerOAuthLockManagerwith file-based and in-memory locks for refresh/login coordination across workersOAuthRequestandOAuthTokenSetdataclasses with scope normalization and validationOAuthCredentialas the resolved credential returned to pluginsOAuthEventSinkabstraction withCollectingOAuthEventSinkandQueuedOAuthEventSinkimplementationsOAuthAuthenticationRequired,OAuthAuthorizationError,OAuthTokenInvalidErrorget_credential_sync,invalidate_sync) for synchronous workflow executordocs/concepts/oauth-manager.md🧪 Testing
poetry run pytest)make test)titan-devComprehensive test coverage including:
📊 Logs
oauth.resolve.started(DEBUG) — Emitted when credential resolution begins; includes connection_idoauth.resolve.succeeded(DEBUG) — Emitted on successful resolution; includes source (env var, cache, legacy, provider)oauth.resolve.failed(DEBUG) — Emitted on resolution failure; includes error detailsoauth.refresh.started(DEBUG) — Emitted when token refresh beginsoauth.refresh.succeeded(DEBUG) — Emitted on successful refreshoauth.refresh.failed(DEBUG) — Emitted on refresh failureoauth.auth.required(DEBUG) — Emitted when user authentication is neededoauth.invalidate.started(DEBUG) — Emitted when token invalidation beginsoauth.invalidate.succeeded(DEBUG) — Emitted on successful invalidation✅ Checklist
Plugins > Git Plugin,GitHub Plugin,Jira Plugin)