-
Notifications
You must be signed in to change notification settings - Fork 425
[feat] Add SSO via OIDC
#3141
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: chore/offline-agenta
Are you sure you want to change the base?
[feat] Add SSO via OIDC
#3141
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| # SuperTokens will use our get_dynamic_oidc_provider to fetch config | ||
| supertokens_url = f"/auth/signinup?thirdPartyId={third_party_id}&redirectToPath={redirect}" | ||
|
|
||
| return RedirectResponse(url=supertokens_url, status_code=302) |
Check warning
Code scanning / CodeQL
URL redirection from remote source Medium
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To fix this problem, we should ensure that the redirect parameter is validated before being used in the redirection logic. There are two main approaches:
- Whitelist: If the set of allowed redirect targets is known and finite, check that
redirectmatches one of these safe values. - Relative path validation: If arbitrary relative paths are allowed, ensure that
redirectdoes NOT specify a full URL (with scheme/host), and is a local path.
For this code, since the usage is for paths within the frontend, the second approach is practical:
- Use Python's
urllib.parse.urlparseto check thatredirectdoes not contain a hostname or scheme. - Remove any backslash characters (
\) to prevent bypass. - If validation fails, redirect to a safe default (e.g.,
/).
Specific changes:
- Add an import for
urlparsefromurllib.parse. - Validate the
redirectparameter before buildingsupertokens_url:- Strip backslashes.
- Ensure
urlparse(redirect).netlocandurlparse(redirect).schemeare empty. - If the validation fails, set
redirectto/.
All code changes are to occur within api/oss/src/apis/fastapi/auth/router.py in the block for /authorize/oidc.
No change to existing functionality except improved security for the redirection.
-
Copy modified line R65 -
Copy modified lines R79-R84
| @@ -62,6 +62,7 @@ | ||
| # Get provider to build third_party_id | ||
| from uuid import UUID | ||
| from oss.src.dbs.postgres.organizations.dao import OrganizationProvidersDAO | ||
| from urllib.parse import urlparse | ||
|
|
||
| providers_dao = OrganizationProvidersDAO() | ||
| provider = await providers_dao.get_by_id(UUID(provider_id)) | ||
| @@ -75,7 +76,12 @@ | ||
|
|
||
| # Redirect to SuperTokens third-party signin | ||
| # SuperTokens will use our get_dynamic_oidc_provider to fetch config | ||
| supertokens_url = f"/auth/signinup?thirdPartyId={third_party_id}&redirectToPath={redirect}" | ||
| # Validate redirect to avoid open redirect vulnerability | ||
| safe_redirect = redirect.replace("\\", "") | ||
| parsed = urlparse(safe_redirect) | ||
| if parsed.netloc or parsed.scheme: | ||
| safe_redirect = "/" | ||
| supertokens_url = f"/auth/signinup?thirdPartyId={third_party_id}&redirectToPath={safe_redirect}" | ||
|
|
||
| return RedirectResponse(url=supertokens_url, status_code=302) | ||
|
|
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.
Pull request overview
This PR adds Single Sign-On (SSO) via OpenID Connect (OIDC) support to Agenta. The implementation introduces a comprehensive authentication discovery system, dynamic OIDC provider configuration, user identity tracking, and organization-level authentication policies.
Key changes include:
- Frontend authentication flow refactored to support multiple auth methods with dynamic discovery
- Backend auth service with OIDC provider management, SuperTokens integration with custom overrides for dynamic providers
- New database tables for user identities and organization authentication policies (EE)
Reviewed changes
Copilot reviewed 32 out of 41 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
web/oss/src/pages/auth/[[...path]].tsx |
Refactored auth page to dynamically show/hide auth methods based on discovery endpoint and environment flags |
web/oss/src/lib/helpers/dynamicEnv.ts |
Added environment variables for email/OIDC auth feature flags |
web/oss/src/components/pages/auth/SocialAuth/index.tsx |
Added optional divider prop to conditionally show separator |
web/entrypoint.sh |
Added logic to derive AUTH_EMAIL_ENABLED and AUTH_OIDC_ENABLED from environment variables |
api/oss/src/core/auth/supertokens_overrides.py |
Implemented SuperTokens overrides for dynamic OIDC providers and custom session handling with user identities |
api/oss/src/core/auth/service.py |
Added AuthService with discovery, authentication, and authorization logic |
api/oss/src/core/auth/oidc.py |
Implemented OIDC client helper utilities for authorization and token exchange |
api/oss/src/core/auth/middleware.py |
Added organization policy enforcement middleware (EE) |
api/oss/src/apis/fastapi/auth/router.py |
Added /discover and /authorize/oidc endpoints |
api/oss/src/dbs/postgres/users/* |
Added user identities DAO, DBE, DBA, mappings, and types |
api/oss/src/dbs/postgres/organizations/* |
Added organization policies, domains, providers, invitations DAOs and models |
api/oss/databases/postgres/migrations/* |
Added migration for user_identities and organization tables |
api/ee/src/dbs/postgres/organizations/* |
EE-specific organization DBE/DBA classes |
api/test-auth.http |
Comprehensive HTTP test file documenting all auth endpoints and flows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class OrganizationPolicyDBA(OrganizationScopeDBA, LegacyLifecycleDBA): | ||
| __abstract__ = True | ||
|
|
||
| id = Column( | ||
| UUID(as_uuid=True), | ||
| primary_key=True, | ||
| default=uuid.uuid7, | ||
| unique=True, | ||
| nullable=False, | ||
| ) | ||
| allowed_methods = Column( | ||
| ARRAY(String), | ||
| nullable=False, | ||
| server_default="{}", | ||
| ) | ||
| invitation_only = Column( | ||
| Boolean, | ||
| nullable=False, | ||
| server_default="true", | ||
| ) | ||
| domains_only = Column( | ||
| Boolean, | ||
| nullable=False, | ||
| server_default="false", | ||
| ) | ||
| disable_root = Column( | ||
| Boolean, | ||
| nullable=False, | ||
| server_default="false", | ||
| ) | ||
|
|
||
|
|
||
| class OrganizationDomainDBA(OrganizationScopeDBA, LegacyLifecycleDBA): | ||
| __abstract__ = True | ||
|
|
||
| id = Column( | ||
| UUID(as_uuid=True), | ||
| primary_key=True, | ||
| default=uuid.uuid7, | ||
| unique=True, | ||
| nullable=False, | ||
| ) | ||
| domain = Column( | ||
| String, | ||
| nullable=False, | ||
| ) | ||
| verified = Column( | ||
| Boolean, | ||
| nullable=False, | ||
| server_default="false", | ||
| ) | ||
| verification_token = Column( | ||
| String, | ||
| nullable=True, | ||
| ) | ||
|
|
||
|
|
||
| class OrganizationProviderDBA(OrganizationScopeDBA, HeaderDBA, LegacyLifecycleDBA): | ||
| __abstract__ = True | ||
|
|
||
| id = Column( | ||
| UUID(as_uuid=True), | ||
| primary_key=True, | ||
| default=uuid.uuid7, | ||
| unique=True, | ||
| nullable=False, | ||
| ) | ||
| slug = Column( | ||
| String, | ||
| nullable=False, | ||
| ) | ||
| enabled = Column( | ||
| Boolean, | ||
| nullable=False, | ||
| server_default="true", | ||
| ) | ||
| domain_id = Column( | ||
| UUID(as_uuid=True), | ||
| nullable=True, | ||
| ) | ||
| config = Column( | ||
| JSONB(none_as_null=True), | ||
| nullable=False, | ||
| ) | ||
|
|
||
|
|
||
| class OrganizationInvitationDBA(OrganizationScopeDBA, LegacyLifecycleDBA): |
Copilot
AI
Dec 25, 2025
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.
The DBA classes inherit from LegacyLifecycleDBA but the migration creates columns matching LifecycleDBA (which includes deleted_at, created_by_id, updated_by_id, and deleted_by_id). This mismatch between the migration and the model definition will cause runtime errors. These classes should inherit from LifecycleDBA instead of LegacyLifecycleDBA to match the migration schema.
| return False | ||
|
|
||
| # Get domain and check if it matches | ||
| domain_dto = await self.domains_dao.get_by_id(provider.domain_id) |
Copilot
AI
Dec 25, 2025
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.
The code calls await self.domains_dao.get_by_id(provider.domain_id) but the OrganizationDomainsDAO class doesn't have a get_by_id method. It only has get_by_domain, list_by_organization, and mark_verified methods. This will cause an AttributeError at runtime.
| domain_dto = await self.domains_dao.get_by_id(provider.domain_id) | |
| if hasattr(self.domains_dao, "get_by_id"): | |
| domain_dto = await self.domains_dao.get_by_id(provider.domain_id) | |
| elif hasattr(self.domains_dao, "get_by_domain"): | |
| domain_dto = await self.domains_dao.get_by_domain(domain) | |
| else: | |
| return False |
| from ee.src.core.organizations.types import ( | ||
| OrganizationPolicy, | ||
| OrganizationPolicyCreate, | ||
| OrganizationPolicyUpdate, | ||
| OrganizationDomain, | ||
| OrganizationDomainCreate, | ||
| OrganizationProvider, | ||
| OrganizationProviderCreate, | ||
| OrganizationProviderUpdate, | ||
| OrganizationInvitation, | ||
| OrganizationInvitationCreate, | ||
| ) |
Copilot
AI
Dec 25, 2025
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.
The DAO imports types from ee.src.core.organizations.types but this file is in the OSS directory (oss/src/dbs/postgres/organizations/dao.py). This creates a dependency on EE code from OSS code, which breaks the architectural separation. These types should either be defined in OSS or the DAO should only be used in EE. Based on the context, these organization-related features appear to be EE-only, so this DAO file should probably be in the EE directory.
| from ee.src.core.organizations.types import ( | ||
| OrganizationPolicy, | ||
| OrganizationPolicyCreate, | ||
| OrganizationPolicyUpdate, | ||
| OrganizationDomain, | ||
| OrganizationDomainCreate, | ||
| OrganizationProvider, | ||
| OrganizationProviderCreate, | ||
| OrganizationProviderUpdate, | ||
| OrganizationInvitation, | ||
| OrganizationInvitationCreate, | ||
| ) |
Copilot
AI
Dec 25, 2025
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 file imports types from ee.src.core.organizations.types but is located in the OSS directory. This creates an improper dependency on EE code from OSS code, violating the architectural separation. Since organization policies, domains, providers, and invitations appear to be EE-only features, this mappings file should be located in the EE directory structure.
| # Format: oidc:{org_id}:{provider_slug} -> sso:{org_slug}:{provider_slug} | ||
| parts = third_party_id.split(":") | ||
| org_id_str, provider_slug = parts[1], parts[2] | ||
| # TODO: Fetch org_slug from organization table | ||
| org_slug = "org" # Placeholder | ||
| method = f"sso:{org_slug}:{provider_slug}" |
Copilot
AI
Dec 25, 2025
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.
The code uses a hardcoded placeholder org_slug = "org" instead of fetching the actual organization slug from the database. This means all SSO identities will be stored with the same generic org slug, making it impossible to distinguish between different organizations' SSO providers. The TODO comment indicates this is known incomplete work that needs to be addressed.
| # Format: oidc:{org_id}:{provider_slug} -> sso:{org_slug}:{provider_slug} | |
| parts = third_party_id.split(":") | |
| org_id_str, provider_slug = parts[1], parts[2] | |
| # TODO: Fetch org_slug from organization table | |
| org_slug = "org" # Placeholder | |
| method = f"sso:{org_slug}:{provider_slug}" | |
| # Format: oidc:{org_id}:{provider_slug} -> sso:{org_id}:{provider_slug} | |
| parts = third_party_id.split(":") | |
| org_id_str, provider_slug = parts[1], parts[2] | |
| method = f"sso:{org_id_str}:{provider_slug}" |
| @@ -0,0 +1,241 @@ | |||
| """SuperTokens override functions for dynamic OIDC providers and custom session handling.""" | |||
|
|
|||
| from typing import Dict, Any, List, Optional, Union | |||
Copilot
AI
Dec 25, 2025
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.
Import of 'List' is not used.
Import of 'Union' is not used.
| from typing import Dict, Any, List, Optional, Union | |
| from typing import Dict, Any, Optional |
| from supertokens_python.recipe.thirdparty.provider import ( | ||
| Provider, | ||
| ProviderInput, | ||
| ProviderConfig, | ||
| ProviderClientConfig, | ||
| ) |
Copilot
AI
Dec 25, 2025
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.
Import of 'Provider' is not used.
| from supertokens_python.recipe.session.interfaces import ( | ||
| RecipeInterface as SessionRecipeInterface, | ||
| ) | ||
| from supertokens_python.types import User, RecipeUserId |
Copilot
AI
Dec 25, 2025
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.
Import of 'User' is not used.
| from supertokens_python.types import User, RecipeUserId | |
| from supertokens_python.types import RecipeUserId |
| # For now, assume they are | ||
| is_member = True | ||
|
|
||
| if not is_member: | ||
| return { | ||
| "error": "NOT_A_MEMBER", | ||
| "message": "You are not a member of this organization", | ||
| } | ||
|
|
Copilot
AI
Dec 25, 2025
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 statement is unreachable.
| # For now, assume they are | |
| is_member = True | |
| if not is_member: | |
| return { | |
| "error": "NOT_A_MEMBER", | |
| "message": "You are not a member of this organization", | |
| } | |
| # For now, assume they are a member and proceed to policy checks |
| # For now, assume they are | ||
| is_member = True | ||
|
|
||
| if not is_member: | ||
| return { | ||
| "error": "NOT_A_MEMBER", | ||
| "message": "You are not a member of this organization", | ||
| } | ||
|
|
Copilot
AI
Dec 25, 2025
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 statement is unreachable.
| # For now, assume they are | |
| is_member = True | |
| if not is_member: | |
| return { | |
| "error": "NOT_A_MEMBER", | |
| "message": "You are not a member of this organization", | |
| } | |
| # For now, assume they are (no membership check implemented yet) |
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.
Pull request overview
Copilot reviewed 57 out of 66 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from uuid import UUID | ||
| from typing import Optional, List | ||
| from sqlalchemy import select | ||
|
|
||
| from oss.src.dbs.postgres.shared.engine import engine | ||
| from oss.src.dbs.postgres.organizations.dbes import ( | ||
| OrganizationPolicyDBE, | ||
| OrganizationDomainDBE, | ||
| OrganizationProviderDBE, | ||
| OrganizationInvitationDBE, | ||
| ) | ||
| from oss.src.dbs.postgres.organizations.mappings import ( | ||
| map_policy_dbe_to_dto, | ||
| map_create_policy_dto_to_dbe, | ||
| map_update_policy_dto_to_dbe, | ||
| map_domain_dbe_to_dto, | ||
| map_create_domain_dto_to_dbe, | ||
| map_provider_dbe_to_dto, | ||
| map_create_provider_dto_to_dbe, | ||
| map_update_provider_dto_to_dbe, | ||
| map_invitation_dbe_to_dto, | ||
| map_create_invitation_dto_to_dbe, | ||
| ) | ||
| from ee.src.core.organizations.types import ( | ||
| OrganizationPolicy, | ||
| OrganizationPolicyCreate, | ||
| OrganizationPolicyUpdate, | ||
| OrganizationDomain, | ||
| OrganizationDomainCreate, | ||
| OrganizationProvider, | ||
| OrganizationProviderCreate, | ||
| OrganizationProviderUpdate, | ||
| OrganizationInvitation, | ||
| OrganizationInvitationCreate, | ||
| ) | ||
|
|
||
|
|
||
| class OrganizationPoliciesDAO: | ||
| async def create(self, dto: OrganizationPolicyCreate) -> OrganizationPolicy: | ||
| policy_dbe = map_create_policy_dto_to_dbe(dto) | ||
|
|
||
| async with engine.core_session() as session: | ||
| session.add(policy_dbe) | ||
| await session.commit() | ||
| await session.refresh(policy_dbe) | ||
|
|
||
| return map_policy_dbe_to_dto(policy_dbe) | ||
|
|
||
| async def get_by_organization( | ||
| self, organization_id: UUID | ||
| ) -> Optional[OrganizationPolicy]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationPolicyDBE).filter_by( | ||
| organization_id=organization_id | ||
| ) | ||
| result = await session.execute(stmt) | ||
| policy_dbe = result.scalar() | ||
|
|
||
| if policy_dbe is None: | ||
| return None | ||
|
|
||
| return map_policy_dbe_to_dto(policy_dbe) | ||
|
|
||
| async def update( | ||
| self, | ||
| organization_id: UUID, | ||
| dto: OrganizationPolicyUpdate, | ||
| ) -> Optional[OrganizationPolicy]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationPolicyDBE).filter_by( | ||
| organization_id=organization_id | ||
| ) | ||
| result = await session.execute(stmt) | ||
| policy_dbe = result.scalar() | ||
|
|
||
| if policy_dbe is None: | ||
| return None | ||
|
|
||
| map_update_policy_dto_to_dbe(policy_dbe, dto) | ||
| await session.commit() | ||
| await session.refresh(policy_dbe) | ||
|
|
||
| return map_policy_dbe_to_dto(policy_dbe) | ||
|
|
||
|
|
||
| class OrganizationDomainsDAO: | ||
| async def create( | ||
| self, dto: OrganizationDomainCreate | ||
| ) -> OrganizationDomain: | ||
| domain_dbe = map_create_domain_dto_to_dbe(dto) | ||
|
|
||
| async with engine.core_session() as session: | ||
| session.add(domain_dbe) | ||
| await session.commit() | ||
| await session.refresh(domain_dbe) | ||
|
|
||
| return map_domain_dbe_to_dto(domain_dbe) | ||
|
|
||
| async def get_by_id(self, domain_id: UUID) -> Optional[OrganizationDomain]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationDomainDBE).filter_by(id=domain_id) | ||
| result = await session.execute(stmt) | ||
| domain_dbe = result.scalar() | ||
|
|
||
| if domain_dbe is None: | ||
| return None | ||
|
|
||
| return map_domain_dbe_to_dto(domain_dbe) | ||
|
|
||
| async def get_by_domain(self, domain: str) -> Optional[OrganizationDomain]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationDomainDBE).filter_by(domain=domain) | ||
| result = await session.execute(stmt) | ||
| domain_dbe = result.scalar() | ||
|
|
||
| if domain_dbe is None: | ||
| return None | ||
|
|
||
| return map_domain_dbe_to_dto(domain_dbe) | ||
|
|
||
| async def list_by_organization( | ||
| self, organization_id: UUID, verified_only: bool = False | ||
| ) -> List[OrganizationDomain]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationDomainDBE).filter_by( | ||
| organization_id=organization_id | ||
| ) | ||
| if verified_only: | ||
| stmt = stmt.filter_by(verified=True) | ||
|
|
||
| result = await session.execute(stmt) | ||
| domain_dbes = result.scalars().all() | ||
|
|
||
| return [map_domain_dbe_to_dto(dbe) for dbe in domain_dbes] | ||
|
|
||
| async def mark_verified(self, domain_id: UUID) -> Optional[OrganizationDomain]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationDomainDBE).filter_by(id=domain_id) | ||
| result = await session.execute(stmt) | ||
| domain_dbe = result.scalar() | ||
|
|
||
| if domain_dbe is None: | ||
| return None | ||
|
|
||
| domain_dbe.verified = True | ||
| await session.commit() | ||
| await session.refresh(domain_dbe) | ||
|
|
||
| return map_domain_dbe_to_dto(domain_dbe) | ||
|
|
||
|
|
||
| class OrganizationProvidersDAO: | ||
| async def create( | ||
| self, dto: OrganizationProviderCreate | ||
| ) -> OrganizationProvider: | ||
| provider_dbe = map_create_provider_dto_to_dbe(dto) | ||
|
|
||
| async with engine.core_session() as session: | ||
| session.add(provider_dbe) | ||
| await session.commit() | ||
| await session.refresh(provider_dbe) | ||
|
|
||
| return map_provider_dbe_to_dto(provider_dbe) | ||
|
|
||
| async def get_by_id(self, provider_id: UUID) -> Optional[OrganizationProvider]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationProviderDBE).filter_by(id=provider_id) | ||
| result = await session.execute(stmt) | ||
| provider_dbe = result.scalar() | ||
|
|
||
| if provider_dbe is None: | ||
| return None | ||
|
|
||
| return map_provider_dbe_to_dto(provider_dbe) | ||
|
|
||
| async def get_by_slug( | ||
| self, organization_id: UUID, slug: str | ||
| ) -> Optional[OrganizationProvider]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationProviderDBE).filter_by( | ||
| organization_id=organization_id, slug=slug | ||
| ) | ||
| result = await session.execute(stmt) | ||
| provider_dbe = result.scalar() | ||
|
|
||
| if provider_dbe is None: | ||
| return None | ||
|
|
||
| return map_provider_dbe_to_dto(provider_dbe) | ||
|
|
||
| async def list_by_organization( | ||
| self, organization_id: UUID, enabled_only: bool = False | ||
| ) -> List[OrganizationProvider]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationProviderDBE).filter_by( | ||
| organization_id=organization_id | ||
| ) | ||
| if enabled_only: | ||
| stmt = stmt.filter_by(enabled=True) | ||
|
|
||
| result = await session.execute(stmt) | ||
| provider_dbes = result.scalars().all() | ||
|
|
||
| return [map_provider_dbe_to_dto(dbe) for dbe in provider_dbes] | ||
|
|
||
| async def list_by_domain( | ||
| self, domain_id: UUID, enabled_only: bool = False | ||
| ) -> List[OrganizationProvider]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationProviderDBE).filter_by(domain_id=domain_id) | ||
| if enabled_only: | ||
| stmt = stmt.filter_by(enabled=True) | ||
|
|
||
| result = await session.execute(stmt) | ||
| provider_dbes = result.scalars().all() | ||
|
|
||
| return [map_provider_dbe_to_dto(dbe) for dbe in provider_dbes] | ||
|
|
||
| async def update( | ||
| self, | ||
| provider_id: UUID, | ||
| dto: OrganizationProviderUpdate, | ||
| ) -> Optional[OrganizationProvider]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationProviderDBE).filter_by(id=provider_id) | ||
| result = await session.execute(stmt) | ||
| provider_dbe = result.scalar() | ||
|
|
||
| if provider_dbe is None: | ||
| return None | ||
|
|
||
| map_update_provider_dto_to_dbe(provider_dbe, dto) | ||
| await session.commit() | ||
| await session.refresh(provider_dbe) | ||
|
|
||
| return map_provider_dbe_to_dto(provider_dbe) | ||
|
|
||
|
|
||
| class OrganizationInvitationsDAO: | ||
| async def create( | ||
| self, dto: OrganizationInvitationCreate | ||
| ) -> OrganizationInvitation: | ||
| invitation_dbe = map_create_invitation_dto_to_dbe(dto) | ||
|
|
||
| async with engine.core_session() as session: | ||
| session.add(invitation_dbe) | ||
| await session.commit() | ||
| await session.refresh(invitation_dbe) | ||
|
|
||
| return map_invitation_dbe_to_dto(invitation_dbe) | ||
|
|
||
| async def get_by_token(self, token: str) -> Optional[OrganizationInvitation]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationInvitationDBE).filter_by(token=token) | ||
| result = await session.execute(stmt) | ||
| invitation_dbe = result.scalar() | ||
|
|
||
| if invitation_dbe is None: | ||
| return None | ||
|
|
||
| return map_invitation_dbe_to_dto(invitation_dbe) | ||
|
|
||
| async def list_by_organization( | ||
| self, organization_id: UUID, status: Optional[str] = None | ||
| ) -> List[OrganizationInvitation]: | ||
| async with engine.core_session() as session: | ||
| stmt = select(OrganizationInvitationDBE).filter_by( | ||
| organization_id=organization_id | ||
| ) | ||
| if status: | ||
| stmt = stmt.filter_by(status=status) | ||
|
|
||
| result = await session.execute(stmt) | ||
| invitation_dbes = result.scalars().all() | ||
|
|
||
| return [map_invitation_dbe_to_dto(dbe) for dbe in invitation_dbes] |
Copilot
AI
Dec 25, 2025
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.
The OSS version defines the same database models and migrations that appear in the EE version. This creates significant code duplication - the entire contents of dbas.py, dbes.py, mappings.py, and dao.py are duplicated. Since these are meant to be EE-only features (organization policies, SSO providers), they should only exist in the EE codebase. The OSS versions should either be removed or contain only stub/fallback implementations.
| from alembic import context | ||
|
|
||
| from sqlalchemy import Connection, func, insert, select, update | ||
| from sqlalchemy.orm import load_only |
Copilot
AI
Dec 25, 2025
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.
The load_only optimization is imported but only partially applied. While line 65 adds optimization for the organization query in this batch processing migration, consider whether other queries in the migration could benefit from similar optimization to reduce memory usage during large data migrations.
| showDivider = true, | ||
| }: SocialAuthProps) => { |
Copilot
AI
Dec 25, 2025
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.
The showDivider prop defaults to true to maintain backward compatibility, but the calling code in the auth page explicitly passes the value on line 225. The prop could be made required instead of optional with a default, making the API more explicit and reducing ambiguity about when the divider appears.
| sa.Column( | ||
| "expires_at", | ||
| sa.TIMESTAMP(timezone=True), | ||
| nullable=True, | ||
| ), |
Copilot
AI
Dec 25, 2025
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.
The expires_at field in the migration is correctly defined as TIMESTAMP, but the DBA model defines it as UUID (lines 125-128 in dbas.py). This mismatch between the migration and the model will cause runtime errors. The DBA model should be updated to use TIMESTAMP type.
| """In-memory state store for OIDC flows. TODO: Move to Redis for production.""" | ||
|
|
||
| from typing import Dict, Optional | ||
| from datetime import datetime, timedelta | ||
| import asyncio | ||
|
|
||
|
|
||
| class StateStore: | ||
| """Simple in-memory state store with expiration.""" | ||
|
|
||
| def __init__(self): | ||
| self._store: Dict[str, Dict] = {} | ||
| self._expiry: Dict[str, datetime] = {} | ||
|
|
||
| async def set(self, key: str, value: Dict, ttl_seconds: int = 600) -> None: | ||
| """Store a value with TTL (default 10 minutes).""" | ||
| self._store[key] = value | ||
| self._expiry[key] = datetime.utcnow() + timedelta(seconds=ttl_seconds) | ||
| await self._cleanup_expired() | ||
|
|
||
| async def get(self, key: str) -> Optional[Dict]: | ||
| """Get a value, return None if expired or not found.""" | ||
| await self._cleanup_expired() | ||
|
|
||
| if key not in self._store: | ||
| return None | ||
|
|
||
| if key in self._expiry and datetime.utcnow() > self._expiry[key]: | ||
| del self._store[key] | ||
| del self._expiry[key] | ||
| return None | ||
|
|
||
| return self._store[key] | ||
|
|
||
| async def delete(self, key: str) -> None: | ||
| """Delete a value.""" | ||
| self._store.pop(key, None) | ||
| self._expiry.pop(key, None) | ||
|
|
||
| async def _cleanup_expired(self) -> None: | ||
| """Remove expired entries.""" | ||
| now = datetime.utcnow() | ||
| expired_keys = [k for k, exp in self._expiry.items() if now > exp] | ||
| for key in expired_keys: | ||
| self._store.pop(key, None) | ||
| self._expiry.pop(key, None) | ||
|
|
||
|
|
||
| # Singleton instance | ||
| state_store = StateStore() |
Copilot
AI
Dec 25, 2025
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.
The in-memory StateStore is not suitable for production use, especially in multi-instance deployments where state needs to be shared. The TODO comment acknowledges this, but using this in production would cause OIDC flows to fail when requests hit different instances. Consider implementing Redis-backed storage before deploying to production, or add validation to prevent deployment with in-memory state.
| @@ -0,0 +1,147 @@ | |||
| """SuperTokens configuration and initialization.""" | |||
|
|
|||
| from typing import Dict, List, Any, Optional | |||
Copilot
AI
Dec 25, 2025
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.
Import of 'Optional' is not used.
| @@ -0,0 +1,343 @@ | |||
| """SuperTokens override functions for dynamic OIDC providers and custom session handling.""" | |||
|
|
|||
| from typing import Dict, Any, List, Optional, Union | |||
Copilot
AI
Dec 25, 2025
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.
Import of 'List' is not used.
| from supertokens_python.recipe.session.interfaces import ( | ||
| RecipeInterface as SessionRecipeInterface, | ||
| ) | ||
| from supertokens_python.types import User, RecipeUserId |
Copilot
AI
Dec 25, 2025
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.
Import of 'User' is not used.
| is_member = True | ||
|
|
||
| if not is_member: | ||
| return { | ||
| "error": "NOT_A_MEMBER", | ||
| "message": "You are not a member of this organization", | ||
| } | ||
|
|
Copilot
AI
Dec 25, 2025
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 statement is unreachable.
| is_member = True | |
| if not is_member: | |
| return { | |
| "error": "NOT_A_MEMBER", | |
| "message": "You are not a member of this organization", | |
| } |
| return { | ||
| "error": "NOT_A_MEMBER", | ||
| "message": "You are not a member of this organization", | ||
| } |
Copilot
AI
Dec 25, 2025
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 statement is unreachable.
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.
Pull request overview
Copilot reviewed 60 out of 69 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
|
|
||
|
|
||
| class OrganizationPolicyDBA(OrganizationScopeDBA, LegacyLifecycleDBA): |
Copilot
AI
Dec 25, 2025
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.
Inconsistent base class usage: OSS uses LifecycleDBA while EE uses LegacyLifecycleDBA. This inconsistency may lead to differences in table schemas and lifecycle field behavior between OSS and EE editions for the same tables.
| call_to_action=( | ||
| "Click the link below to accept the invitation:</p><br>" | ||
| f'<a href="{invite_link}">Accept Invitation</a>' | ||
| ), |
Copilot
AI
Dec 25, 2025
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.
The variable invite_link is referenced in the f-string but is not defined in this diff section. This will cause a NameError. Check if invite_link is defined earlier in the function or if it should be constructed here.
| # Get user ID and check membership | ||
| user_id = session.get_user_id() | ||
|
|
||
| # TODO: Check if user is a member of organization | ||
| # For now, assume they are | ||
| is_member = True | ||
|
|
||
| if not is_member: | ||
| return { | ||
| "error": "NOT_A_MEMBER", | ||
| "message": "You are not a member of this organization", | ||
| } | ||
|
|
Copilot
AI
Dec 25, 2025
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 statement is unreachable.
| # Get user ID and check membership | |
| user_id = session.get_user_id() | |
| # TODO: Check if user is a member of organization | |
| # For now, assume they are | |
| is_member = True | |
| if not is_member: | |
| return { | |
| "error": "NOT_A_MEMBER", | |
| "message": "You are not a member of this organization", | |
| } | |
| # Get user ID (membership enforcement not yet implemented) | |
| # TODO: Enforce that user is a member of the organization before applying policy | |
| user_id = session.get_user_id() |
| return { | ||
| "error": "NOT_A_MEMBER", | ||
| "message": "You are not a member of this organization", | ||
| } |
Copilot
AI
Dec 25, 2025
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 statement is unreachable.
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.
Pull request overview
Copilot reviewed 62 out of 71 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| identities = payload.get("identities", []) | ||
|
|
||
| # Get user ID and check membership | ||
| user_id = session.get_user_id() |
Copilot
AI
Dec 26, 2025
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.
Variable user_id is not used.
| user_id = session.get_user_id() |
| return { | ||
| "error": "NOT_A_MEMBER", | ||
| "message": "You are not a member of this organization", | ||
| } |
Copilot
AI
Dec 26, 2025
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 statement is unreachable.
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.
Pull request overview
Copilot reviewed 95 out of 104 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
web/entrypoint.sh:1
- The order of NEXT_PUBLIC_AGENTA_WEB_URL and NEXT_PUBLIC_AGENTA_API_URL has been swapped compared to the original, but the surrounding code context suggests this was intentional. However, this creates an inconsistency with the removed lines that had AGENTA_API_URL first. Verify this order change is intentional and doesn't break existing configurations that depend on the original ordering.
api/oss/src/utils/env.py:1 - Hardcoding a default PostHog API key in production code is a security risk. This key should only be used in development/demo environments. Consider making this explicitly conditional on environment (e.g., only default in demo mode) or removing the default entirely to force explicit configuration.
api/oss/src/utils/env.py:1 - Defaulting critical security keys to 'replace-me' is dangerous. These should fail loudly if not set in production rather than falling back to insecure defaults. Consider raising an exception when these are not configured in non-development environments.
api/oss/src/services/organization_service.py:1 - The error message 'No existing invitation found for the user' is misleading when an existing_role exists but no existing_invitation. The user may have been previously invited (thus having a role) but the invitation record is missing. Consider a more accurate message like 'User has no active invitation or existing role in this organization'.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # --> GET ORGANIZATION BATCH | ||
| query = ( | ||
| select(OrganizationDB) | ||
| .options(load_only(OrganizationDB.id, OrganizationDB.owner)) |
Copilot
AI
Dec 26, 2025
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.
Adding load_only() to limit columns fetched is good, but verify that no other attributes of OrganizationDB are accessed elsewhere in this migration that aren't included in this list. If other attributes are accessed, this could cause additional queries or errors.
| .options(load_only(OrganizationDB.id, OrganizationDB.owner)) |
Extend domain verification More
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.
Pull request overview
Copilot reviewed 98 out of 107 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| identities = payload.get("identities", []) | ||
|
|
||
| # Get user ID and check membership | ||
| user_id = session.get_user_id() |
Copilot
AI
Dec 26, 2025
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.
Variable user_id is not used.
| from typing import Sequence, Union | ||
| from alembic import op | ||
| import sqlalchemy as sa | ||
| from sqlalchemy import select, update, func, text |
Copilot
AI
Dec 26, 2025
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.
Import of 'func' is not used.
Import of 'select' is not used.
Import of 'update' is not used.
| from sqlalchemy import select, update, func, text | |
| from sqlalchemy import text |
| from alembic import op | ||
| import sqlalchemy as sa | ||
| from sqlalchemy import select, update, func, text | ||
| from sqlalchemy.dialects import postgresql |
Copilot
AI
Dec 26, 2025
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.
Import of 'postgresql' is not used.
| from sqlalchemy.dialects import postgresql |
|
|
||
| from typing import Optional, Callable, List | ||
| from uuid import UUID | ||
| from fastapi import Request, Response, HTTPException |
Copilot
AI
Dec 26, 2025
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.
Import of 'HTTPException' is not used.
|
|
||
| import secrets | ||
| import httpx | ||
| from typing import Dict, Any, Optional |
Copilot
AI
Dec 26, 2025
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.
Import of 'Optional' is not used.
| # Step 2: OSS-specific validation and migration | ||
| if not is_ee: | ||
| # OSS: Must have exactly 1 organization | ||
| org_count = conn.execute(text("SELECT COUNT(*) FROM organizations")).scalar() |
Copilot
AI
Dec 26, 2025
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 statement is unreachable.
| return { | ||
| "error": "NOT_A_MEMBER", | ||
| "message": "You are not a member of this organization", | ||
| } |
Copilot
AI
Dec 26, 2025
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 statement is unreachable.
No description provided.