-
Notifications
You must be signed in to change notification settings - Fork 5
OIDC support #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
OIDC support #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 19
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=settings.cors_allow_origins, | ||
| allow_origins=[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: CORS configuration hardcoded to empty list
The CORS allow_origins is hardcoded to an empty list [], replacing the previous dynamic configuration from settings.cors_allow_origins. This effectively disables CORS for all origins, which will block any cross-origin requests from frontend applications. The new settings structure has CORS origins available at settings.api.cors_origins, but instead of updating the path, the value was hardcoded to empty.
| from idun_agent_schema.engine.adk import AdkAgentConfig | ||
| from idun_agent_engine.server.server_config import ServerAPIConfig | ||
| from yaml import YAMLError | ||
| from yaml.serializer import YAMLError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Incorrect import path for YAMLError class
The import was changed from from yaml import YAMLError to from yaml.serializer import YAMLError. However, the yaml.serializer module only exports Serializer and SerializerError, not YAMLError. The YAMLError class is defined in yaml.error and re-exported from the main yaml package. This change will cause an ImportError when the module is loaded.
| expires_at = int(data.get("expires_at") or 0) | ||
| print(f"access_token: {access_token}") | ||
| print(f"refresh_token: {refresh_token}") | ||
| print(f"expires_at: {expires_at}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Debug print statements expose sensitive authentication tokens
Several print() statements output sensitive authentication data including access_token and refresh_token values to stdout. In production environments, these tokens could end up in application logs, exposing credentials that could be used to impersonate users. These appear to be debugging statements that were accidentally left in the code.
Additional Locations (1)
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, detail="Session expired" | ||
| ) | ||
| return JSONResponse(content={"session": json.loads(raw)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Endpoint exposes raw authentication tokens to client
The /me endpoint returns the full session data including raw id_token, access_token, and refresh_token values via JSON response. These tokens are typically kept server-side and never exposed to the client once a session is established. Exposing these tokens increases the attack surface for token theft via XSS vulnerabilities, response interception, or malicious browser extensions. The endpoint could return just the normalized principal information (user_id, email, roles) instead of the complete session with raw tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible de regarder cette issue @ahmedennaifer
…lder - Introduced validation for AdkAgentConfig in the ConfigBuilder class. - Enhanced error handling for invalid ADK agent configurations. - Updated imports to include AdkAgentConfig for proper configuration management.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 19
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ) from e | ||
| try: | ||
| guardrails = yaml_config.get("guardrails", "") | ||
| self._guardrails = guardrails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Guardrails config assigned as raw dict instead of parsed object
The new line self._guardrails = guardrails assigns the raw YAML value (likely a dict) to _guardrails before checking if it's empty. When guardrails is truthy (contains config data), the conditional on line 119 is false, leaving _guardrails as a raw dict instead of a Guardrails instance. The _guardrails attribute is typed as Guardrails | None and is passed to EngineConfig in the build() method, causing type inconsistency. When guardrails config is present, it needs to be converted to a Guardrails object rather than being assigned as a raw value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le fichier setting de la base postgres est déjà creer ici: idun-agent-platform/services/idun_agent_manager/src/app/infrastructure/db/models/settings.py @ahmedennaifer
| "orjson>=3.10.12,<4.0.0", # Fast JSON serialization | ||
| "tenacity>=9.0.0,<10.0.0", # Retry logic | ||
|
|
||
| "redis>=5.2.0,<6.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A supprimer @ahmedennaifer
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, detail="Session expired" | ||
| ) | ||
| return JSONResponse(content={"session": json.loads(raw)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible de regarder cette issue @ahmedennaifer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inutilisé ? @ahmedennaifer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Si utilisé dans env.py entre autre
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inutilisé ? @ahmedennaifer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment t'as creer la nouvelle revision alembic sans ajouter UserModel dans idun-agent-platform/services/idun_agent_manager/alembic/env.py ? @ahmedennaifer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
j'ai du faire la revision a la main car j'avais des problèmes avec la migration de certaines briques pour SessionModel. Le UserModel n'est pas utilisé encore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment t'as creer la nouvelle revision alembic sans ajouter SessionModel dans idun-agent-platform/services/idun_agent_manager/alembic/env.py ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il manque UserModel @ahmedennaifer
Note
Introduce OIDC-based authentication with JWT/OAuth handlers, Postgres-backed session storage, and supporting API/router, models, and migrations.
infrastructure/auth/oidc.pywith supportingjwt_handler.pyandoauth_handler.py.api/v1/deps.pyandapi/v1/routers/auth.py.infrastructure/cache/session_storage.py,session_provider.py, and Postgres-backedpostgres_session_storage.py.infrastructure/db/models/session.pyand Alembic migration20251127_1149_add_session_table.py.core/settings.pyandinfrastructure/auth/__init__.py.libs/idun_agent_engine/.../config_builder.py,libs/idun_agent_schema/.../manager/settings.py, anddeps.py.core/errors.pyand app bootstrap inmain.py.domain/agents/{ports,entities}.py.docker-compose.dev.ymlandservices/idun_agent_manager/pyproject.toml.Written by Cursor Bugbot for commit a3c7f7a. This will update automatically on new commits. Configure here.