From 692010f0fcf9812e23e75151504608be9dd0bab3 Mon Sep 17 00:00:00 2001 From: "Byron.wang" Date: Thu, 3 Sep 2026 08:47:14 +0000 Subject: [PATCH 01/50] refactor(api): decouple console OAuth flow from legacy account services (#41188) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --- api/.importlinter | 2 + api/controllers/console/auth/oauth.py | 385 +++---- api/controllers/console/wraps.py | 10 + api/extensions/ext_application_services.py | 72 ++ api/models/account.py | 9 - api/openapi/markdown/console-openapi.md | 10 +- .../account_integration_repository.py | 33 + api/repositories/account_oauth_repository.py | 128 +++ api/repositories/account_repository.py | 10 + .../workspace_query_repository.py | 4 + api/services/account_errors.py | 56 + api/services/account_oauth_adapters.py | 203 ++++ api/services/account_oauth_service.py | 283 ++++++ api/services/account_ports.py | 9 + api/services/account_service.py | 40 - .../entities/account_oauth_entities.py | 62 ++ api/services/errors/account.py | 4 - api/services/errors/workspace.py | 4 - .../models/test_account.py | 36 +- .../test_account_oauth_identity_lock.py | 64 ++ .../services/test_account_service.py | 172 ---- .../controllers/console/auth/test_oauth.py | 960 +++++------------- .../console/auth/test_oauth_redirect.py | 190 ---- .../console/auth/test_oauth_timezone.py | 131 --- .../console/workspace/test_workspace.py | 3 + .../test_ext_application_services.py | 22 + .../repositories/test_account_repository.py | 64 ++ .../services/test_account_oauth_adapters.py | 318 ++++++ .../services/test_account_oauth_service.py | 762 ++++++++++++++ .../services/test_account_service.py | 77 -- 30 files changed, 2512 insertions(+), 1611 deletions(-) create mode 100644 api/repositories/account_oauth_repository.py create mode 100644 api/services/account_oauth_adapters.py create mode 100644 api/services/account_oauth_service.py create mode 100644 api/services/entities/account_oauth_entities.py create mode 100644 api/tests/test_containers_integration_tests/services/test_account_oauth_identity_lock.py delete mode 100644 api/tests/unit_tests/controllers/console/auth/test_oauth_redirect.py delete mode 100644 api/tests/unit_tests/controllers/console/auth/test_oauth_timezone.py create mode 100644 api/tests/unit_tests/services/test_account_oauth_adapters.py create mode 100644 api/tests/unit_tests/services/test_account_oauth_service.py diff --git a/api/.importlinter b/api/.importlinter index 53da8e4c6c5..99bff19c96d 100644 --- a/api/.importlinter +++ b/api/.importlinter @@ -215,11 +215,13 @@ source_modules = services.account_initialization_service services.account_integration_service services.account_login_service + services.account_oauth_service services.account_password_service services.account_ports services.account_profile_service services.entities.account_entities services.entities.account_login_entities + services.entities.account_oauth_entities services.entities.auth_audit_entities forbidden_modules = configs diff --git a/api/controllers/console/auth/oauth.py b/api/controllers/console/auth/oauth.py index 58f13f33511..5ff5a86c3d5 100644 --- a/api/controllers/console/auth/oauth.py +++ b/api/controllers/console/auth/oauth.py @@ -1,47 +1,51 @@ -import logging import urllib.parse -import httpx -from flask import current_app, redirect, request +from flask import redirect, request from flask_restx import Resource from pydantic import BaseModel, Field -from werkzeug.exceptions import Unauthorized from werkzeug.wrappers import Response from configs import dify_config from constants.languages import languages from controllers.common.fields import RedirectResponse -from controllers.common.schema import query_params_from_model, register_response_schema_model, register_schema_models +from controllers.common.schema import query_params_from_model, register_response_schema_models, register_schema_models from controllers.console.error import AccountInFreezeError, EmailDomainSuspendedError -from enums import DeploymentEdition -from extensions.ext_database import db -from libs.datetime_utils import naive_utc_now -from libs.helper import extract_remote_ip +from controllers.console.wraps import model_validate, setup_required, social_oauth_login_enabled +from extensions.ext_application_services import application_services +from fields.base import ResponseModel +from libs.helper import dump_response, extract_remote_ip from libs.helper import timezone as validate_timezone_string -from libs.oauth import GitHubOAuth, GoogleOAuth, OAuthUserInfo, decode_oauth_state +from libs.oauth import decode_oauth_state from libs.token import ( set_access_token_to_cookie, set_csrf_token_to_cookie, set_refresh_token_to_cookie, ) -from models import Account, AccountStatus -from services.account_service import AccountService, RegisterService, TenantService -from services.billing_service import BillingService -from services.errors.account import ( - AccountNotFoundError, - AccountRegisterError, - SeatsLimitExceededError, +from services.account_errors import ( + AccountEmailDomainSuspendedError, + AccountEmailFrozenError, + InvalidOAuthInvitationError, + InvalidOAuthProviderError, + OAuthAccountBannedError, + OAuthAccountNotFoundError, + OAuthIdentityLockUnavailableError, + OAuthInvitationAccountMismatchError, + OAuthProviderAuthorizationError, + OAuthProviderRequestError, + OAuthRegistrationError, + OAuthSeatsLimitExceededError, + OAuthWorkspaceCreationNotAllowedError, ) -from services.errors.account import ( - EmailDomainSuspendedError as EmailDomainSuspendedRegistrationError, +from services.entities.account_entities import AccountSessionTokens +from services.entities.account_oauth_entities import ( + OAuthAuthorizationRequest, + OAuthCallbackCommand, + OAuthCallbackResult, + OAuthInvitationResult, ) -from services.errors.workspace import WorkSpaceNotAllowedCreateError, WorkSpaceNotFoundError -from services.system_feature_service import SystemFeatureService from .. import console_ns -logger = logging.getLogger(__name__) - class OAuthLoginQuery(BaseModel): invite_token: str | None = Field(default=None, description="Optional invitation token") @@ -55,31 +59,12 @@ class OAuthCallbackQuery(BaseModel): state: str | None = Field(default=None, description="OAuth state parameter") +class OAuthErrorResponse(ResponseModel): + error: str = Field(description="OAuth error message") + + register_schema_models(console_ns, OAuthLoginQuery, OAuthCallbackQuery) -register_response_schema_model(console_ns, RedirectResponse) - - -def get_oauth_providers(): - with current_app.app_context(): - if not dify_config.GITHUB_CLIENT_ID or not dify_config.GITHUB_CLIENT_SECRET: - github_oauth = None - else: - github_oauth = GitHubOAuth( - client_id=dify_config.GITHUB_CLIENT_ID, - client_secret=dify_config.GITHUB_CLIENT_SECRET, - redirect_uri=dify_config.CONSOLE_API_URL + "/console/api/oauth/authorize/github", - ) - if not dify_config.GOOGLE_CLIENT_ID or not dify_config.GOOGLE_CLIENT_SECRET: - google_oauth = None - else: - google_oauth = GoogleOAuth( - client_id=dify_config.GOOGLE_CLIENT_ID, - client_secret=dify_config.GOOGLE_CLIENT_SECRET, - redirect_uri=dify_config.CONSOLE_API_URL + "/console/api/oauth/authorize/google", - ) - - OAUTH_PROVIDERS = {"github": github_oauth, "google": google_oauth} - return OAUTH_PROVIDERS +register_response_schema_models(console_ns, RedirectResponse, OAuthErrorResponse) def _validated_timezone(value: str | None) -> str | None: @@ -97,22 +82,33 @@ def _validated_language(value: str | None) -> str | None: return None -def _url_origin(url: str) -> tuple[str, str, int] | None: - parsed_url = urllib.parse.urlsplit(url) - if parsed_url.scheme not in {"http", "https"} or parsed_url.hostname is None: - return None - - try: - port = parsed_url.port - except ValueError: - return None - - if port is None: - port = 443 if parsed_url.scheme == "https" else 80 - return parsed_url.scheme, parsed_url.hostname, port +def _preferred_interface_language() -> str | None: + preferred_lang = request.accept_languages.best_match(languages) + if preferred_lang and preferred_lang in languages: + return preferred_lang + return None -def _get_redirect_target(redirect_url: str | None) -> str: +def _redirect_with_console_session(tokens: AccountSessionTokens, target_url: str) -> Response: + """Attach application-issued Console session cookies to a redirect response.""" + response = redirect(target_url) + set_access_token_to_cookie(request, response, tokens.access_token) + set_refresh_token_to_cookie(request, response, tokens.refresh_token) + set_csrf_token_to_cookie(request, response, tokens.csrf_token) + return response + + +def _oauth_callback_target(result: OAuthCallbackResult, requested_redirect: str | None) -> str: + if isinstance(result, OAuthInvitationResult): + query = urllib.parse.urlencode({"invite_token": result.invite_token}) + return f"{dify_config.CONSOLE_WEB_URL}/signin/invite-settings?{query}" + + target_url = _safe_console_redirect_target(requested_redirect) + query_char = "&" if "?" in target_url else "?" + return f"{target_url}{query_char}oauth_new_user={str(result.oauth_new_user).lower()}" + + +def _safe_console_redirect_target(redirect_url: str | None) -> str: if not redirect_url: return dify_config.CONSOLE_WEB_URL @@ -127,28 +123,22 @@ def _get_redirect_target(redirect_url: str | None) -> str: return dify_config.CONSOLE_WEB_URL -def _preferred_interface_language(language: str | None = None) -> str: - if language: - return language - - preferred_lang = request.accept_languages.best_match(languages) - if preferred_lang and preferred_lang in languages: - return preferred_lang - return languages[0] +def _url_origin(url: str) -> tuple[str, str, int] | None: + parsed_url = urllib.parse.urlsplit(url) + if parsed_url.scheme not in {"http", "https"} or parsed_url.hostname is None: + return None + try: + port = parsed_url.port + except ValueError: + return None + if port is None: + port = 443 if parsed_url.scheme == "https" else 80 + return parsed_url.scheme, parsed_url.hostname, port -def _redirect_with_console_session(account: Account, target_url: str) -> Response: - """Create a console session and attach its cookies to a redirect response.""" - token_pair = AccountService.login( - account=account, - session=db.session(), - ip_address=extract_remote_ip(request), - ) - response = redirect(target_url) - set_access_token_to_cookie(request, response, token_pair.access_token) - set_refresh_token_to_cookie(request, response, token_pair.refresh_token) - set_csrf_token_to_cookie(request, response, token_pair.csrf_token) - return response +def _signin_redirect(message: str, **params: str) -> Response: + query = urllib.parse.urlencode({"message": message, **params}) + return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?{query}") @console_ns.route("/oauth/login/") @@ -158,24 +148,23 @@ class OAuthLogin(Resource): @console_ns.doc(params={"provider": "OAuth provider name (github/google)"}) @console_ns.doc(params=query_params_from_model(OAuthLoginQuery)) @console_ns.response(302, "Redirect to OAuth authorization URL", console_ns.models[RedirectResponse.__name__]) - @console_ns.response(400, "Invalid provider") - def get(self, provider: str): - invite_token = request.args.get("invite_token") or None - timezone = _validated_timezone(request.args.get("timezone") or None) - language = _validated_language(request.args.get("language") or None) - redirect_url = request.args.get("redirect_url") or None - OAUTH_PROVIDERS = get_oauth_providers() - with current_app.app_context(): - oauth_provider = OAUTH_PROVIDERS.get(provider) - if not oauth_provider: - return {"error": "Invalid provider"}, 400 - - auth_url = oauth_provider.get_authorization_url( - invite_token=invite_token, - timezone=timezone, - language=language, - redirect_url=redirect_url, - ) + @console_ns.response(400, "Invalid provider", console_ns.models[OAuthErrorResponse.__name__]) + @setup_required + @social_oauth_login_enabled + @model_validate(OAuthLoginQuery) + def get(self, req_data: OAuthLoginQuery, provider: str): + try: + auth_url = application_services().accounts.oauth.start_authorization( + provider, + OAuthAuthorizationRequest( + invite_token=req_data.invite_token or None, + timezone=_validated_timezone(req_data.timezone), + language=_validated_language(req_data.language), + redirect_url=req_data.redirect_url or None, + ), + ) + except InvalidOAuthProviderError: + return dump_response(OAuthErrorResponse, {"error": "Invalid provider"}), 400 return redirect(auth_url) @@ -186,161 +175,53 @@ class OAuthCallback(Resource): @console_ns.doc(params={"provider": "OAuth provider name (github/google)"}) @console_ns.doc(params=query_params_from_model(OAuthCallbackQuery)) @console_ns.response(302, "Redirect to console with access token", console_ns.models[RedirectResponse.__name__]) - @console_ns.response(400, "OAuth process failed") - def get(self, provider: str): - OAUTH_PROVIDERS = get_oauth_providers() - with current_app.app_context(): - oauth_provider = OAUTH_PROVIDERS.get(provider) - if not oauth_provider: - return {"error": "Invalid provider"}, 400 - - code = request.args.get("code") - state = request.args.get("state") - oauth_state = decode_oauth_state(state) - invite_token = oauth_state.get("invite_token") - timezone = _validated_timezone(oauth_state.get("timezone")) - language = _validated_language(oauth_state.get("language")) - redirect_url = oauth_state.get("redirect_url") - - if not code: - return {"error": "Authorization code is required"}, 400 - + @console_ns.response(400, "OAuth process failed", console_ns.models[OAuthErrorResponse.__name__]) + @setup_required + @social_oauth_login_enabled + @model_validate(OAuthCallbackQuery) + def get(self, req_data: OAuthCallbackQuery, provider: str): + oauth_state = decode_oauth_state(req_data.state) try: - token = oauth_provider.get_access_token(code) - user_info = oauth_provider.get_user_info(token) - except httpx.RequestError as e: - error_text = str(e) - if isinstance(e, httpx.HTTPStatusError): - error_text = e.response.text - logger.exception("An error occurred during the OAuth process with %s: %s", provider, error_text) - return {"error": "OAuth process failed"}, 400 - except ValueError as e: - logger.warning("OAuth error with %s", provider, exc_info=True) - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?message={urllib.parse.quote(str(e))}") - - if invite_token and RegisterService.is_valid_invite_token(invite_token): - invitation = RegisterService.get_invitation_if_token_valid( - None, - None, - invite_token, - session=db.session(), + result = application_services().accounts.oauth.complete_authorization( + OAuthCallbackCommand( + provider=provider, + code=req_data.code, + invite_token=oauth_state.get("invite_token"), + timezone=_validated_timezone(oauth_state.get("timezone")), + language=_validated_language(oauth_state.get("language")), + browser_language=_preferred_interface_language(), + ip_address=extract_remote_ip(request), + ) ) - if not invitation: - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?message=Invalid invitation token.") - if invitation["data"]["email"].lower() != user_info.email.lower(): - message = "This invitation was sent to another account. Please sign in with the invited account." - query = urllib.parse.urlencode({"message": message, "invite_token": invite_token}) - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?{query}") - - account = invitation["account"] - if account.status == AccountStatus.BANNED: - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?message=Account is banned.") - - AccountService.link_account_integrate(provider, user_info.id, account, session=db.session()) - target_url = f"{dify_config.CONSOLE_WEB_URL}/signin/invite-settings?invite_token={invite_token}" - return _redirect_with_console_session(account, target_url) - - try: - account, oauth_new_user = _generate_account( - provider, - user_info, - timezone=timezone, - language=language, - ip_address=extract_remote_ip(request), + except InvalidOAuthProviderError: + return dump_response(OAuthErrorResponse, {"error": "Invalid provider"}), 400 + except (OAuthProviderRequestError, OAuthIdentityLockUnavailableError): + return dump_response(OAuthErrorResponse, {"error": "OAuth process failed"}), 400 + except OAuthProviderAuthorizationError as exc: + return _signin_redirect(exc.description) + except InvalidOAuthInvitationError: + return _signin_redirect("Invalid invitation token.") + except OAuthInvitationAccountMismatchError as exc: + return _signin_redirect( + "This invitation was sent to another account. Please sign in with the invited account.", + invite_token=exc.invite_token, ) - except AccountNotFoundError: - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?message=Account not found.") - except (WorkSpaceNotFoundError, WorkSpaceNotAllowedCreateError): - return redirect( - f"{dify_config.CONSOLE_WEB_URL}/signin" - "?message=Workspace not found, please contact system admin to invite you to join in a workspace." + except OAuthAccountBannedError: + return _signin_redirect("Account is banned.") + except OAuthAccountNotFoundError: + return _signin_redirect("Account not found.") + except OAuthWorkspaceCreationNotAllowedError: + return _signin_redirect( + "Workspace not found, please contact system admin to invite you to join in a workspace." ) - except SeatsLimitExceededError: - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?message=Licensed seats limit exceeded.") - except EmailDomainSuspendedRegistrationError: - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?message={EmailDomainSuspendedError.description}") - except AccountRegisterError as exc: - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?message={exc.description}") + except OAuthSeatsLimitExceededError: + return _signin_redirect("Licensed seats limit exceeded.") + except AccountEmailDomainSuspendedError: + return _signin_redirect(EmailDomainSuspendedError.description or "") + except AccountEmailFrozenError: + return _signin_redirect(AccountInFreezeError.description or "") + except OAuthRegistrationError as exc: + return _signin_redirect(exc.description) - # Check account status - if account.status == AccountStatus.BANNED: - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?message=Account is banned.") - - if account.status == AccountStatus.PENDING: - account.status = AccountStatus.ACTIVE - account.initialized_at = naive_utc_now() - db.session.commit() - - try: - TenantService.create_owner_tenant_if_not_exist(account, session=db.session()) - except Unauthorized: - return redirect(f"{dify_config.CONSOLE_WEB_URL}/signin?message=Workspace not found.") - except WorkSpaceNotAllowedCreateError: - return redirect( - f"{dify_config.CONSOLE_WEB_URL}/signin" - "?message=Workspace not found, please contact system admin to invite you to join in a workspace." - ) - - target_url = _get_redirect_target(redirect_url) - query_char = "&" if "?" in target_url else "?" - target_url = f"{target_url}{query_char}oauth_new_user={str(oauth_new_user).lower()}" - return _redirect_with_console_session(account, target_url) - - -def _get_account_by_openid_or_email(provider: str, user_info: OAuthUserInfo) -> Account | None: - account: Account | None = Account.get_by_openid(provider, user_info.id) - - if not account: - account = AccountService.get_account_by_email_with_case_fallback(user_info.email, session=db.session()) - - return account - - -def _generate_account( - provider: str, - user_info: OAuthUserInfo, - timezone: str | None = None, - language: str | None = None, - ip_address: str | None = None, -) -> tuple[Account, bool]: - # Get account by openid or email. - account = _get_account_by_openid_or_email(provider, user_info) - oauth_new_user = False - - if account: - tenants = TenantService.get_join_tenants(account, session=db.session()) - if not tenants: - if not SystemFeatureService.is_workspace_creation_allowed(): - raise WorkSpaceNotAllowedCreateError() - else: - TenantService.create_owner_tenant(account, session=db.session()) - - if not account: - normalized_email = user_info.email.lower() - oauth_new_user = True - if not SystemFeatureService.is_registration_allowed(): - if dify_config.DEPLOYMENT_EDITION == DeploymentEdition.CLOUD: - freeze_type = BillingService.get_email_freeze_type(normalized_email) - if freeze_type: - if freeze_type == "email_domain_suspended": - raise EmailDomainSuspendedRegistrationError() - raise AccountRegisterError(description=AccountInFreezeError.description or "") - raise AccountRegisterError(description=("Invalid email or password")) - account_name = user_info.name or "Dify" - interface_language = _preferred_interface_language(language) - account = RegisterService.register( - email=normalized_email, - name=account_name, - password=None, - open_id=user_info.id, - provider=provider, - language=interface_language, - timezone=timezone, - ip_address=ip_address, - session=db.session(), - ) - - # Link account - AccountService.link_account_integrate(provider, user_info.id, account, session=db.session()) - - return account, oauth_new_user + target_url = _oauth_callback_target(result, oauth_state.get("redirect_url")) + return _redirect_with_console_session(result.tokens, target_url) diff --git a/api/controllers/console/wraps.py b/api/controllers/console/wraps.py index dd681a3b5e5..a56a6de5b14 100644 --- a/api/controllers/console/wraps.py +++ b/api/controllers/console/wraps.py @@ -356,6 +356,16 @@ def email_password_login_enabled[**P, R](view: Callable[P, R]) -> Callable[P, R] return decorated +def social_oauth_login_enabled[**P, R](view: Callable[P, R]) -> Callable[P, R]: + @wraps(view) + def decorated(*args: P.args, **kwargs: P.kwargs): + if not dify_config.ENABLE_SOCIAL_OAUTH_LOGIN: + abort(403) + return view(*args, **kwargs) + + return decorated + + def enable_change_email[**P, R](view: Callable[P, R]) -> Callable[P, R]: @wraps(view) def decorated(*args: P.args, **kwargs: P.kwargs): diff --git a/api/extensions/ext_application_services.py b/api/extensions/ext_application_services.py index 65b948ccb9f..5885589c27b 100644 --- a/api/extensions/ext_application_services.py +++ b/api/extensions/ext_application_services.py @@ -14,6 +14,7 @@ from sqlalchemy.orm import Session, sessionmaker from configs import dify_config from constants.dsl_version import CURRENT_APP_DSL_VERSION +from constants.languages import languages from core.db.session_factory import get_session_maker from core.helper.ssrf_proxy import ssrf_proxy from core.schemas.schema_manager import SchemaManager @@ -21,9 +22,16 @@ from enums import DeploymentEdition, WebAppAccessMode from extensions.ext_redis import RedisClientWrapper, redis_client from libs.datetime_utils import naive_utc_now from libs.helper import RateLimiter +from libs.oauth import GitHubOAuth, GoogleOAuth from libs.passport import PassportService from repositories.account_activation_repository import SQLAlchemyAccountActivationRepository from repositories.account_integration_repository import SQLAlchemyAccountIntegrationRepository +from repositories.account_oauth_repository import ( + AccountServiceOAuthAccountRegistrationGateway, + AccountServiceOAuthSessionGateway, + AccountServiceOAuthWorkspaceGateway, + RegisterServiceOAuthInvitationGateway, +) from repositories.account_repository import SQLAlchemyAccountRepository from repositories.app_definition_query_repository import AppDefinitionQueryRepository from repositories.app_site_command_repository import AppSiteCommandRepository @@ -103,6 +111,12 @@ from services.account_login_adapters import ( TurnstileHumanVerificationGateway, ) from services.account_login_service import ConsoleAuthenticationService +from services.account_oauth_adapters import ( + DeploymentOAuthPolicyGateway, + DifyOAuthProviderGateway, + RedisOAuthAccountClaimLock, +) +from services.account_oauth_service import AccountOAuthService, OAuthProviderGateway from services.account_password_hasher import DefaultAccountPasswordHasher from services.account_password_service import AccountPasswordService from services.account_profile_service import AccountProfileService @@ -200,6 +214,7 @@ class AccountServices: forgot_password: AccountForgotPasswordService initialization: AccountInitializationService integrations: AccountIntegrationService + oauth: AccountOAuthService password: AccountPasswordService profile: AccountProfileService @@ -278,6 +293,55 @@ def _build_oauth_server_service( ) +def _build_account_oauth_service( + *, + database_client: sessionmaker[Session], + deployment_edition: DeploymentEdition, + redis: RedisClientWrapper, + accounts: SQLAlchemyAccountRepository, + integrations: SQLAlchemyAccountIntegrationRepository, + memberships: WorkspaceQueryRepository, +) -> AccountOAuthService: + providers: dict[str, OAuthProviderGateway] = {} + if dify_config.GITHUB_CLIENT_ID and dify_config.GITHUB_CLIENT_SECRET: + providers["github"] = DifyOAuthProviderGateway( + provider_name="github", + client=GitHubOAuth( + client_id=dify_config.GITHUB_CLIENT_ID, + client_secret=dify_config.GITHUB_CLIENT_SECRET, + redirect_uri=dify_config.CONSOLE_API_URL + "/console/api/oauth/authorize/github", + ), + ) + if dify_config.GOOGLE_CLIENT_ID and dify_config.GOOGLE_CLIENT_SECRET: + providers["google"] = DifyOAuthProviderGateway( + provider_name="google", + client=GoogleOAuth( + client_id=dify_config.GOOGLE_CLIENT_ID, + client_secret=dify_config.GOOGLE_CLIENT_SECRET, + redirect_uri=dify_config.CONSOLE_API_URL + "/console/api/oauth/authorize/google", + ), + ) + + policy = DeploymentOAuthPolicyGateway( + billing_enabled=deployment_edition == DeploymentEdition.CLOUD, + ) + return AccountOAuthService( + providers=providers, + accounts=accounts, + integrations=integrations, + memberships=memberships, + invitations=RegisterServiceOAuthInvitationGateway(session_factory=database_client), + account_claims=RedisOAuthAccountClaimLock(client=redis), + registration=AccountServiceOAuthAccountRegistrationGateway(session_factory=database_client), + workspaces=AccountServiceOAuthWorkspaceGateway(session_factory=database_client), + sessions=AccountServiceOAuthSessionGateway(session_factory=database_client), + registration_policy=policy, + workspace_policy=policy, + supported_languages=languages, + now=naive_utc_now, + ) + + def build_application_services( *, database_client: sessionmaker[Session], @@ -426,6 +490,14 @@ def build_application_services( now=naive_utc_now, ), integrations=AccountIntegrationService(integrations=integrations), + oauth=_build_account_oauth_service( + database_client=database_client, + deployment_edition=deployment_edition, + redis=redis, + accounts=accounts, + integrations=integrations, + memberships=workspace_query_repository, + ), password=AccountPasswordService( accounts=accounts, passwords=passwords, diff --git a/api/models/account.py b/api/models/account.py index 822b784a2fa..6700fc86c12 100644 --- a/api/models/account.py +++ b/api/models/account.py @@ -187,15 +187,6 @@ class Account(UserMixin, TypeBase): def get_status(self) -> AccountStatus: return self.status - @classmethod - def get_by_openid(cls, provider: str, open_id: str): - account_integrate = db.session.execute( - select(AccountIntegrate).where(AccountIntegrate.provider == provider, AccountIntegrate.open_id == open_id) - ).scalar_one_or_none() - if account_integrate: - return db.session.scalar(select(Account).where(Account.id == account_integrate.account_id)) - return None - # check current_user.current_tenant.current_role in ['admin', 'owner'] @property def is_admin_or_owner(self): diff --git a/api/openapi/markdown/console-openapi.md b/api/openapi/markdown/console-openapi.md index 4a63869ccd9..018fe4e47a8 100644 --- a/api/openapi/markdown/console-openapi.md +++ b/api/openapi/markdown/console-openapi.md @@ -7266,7 +7266,7 @@ Handle OAuth callback and complete login process | Code | Description | Schema | | ---- | ----------- | ------ | | 302 | Redirect to console with access token | **application/json**: [RedirectResponse](#redirectresponse)
| -| 400 | OAuth process failed | | +| 400 | OAuth process failed | **application/json**: [OAuthErrorResponse](#oautherrorresponse)
| ### [GET] /oauth/data-source/binding/{provider} Bind OAuth data source with authorization code @@ -7355,7 +7355,7 @@ Initiate OAuth login process | Code | Description | Schema | | ---- | ----------- | ------ | | 302 | Redirect to OAuth authorization URL | **application/json**: [RedirectResponse](#redirectresponse)
| -| 400 | Invalid provider | | +| 400 | Invalid provider | **application/json**: [OAuthErrorResponse](#oautherrorresponse)
| ### [GET] /oauth/plugin/{provider_id}/datasource/callback #### Parameters @@ -19781,6 +19781,12 @@ Coarse node-level status used by Inspector to pick a banner. | ---- | ---- | ----------- | -------- | | result | string | Operation result | Yes | +#### OAuthErrorResponse + +| Name | Type | Description | Required | +| ---- | ---- | ----------- | -------- | +| error | string | OAuth error message | Yes | + #### OAuthLoginQuery | Name | Type | Description | Required | diff --git a/api/repositories/account_integration_repository.py b/api/repositories/account_integration_repository.py index 949d6a2e408..5b1abd39419 100644 --- a/api/repositories/account_integration_repository.py +++ b/api/repositories/account_integration_repository.py @@ -14,6 +14,15 @@ class SQLAlchemyAccountIntegrationRepository(AccountIntegrationRepository): def __init__(self, session_factory: sessionmaker[Session]) -> None: self._session_factory = session_factory + @override + def find_account_id(self, *, provider: str, open_id: str) -> str | None: + with self._session_factory() as session: + return session.scalar( + select(AccountIntegrate.account_id) + .where(AccountIntegrate.provider == provider, AccountIntegrate.open_id == open_id) + .limit(1) + ) + @override def list_for_account(self, account_id: str) -> list[AccountIntegrationSnapshot]: with self._session_factory() as session: @@ -23,3 +32,27 @@ class SQLAlchemyAccountIntegrationRepository(AccountIntegrationRepository): ) ).all() return [AccountIntegrationSnapshot(provider=row.provider, created_at=row.created_at) for row in rows] + + @override + def link(self, account_id: str, *, provider: str, open_id: str) -> None: + with self._session_factory.begin() as session: + integration = session.scalar( + select(AccountIntegrate) + .where( + AccountIntegrate.account_id == account_id, + AccountIntegrate.provider == provider, + ) + .limit(1) + ) + if integration is None: + session.add( + AccountIntegrate( + account_id=account_id, + provider=provider, + open_id=open_id, + encrypted_token="", + ) + ) + return + integration.open_id = open_id + integration.encrypted_token = "" diff --git a/api/repositories/account_oauth_repository.py b/api/repositories/account_oauth_repository.py new file mode 100644 index 00000000000..3f43cfeb54e --- /dev/null +++ b/api/repositories/account_oauth_repository.py @@ -0,0 +1,128 @@ +"""Persistence-backed gateways for Console account OAuth sign-in.""" + +from typing import override + +from sqlalchemy.orm import Session, sessionmaker + +from libs.datetime_utils import naive_utc_now +from models.account import Account, AccountStatus +from services.account_errors import ( + AccountEmailDomainSuspendedError, + OAuthAccountNotFoundError, + OAuthRegistrationError, + OAuthSeatsLimitExceededError, + OAuthWorkspaceCreationNotAllowedError, +) +from services.account_oauth_service import ( + OAuthAccountRegistrationGateway, + OAuthInvitationGateway, + OAuthSessionGateway, + OAuthWorkspaceGateway, +) +from services.account_service import AccountService, RegisterService, TenantService +from services.enterprise.enterprise_service import try_join_default_workspace +from services.entities.account_entities import AccountSessionTokens +from services.entities.account_oauth_entities import ( + OAuthAccountRegistration, + OAuthInvitation, +) +from services.errors.account import AccountRegisterError, EmailDomainSuspendedError, SeatsLimitExceededError +from services.errors.workspace import WorkSpaceNotAllowedCreateError, WorkspacesLimitExceededError + + +class RegisterServiceOAuthInvitationGateway(OAuthInvitationGateway): + def __init__(self, *, session_factory: sessionmaker[Session]) -> None: + self._session_factory = session_factory + + @override + def resolve(self, invite_token: str) -> OAuthInvitation | None: + with self._session_factory() as session: + invitation = RegisterService.get_invitation_if_token_valid( + None, + None, + invite_token, + session=session, + ) + if invitation is None: + return None + account = invitation["account"] + return OAuthInvitation( + account_id=account.id, + account_email=account.email, + account_status=account.status.value, + ) + + +class AccountServiceOAuthAccountRegistrationGateway(OAuthAccountRegistrationGateway): + def __init__(self, *, session_factory: sessionmaker[Session]) -> None: + self._session_factory = session_factory + + @override + def register(self, registration: OAuthAccountRegistration) -> str: + with self._session_factory() as session: + try: + account = AccountService.create_account( + email=registration.email, + name=registration.name, + interface_language=registration.language, + password=None, + timezone=registration.timezone, + ip_address=registration.ip_address, + check_normalized_email=True, + session=session, + ) + account.status = AccountStatus.ACTIVE + account.initialized_at = naive_utc_now() + session.commit() + except EmailDomainSuspendedError as exc: + raise AccountEmailDomainSuspendedError from exc + except SeatsLimitExceededError as exc: + raise OAuthSeatsLimitExceededError from exc + except AccountRegisterError as exc: + raise OAuthRegistrationError(exc.description) from exc + except Exception as exc: + session.rollback() + raise OAuthRegistrationError(f"Registration failed: {exc}") from exc + return account.id + + +class AccountServiceOAuthWorkspaceGateway(OAuthWorkspaceGateway): + """Adapt account workspace operations to the OAuth application port.""" + + def __init__(self, *, session_factory: sessionmaker[Session]) -> None: + self._session_factory = session_factory + + @override + def create_owner_workspace(self, account_id: str) -> None: + with self._session_factory() as session: + account = session.get(Account, account_id) + if account is None: + raise OAuthAccountNotFoundError + try: + TenantService.create_owner_tenant(account, session=session) + except (WorkSpaceNotAllowedCreateError, WorkspacesLimitExceededError) as exc: + raise OAuthWorkspaceCreationNotAllowedError from exc + + @override + def try_join_default_workspace(self, account_id: str) -> None: + try_join_default_workspace(account_id) + + +class AccountServiceOAuthSessionGateway(OAuthSessionGateway): + """Adapt Console session issuance to the OAuth application port.""" + + def __init__(self, *, session_factory: sessionmaker[Session]) -> None: + self._session_factory = session_factory + + @override + def login(self, account_id: str, *, ip_address: str) -> AccountSessionTokens: + with self._session_factory() as session: + account = session.get(Account, account_id) + if account is None: + raise OAuthAccountNotFoundError + token_pair = AccountService.login(account=account, session=session, ip_address=ip_address) + return AccountSessionTokens( + access_token=token_pair.access_token, + refresh_token=token_pair.refresh_token, + csrf_token=token_pair.csrf_token, + ) diff --git a/api/repositories/account_repository.py b/api/repositories/account_repository.py index 1f1c988fb83..65c352384d6 100644 --- a/api/repositories/account_repository.py +++ b/api/repositories/account_repository.py @@ -1,5 +1,6 @@ """SQLAlchemy implementation of the account persistence port.""" +from datetime import datetime from typing import override from sqlalchemy import case, delete, select @@ -45,6 +46,15 @@ class SQLAlchemyAccountRepository(AccountRepository, ConsoleAuthAccountRepositor account = session.execute(select(Account).where(Account.email == email.lower())).scalar_one_or_none() return self._to_snapshot(account) if account is not None else None + @override + def activate_pending(self, account_id: str, *, initialized_at: datetime) -> None: + with self._session_factory.begin() as session: + account = session.get(Account, account_id) + if account is None or account.status != AccountStatus.PENDING: + return + account.status = AccountStatus.ACTIVE + account.initialized_at = initialized_at + @override def get_credentials(self, account_id: str) -> AccountCredentials | None: with self._session_factory() as session: diff --git a/api/repositories/workspace_query_repository.py b/api/repositories/workspace_query_repository.py index 9af32673fa5..84758d997be 100644 --- a/api/repositories/workspace_query_repository.py +++ b/api/repositories/workspace_query_repository.py @@ -65,3 +65,7 @@ class WorkspaceQueryRepository(WorkspaceQuery, AccountWorkspaceMembershipQuery, ) with self._session_factory() as session: return session.scalar(stmt) is not None + + @override + def has_active_membership(self, account_id: str) -> bool: + return self.has_active_for_account(account_id) diff --git a/api/services/account_errors.py b/api/services/account_errors.py index b64b245ddac..773ae90ea46 100644 --- a/api/services/account_errors.py +++ b/api/services/account_errors.py @@ -161,6 +161,62 @@ class EducationDiscountPausedError(AccountApplicationError): """Education discount activation is temporarily paused.""" +class InvalidOAuthProviderError(AccountApplicationError): + """The requested Console OAuth provider is unavailable.""" + + +class OAuthProviderRequestError(AccountApplicationError): + """The remote OAuth provider could not complete the request.""" + + +class OAuthProviderAuthorizationError(AccountApplicationError): + """The remote OAuth provider rejected the authorization exchange.""" + + def __init__(self, description: str) -> None: + super().__init__(description) + self.description = description + + +class OAuthIdentityLockUnavailableError(AccountApplicationError): + """The OAuth account claim could not be acquired or its lease was lost.""" + + +class InvalidOAuthInvitationError(AccountApplicationError): + """The OAuth callback references an invitation that can no longer be resolved.""" + + +class OAuthInvitationAccountMismatchError(AccountApplicationError): + """The OAuth identity does not own the account referenced by the invitation.""" + + def __init__(self, invite_token: str) -> None: + super().__init__(invite_token) + self.invite_token = invite_token + + +class OAuthAccountBannedError(AccountApplicationError): + """The OAuth identity resolves to a banned Console account.""" + + +class OAuthAccountNotFoundError(AccountApplicationError): + """An account disappeared while the OAuth use case was running.""" + + +class OAuthWorkspaceCreationNotAllowedError(AccountApplicationError): + """Workspace policy prevents provisioning a workspace for the OAuth account.""" + + +class OAuthSeatsLimitExceededError(AccountApplicationError): + """Account registration would exceed the licensed seat limit.""" + + +class OAuthRegistrationError(AccountApplicationError): + """The OAuth account could not be registered.""" + + def __init__(self, description: str) -> None: + super().__init__(description) + self.description = description + + class EducationRateLimitExceededError(AccountApplicationError): """Too many education verification or activation requests were made.""" diff --git a/api/services/account_oauth_adapters.py b/api/services/account_oauth_adapters.py new file mode 100644 index 00000000000..c9586958311 --- /dev/null +++ b/api/services/account_oauth_adapters.py @@ -0,0 +1,203 @@ +"""Infrastructure gateways for Console account OAuth sign-in.""" + +import logging +from collections.abc import Generator +from contextlib import AbstractContextManager, contextmanager +from hashlib import sha256 +from threading import Event, Thread +from typing import Protocol, override + +import httpx +from redis import RedisError +from redis.exceptions import LockError + +from extensions.ext_redis import RedisClientWrapper +from libs.oauth import OAuth +from services.account_errors import ( + OAuthIdentityLockUnavailableError, + OAuthProviderAuthorizationError, + OAuthProviderRequestError, +) +from services.account_oauth_service import ( + OAuthAccountClaimLease, + OAuthAccountClaimLock, + OAuthProviderGateway, + OAuthRegistrationPolicyGateway, + OAuthWorkspacePolicyGateway, +) +from services.billing_service import BillingService +from services.entities.account_oauth_entities import OAuthAuthorizationRequest, OAuthIdentity +from services.system_feature_service import SystemFeatureService + +logger = logging.getLogger(__name__) + +_OAUTH_ACCOUNT_CLAIM_LOCK_PREFIX = "oauth:account-claim:" +_OAUTH_ACCOUNT_CLAIM_LOCK_TIMEOUT_SECONDS = 60 +_OAUTH_ACCOUNT_CLAIM_LOCK_BLOCKING_TIMEOUT_SECONDS = 10 +_OAUTH_ACCOUNT_CLAIM_LOCK_RENEW_INTERVAL_SECONDS = 20 +_OAUTH_ACCOUNT_CLAIM_LOCK_HEARTBEAT_JOIN_TIMEOUT_SECONDS = 2 + + +class _RedisLock(Protocol): + def acquire(self) -> bool: ... + + def reacquire(self) -> bool: ... + + def release(self) -> None: ... + + +class _RedisOAuthAccountClaimLease(OAuthAccountClaimLease): + def __init__(self, *, locks: tuple[_RedisLock, ...], lost: Event) -> None: + self._locks = locks + self._lost = lost + + @override + def ensure_owned(self) -> None: + if self._lost.is_set(): + raise OAuthIdentityLockUnavailableError + try: + for lock in self._locks: + lock.reacquire() + except (LockError, RedisError) as exc: + self._lost.set() + raise OAuthIdentityLockUnavailableError from exc + except Exception as exc: + self._lost.set() + raise OAuthIdentityLockUnavailableError from exc + + def mark_lost(self) -> None: + self._lost.set() + + +class DifyOAuthProviderGateway(OAuthProviderGateway): + def __init__(self, *, provider_name: str, client: OAuth) -> None: + self._provider_name = provider_name + self._client = client + + @override + def get_authorization_url(self, request: OAuthAuthorizationRequest) -> str: + return self._client.get_authorization_url( + invite_token=request.invite_token, + timezone=request.timezone, + language=request.language, + redirect_url=request.redirect_url, + ) + + @override + def get_identity(self, code: str) -> OAuthIdentity: + try: + token = self._client.get_access_token(code) + user_info = self._client.get_user_info(token) + except httpx.HTTPError as exc: + error_text = exc.response.text if isinstance(exc, httpx.HTTPStatusError) else str(exc) + logger.exception( + "An error occurred during the OAuth process with %s: %s", + self._provider_name, + error_text, + ) + raise OAuthProviderRequestError from exc + except ValueError as exc: + logger.warning("OAuth error with %s", self._provider_name, exc_info=True) + raise OAuthProviderAuthorizationError(str(exc)) from exc + return OAuthIdentity(id=user_info.id, name=user_info.name, email=user_info.email) + + +class RedisOAuthAccountClaimLock(OAuthAccountClaimLock): + def __init__(self, *, client: RedisClientWrapper) -> None: + self._client = client + + @override + def acquire(self, *, provider: str, open_id: str, email: str) -> AbstractContextManager[OAuthAccountClaimLease]: + return self._acquire( + lock_names=( + self._lock_name("identity", provider, open_id), + self._lock_name("email", email), + ) + ) + + @override + def acquire_account(self, account_id: str) -> AbstractContextManager[OAuthAccountClaimLease]: + return self._acquire(lock_names=(self._lock_name("account", account_id),)) + + @contextmanager + def _acquire(self, *, lock_names: tuple[str, ...]) -> Generator[OAuthAccountClaimLease, None, None]: + sorted_lock_names = sorted(set(lock_names)) + locks: list[_RedisLock] = [] + try: + for lock_name in sorted_lock_names: + lock = self._client.lock( + lock_name, + timeout=_OAUTH_ACCOUNT_CLAIM_LOCK_TIMEOUT_SECONDS, + blocking_timeout=_OAUTH_ACCOUNT_CLAIM_LOCK_BLOCKING_TIMEOUT_SECONDS, + thread_local=False, + ) + if not lock.acquire(): + raise OAuthIdentityLockUnavailableError + locks.append(lock) + except (LockError, RedisError) as exc: + self._release(locks) + raise OAuthIdentityLockUnavailableError from exc + except OAuthIdentityLockUnavailableError: + self._release(locks) + raise + + stop_heartbeat = Event() + lease = _RedisOAuthAccountClaimLease(locks=tuple(locks), lost=Event()) + heartbeat = Thread( + target=self._renew_while_held, + args=(lease, stop_heartbeat), + daemon=True, + name=f"OAuthAccountClaimLock({sha256(''.join(sorted_lock_names).encode()).hexdigest()[:12]})", + ) + heartbeat.start() + try: + yield lease + lease.ensure_owned() + finally: + stop_heartbeat.set() + heartbeat.join(timeout=_OAUTH_ACCOUNT_CLAIM_LOCK_HEARTBEAT_JOIN_TIMEOUT_SECONDS) + if heartbeat.is_alive(): + logger.warning("OAuth account claim lock heartbeat did not stop before release") + self._release(locks) + + @staticmethod + def _lock_name(kind: str, *parts: str) -> str: + digest = sha256("\0".join((kind, *parts)).encode()).hexdigest() + return f"{_OAUTH_ACCOUNT_CLAIM_LOCK_PREFIX}{digest}" + + @staticmethod + def _release(locks: list[_RedisLock]) -> None: + for lock in reversed(locks): + try: + lock.release() + except (LockError, RedisError): + logger.warning("Failed to release OAuth account claim lock", exc_info=True) + + @staticmethod + def _renew_while_held(lease: _RedisOAuthAccountClaimLease, stop_heartbeat: Event) -> None: + while not stop_heartbeat.wait(_OAUTH_ACCOUNT_CLAIM_LOCK_RENEW_INTERVAL_SECONDS): + try: + lease.ensure_owned() + except OAuthIdentityLockUnavailableError: + lease.mark_lost() + logger.error("OAuth account claim lock ownership was lost; stop renewing", exc_info=True) + return + + +class DeploymentOAuthPolicyGateway(OAuthRegistrationPolicyGateway, OAuthWorkspacePolicyGateway): + def __init__(self, *, billing_enabled: bool) -> None: + self._billing_enabled = billing_enabled + + @override + def is_registration_allowed(self) -> bool: + return SystemFeatureService.is_registration_allowed() + + @override + def get_freeze_type(self, email: str) -> str | None: + if not self._billing_enabled: + return None + return BillingService.get_email_freeze_type(email) + + @override + def is_creation_allowed(self) -> bool: + return SystemFeatureService.is_workspace_creation_allowed() diff --git a/api/services/account_oauth_service.py b/api/services/account_oauth_service.py new file mode 100644 index 00000000000..3a8354b6e7e --- /dev/null +++ b/api/services/account_oauth_service.py @@ -0,0 +1,283 @@ +"""Application service for Console account OAuth sign-in.""" + +from collections.abc import Callable, Mapping, Sequence +from contextlib import AbstractContextManager +from datetime import datetime +from typing import Protocol + +from services.account_email import normalize_email +from services.account_errors import ( + AccountEmailDomainSuspendedError, + AccountEmailFrozenError, + InvalidOAuthInvitationError, + InvalidOAuthProviderError, + OAuthAccountBannedError, + OAuthAccountNotFoundError, + OAuthInvitationAccountMismatchError, + OAuthRegistrationError, + OAuthWorkspaceCreationNotAllowedError, +) +from services.account_ports import AccountIntegrationRepository, AccountRepository, AccountWorkspaceMembershipQuery +from services.entities.account_entities import AccountSessionTokens, AccountSnapshot +from services.entities.account_oauth_entities import ( + OAuthAccountRegistration, + OAuthAuthorizationRequest, + OAuthCallbackCommand, + OAuthCallbackResult, + OAuthIdentity, + OAuthInvitation, + OAuthInvitationResult, + OAuthSignInResult, +) + +_BANNED_ACCOUNT_STATUS = "banned" +_PENDING_ACCOUNT_STATUS = "pending" + + +class OAuthProviderGateway(Protocol): + def get_authorization_url(self, request: OAuthAuthorizationRequest) -> str: ... + + def get_identity(self, code: str) -> OAuthIdentity: ... + + +class OAuthInvitationGateway(Protocol): + def resolve(self, invite_token: str) -> OAuthInvitation | None: ... + + +class OAuthAccountClaimLease(Protocol): + def ensure_owned(self) -> None: ... + + +class OAuthAccountClaimLock(Protocol): + def acquire(self, *, provider: str, open_id: str, email: str) -> AbstractContextManager[OAuthAccountClaimLease]: ... + + def acquire_account(self, account_id: str) -> AbstractContextManager[OAuthAccountClaimLease]: ... + + +class OAuthAccountRegistrationGateway(Protocol): + def register(self, registration: OAuthAccountRegistration) -> str: ... + + +class OAuthWorkspaceGateway(Protocol): + def create_owner_workspace(self, account_id: str) -> None: ... + + def try_join_default_workspace(self, account_id: str) -> None: ... + + +class OAuthSessionGateway(Protocol): + def login(self, account_id: str, *, ip_address: str) -> AccountSessionTokens: ... + + +class OAuthRegistrationPolicyGateway(Protocol): + def is_registration_allowed(self) -> bool: ... + + def get_freeze_type(self, email: str) -> str | None: ... + + +class OAuthWorkspacePolicyGateway(Protocol): + def is_creation_allowed(self) -> bool: ... + + +class AccountOAuthService: + def __init__( + self, + *, + providers: Mapping[str, OAuthProviderGateway], + accounts: AccountRepository, + integrations: AccountIntegrationRepository, + memberships: AccountWorkspaceMembershipQuery, + invitations: OAuthInvitationGateway, + account_claims: OAuthAccountClaimLock, + registration: OAuthAccountRegistrationGateway, + workspaces: OAuthWorkspaceGateway, + sessions: OAuthSessionGateway, + registration_policy: OAuthRegistrationPolicyGateway, + workspace_policy: OAuthWorkspacePolicyGateway, + supported_languages: Sequence[str], + now: Callable[[], datetime], + ) -> None: + self._providers = dict(providers) + self._accounts = accounts + self._integrations = integrations + self._memberships = memberships + self._invitations = invitations + self._account_claims = account_claims + self._registration = registration + self._workspaces = workspaces + self._sessions = sessions + self._registration_policy = registration_policy + self._workspace_policy = workspace_policy + self._supported_languages = tuple(supported_languages) + self._now = now + + def start_authorization(self, provider: str, request: OAuthAuthorizationRequest) -> str: + return self._provider(provider).get_authorization_url(request) + + def complete_authorization(self, command: OAuthCallbackCommand) -> OAuthCallbackResult: + provider = self._provider(command.provider) + identity = provider.get_identity(command.code) + identity_email_key = self._identity_email_key(identity.email) + + with self._account_claims.acquire( + provider=command.provider, + open_id=identity.id, + email=identity_email_key, + ) as identity_claim: + return self._complete_claimed_authorization(command, identity, identity_claim) + + def _complete_claimed_authorization( + self, + command: OAuthCallbackCommand, + identity: OAuthIdentity, + identity_claim: OAuthAccountClaimLease, + ) -> OAuthCallbackResult: + if command.invite_token is not None: + return self._complete_invitation(command, identity, identity_claim) + + account = self._resolve_account(command.provider, identity) + oauth_new_user = account is None + if account is None: + identity_claim.ensure_owned() + account = self._register_account(command, identity) + identity_claim.ensure_owned() + + self._ensure_account_can_login(account) + identity_claim.ensure_owned() + self._integrations.link(account.id, provider=command.provider, open_id=identity.id) + identity_claim.ensure_owned() + with self._account_claims.acquire_account(account.id) as account_claim: + if oauth_new_user: + self._provision_new_account_workspaces(account.id, account_claim) + else: + self._provision_owner_workspace_if_required(account.id, account_claim) + if account.status == _PENDING_ACCOUNT_STATUS: + account_claim.ensure_owned() + self._accounts.activate_pending(account.id, initialized_at=self._now()) + account_claim.ensure_owned() + + identity_claim.ensure_owned() + tokens = self._sessions.login(account.id, ip_address=command.ip_address) + return OAuthSignInResult(tokens=tokens, oauth_new_user=oauth_new_user) + + def _provision_new_account_workspaces( + self, + account_id: str, + account_claim: OAuthAccountClaimLease, + ) -> None: + if self._memberships.has_active_membership(account_id): + account_claim.ensure_owned() + self._workspaces.try_join_default_workspace(account_id) + account_claim.ensure_owned() + return + + creation_error = OAuthWorkspaceCreationNotAllowedError() + account_claim.ensure_owned() + if self._workspace_policy.is_creation_allowed(): + account_claim.ensure_owned() + try: + self._workspaces.create_owner_workspace(account_id) + except OAuthWorkspaceCreationNotAllowedError as exc: + creation_error = exc + else: + account_claim.ensure_owned() + self._workspaces.try_join_default_workspace(account_id) + account_claim.ensure_owned() + return + + account_claim.ensure_owned() + self._workspaces.try_join_default_workspace(account_id) + account_claim.ensure_owned() + if self._memberships.has_active_membership(account_id): + return + raise creation_error + + def _provision_owner_workspace_if_required( + self, + account_id: str, + account_claim: OAuthAccountClaimLease, + ) -> None: + if self._memberships.has_active_membership(account_id): + return + account_claim.ensure_owned() + if not self._workspace_policy.is_creation_allowed(): + raise OAuthWorkspaceCreationNotAllowedError + account_claim.ensure_owned() + self._workspaces.create_owner_workspace(account_id) + account_claim.ensure_owned() + + def _complete_invitation( + self, + command: OAuthCallbackCommand, + identity: OAuthIdentity, + identity_claim: OAuthAccountClaimLease, + ) -> OAuthCallbackResult: + invite_token = command.invite_token + if invite_token is None: + raise AssertionError("invitation completion requires a token") + invitation = self._invitations.resolve(invite_token) + if invitation is None: + raise InvalidOAuthInvitationError + if self._normalize_email(invitation.account_email) != self._normalize_email(identity.email): + raise OAuthInvitationAccountMismatchError(invite_token) + if invitation.account_status == _BANNED_ACCOUNT_STATUS: + raise OAuthAccountBannedError + + identity_claim.ensure_owned() + self._integrations.link(invitation.account_id, provider=command.provider, open_id=identity.id) + identity_claim.ensure_owned() + tokens = self._sessions.login(invitation.account_id, ip_address=command.ip_address) + return OAuthInvitationResult(tokens=tokens, invite_token=invite_token) + + def _register_account(self, command: OAuthCallbackCommand, identity: OAuthIdentity) -> AccountSnapshot: + normalized_email = self._normalize_email(identity.email) + if not self._registration_policy.is_registration_allowed(): + freeze_type = self._registration_policy.get_freeze_type(normalized_email) + if freeze_type == "email_domain_suspended": + raise AccountEmailDomainSuspendedError + if freeze_type: + raise AccountEmailFrozenError + raise OAuthRegistrationError("Invalid email or password") + + language = command.language or command.browser_language + if language not in self._supported_languages: + language = self._supported_languages[0] + account_id = self._registration.register( + OAuthAccountRegistration( + email=normalized_email, + name=identity.name or "Dify", + language=language, + timezone=command.timezone, + ip_address=command.ip_address, + ) + ) + account = self._accounts.get(account_id) + if account is None: + raise OAuthAccountNotFoundError + return account + + def _resolve_account(self, provider: str, identity: OAuthIdentity) -> AccountSnapshot | None: + account_id = self._integrations.find_account_id(provider=provider, open_id=identity.id) + if account_id is not None: + account = self._accounts.get(account_id) + if account is not None: + return account + return self._accounts.find_by_email(identity.email) + + @staticmethod + def _normalize_email(email: str) -> str: + return email.strip().lower() + + @staticmethod + def _identity_email_key(email: str) -> str: + return normalize_email(email.strip()) + + @staticmethod + def _ensure_account_can_login(account: AccountSnapshot) -> None: + if account.status == _BANNED_ACCOUNT_STATUS: + raise OAuthAccountBannedError + + def _provider(self, provider: str) -> OAuthProviderGateway: + gateway = self._providers.get(provider) + if gateway is None: + raise InvalidOAuthProviderError + return gateway diff --git a/api/services/account_ports.py b/api/services/account_ports.py index 78792664127..8ea4f06e074 100644 --- a/api/services/account_ports.py +++ b/api/services/account_ports.py @@ -1,6 +1,7 @@ """Persistence ports used by account application services.""" from collections.abc import Sequence +from datetime import datetime from typing import Protocol from services.entities.account_entities import ( @@ -21,6 +22,8 @@ class AccountRepository(Protocol): def find_by_email(self, email: str) -> AccountSnapshot | None: ... + def activate_pending(self, account_id: str, *, initialized_at: datetime) -> None: ... + def get_credentials(self, account_id: str) -> AccountCredentials | None: ... def update_profile(self, account_id: str, changes: AccountProfileChanges) -> AccountSnapshot | None: ... @@ -42,12 +45,18 @@ class AccountRepository(Protocol): class AccountIntegrationRepository(Protocol): + def find_account_id(self, *, provider: str, open_id: str) -> str | None: ... + def list_for_account(self, account_id: str) -> list[AccountIntegrationSnapshot]: ... + def link(self, account_id: str, *, provider: str, open_id: str) -> None: ... + class AccountWorkspaceMembershipQuery(Protocol): def list_ids_for_account(self, account_id: str) -> Sequence[str]: ... + def has_active_membership(self, account_id: str) -> bool: ... + class AccountAvatarFileGateway(Protocol): def get_owned_signed_url(self, *, account_id: str, upload_file_id: str) -> str | None: ... diff --git a/api/services/account_service.py b/api/services/account_service.py index e9b91f6af0d..dae4ea2b4a6 100644 --- a/api/services/account_service.py +++ b/api/services/account_service.py @@ -72,7 +72,6 @@ from services.errors.account import ( CannotOperateSelfError, EmailDomainSuspendedError, InvalidActionError, - LinkAccountIntegrateError, MemberNotInTenantError, NoPermissionError, RefreshTokenNotFoundError, @@ -514,35 +513,6 @@ class AccountService: return account - @staticmethod - def link_account_integrate(provider: str, open_id: str, account: Account, *, session: Session): - """Link account integrate""" - try: - # Query whether there is an existing binding record for the same provider - account_integrate: AccountIntegrate | None = session.scalar( - select(AccountIntegrate) - .where(AccountIntegrate.account_id == account.id, AccountIntegrate.provider == provider) - .limit(1) - ) - - if account_integrate: - # If it exists, update the record - account_integrate.open_id = open_id - account_integrate.encrypted_token = "" # todo - account_integrate.updated_at = naive_utc_now() - else: - # If it does not exist, create a new record - account_integrate = AccountIntegrate( - account_id=account.id, provider=provider, open_id=open_id, encrypted_token="" - ) - session.add(account_integrate) - - session.commit() - logger.info("Account %s linked %s account %s.", account.id, provider, open_id) - except Exception as e: - logger.exception("Failed to link %s account %s to Account %s", provider, open_id, account.id) - raise LinkAccountIntegrateError("Failed to link account.") from e - @staticmethod def update_account_email(account: Account, email: str, session: Session) -> Account: """Update account email""" @@ -1814,8 +1784,6 @@ class RegisterService: email: str, name: str, password: str | None = None, - open_id: str | None = None, - provider: str | None = None, language: str | None = None, status: AccountStatus | None = None, is_setup: bool | None = False, @@ -1844,9 +1812,6 @@ class RegisterService: account.status = status or AccountStatus.ACTIVE account.initialized_at = naive_utc_now() - if open_id is not None and provider is not None: - AccountService.link_account_integrate(provider, open_id, account, session=session) - if ( SystemFeatureService.is_workspace_creation_allowed() and create_workspace_required @@ -1999,11 +1964,6 @@ class RegisterService: redis_client.setex(cls._get_invitation_token_key(token), expiry_hours * 60 * 60, json.dumps(invitation_data)) return token - @classmethod - def is_valid_invite_token(cls, token: str) -> bool: - data = redis_client.get(cls._get_invitation_token_key(token)) - return data is not None - @classmethod def revoke_token(cls, workspace_id: str | None, email: str | None, token: str): if workspace_id and email: diff --git a/api/services/entities/account_oauth_entities.py b/api/services/entities/account_oauth_entities.py new file mode 100644 index 00000000000..bfe66b99e63 --- /dev/null +++ b/api/services/entities/account_oauth_entities.py @@ -0,0 +1,62 @@ +"""Framework-neutral contracts for Console account OAuth sign-in.""" + +from dataclasses import dataclass + +from services.entities.account_entities import AccountSessionTokens as _AccountSessionTokens + + +@dataclass(frozen=True, slots=True) +class OAuthAuthorizationRequest: + invite_token: str | None = None + timezone: str | None = None + language: str | None = None + redirect_url: str | None = None + + +@dataclass(frozen=True, slots=True) +class OAuthIdentity: + id: str + name: str + email: str + + +@dataclass(frozen=True, slots=True) +class OAuthCallbackCommand: + provider: str + code: str + invite_token: str | None + timezone: str | None + language: str | None + browser_language: str | None + ip_address: str + + +@dataclass(frozen=True, slots=True) +class OAuthInvitation: + account_id: str + account_email: str + account_status: str + + +@dataclass(frozen=True, slots=True) +class OAuthAccountRegistration: + email: str + name: str + language: str + timezone: str | None + ip_address: str + + +@dataclass(frozen=True, slots=True) +class OAuthSignInResult: + tokens: _AccountSessionTokens + oauth_new_user: bool + + +@dataclass(frozen=True, slots=True) +class OAuthInvitationResult: + tokens: _AccountSessionTokens + invite_token: str + + +type OAuthCallbackResult = OAuthSignInResult | OAuthInvitationResult diff --git a/api/services/errors/account.py b/api/services/errors/account.py index fc1d6772174..aa9084efb30 100644 --- a/api/services/errors/account.py +++ b/api/services/errors/account.py @@ -38,10 +38,6 @@ class AccountNotLinkTenantError(BaseServiceError): pass -class LinkAccountIntegrateError(BaseServiceError): - pass - - class TenantNotFoundError(BaseServiceError): pass diff --git a/api/services/errors/workspace.py b/api/services/errors/workspace.py index 577238507f8..18ff8b6eccd 100644 --- a/api/services/errors/workspace.py +++ b/api/services/errors/workspace.py @@ -5,9 +5,5 @@ class WorkSpaceNotAllowedCreateError(BaseServiceError): pass -class WorkSpaceNotFoundError(BaseServiceError): - pass - - class WorkspacesLimitExceededError(BaseServiceError): pass diff --git a/api/tests/test_containers_integration_tests/models/test_account.py b/api/tests/test_containers_integration_tests/models/test_account.py index 1f1c4a4ede1..676bf013bcd 100644 --- a/api/tests/test_containers_integration_tests/models/test_account.py +++ b/api/tests/test_containers_integration_tests/models/test_account.py @@ -8,7 +8,6 @@ Also absorbs unit_tests/models/test_account.py role helper coverage. Covers: - Account.current_tenant setter (sets _current_tenant and role from TenantAccountJoin) - Account.set_tenant_id (resolves tenant + role from real join row) -- Account.get_by_openid (AccountIntegrate lookup then Account fetch) - Tenant.get_accounts (returns accounts linked via TenantAccountJoin) """ @@ -20,9 +19,9 @@ import pytest from sqlalchemy import delete from sqlalchemy.orm import Session -from models.account import Account, AccountIntegrate, Tenant, TenantAccountJoin, TenantAccountRole +from models.account import Account, Tenant, TenantAccountJoin, TenantAccountRole -TrackedRow = Account | AccountIntegrate | Tenant | TenantAccountJoin +TrackedRow = Account | Tenant | TenantAccountJoin def _cleanup_tracked_rows(db_session: Session, tracked: list[TrackedRow]) -> None: @@ -172,37 +171,6 @@ class TestAccountSetTenantId(_DBTrackingTestBase): assert account._current_tenant is None -class TestAccountGetByOpenId(_DBTrackingTestBase): - """Integration tests for Account.get_by_openid class method.""" - - def test_get_by_openid_returns_account_when_integrate_exists(self, db_session_with_containers: Session) -> None: - """get_by_openid returns the Account when a matching AccountIntegrate row exists.""" - account = self._create_account(db_session_with_containers, email_prefix="openid") - provider = "google" - open_id = f"google_{uuid4()}" - - integrate = AccountIntegrate( - account_id=account.id, - provider=provider, - open_id=open_id, - encrypted_token="token", - ) - db_session_with_containers.add(integrate) - db_session_with_containers.flush() - self._tracked.append(integrate) - - result = Account.get_by_openid(provider, open_id) - - assert result is not None - assert result.id == account.id - - def test_get_by_openid_returns_none_when_no_integrate_exists(self) -> None: - """get_by_openid returns None when no AccountIntegrate row matches.""" - result = Account.get_by_openid("github", f"github_{uuid4()}") - - assert result is None - - class TestTenantGetAccounts(_DBTrackingTestBase): """Integration tests for Tenant.get_accounts method.""" diff --git a/api/tests/test_containers_integration_tests/services/test_account_oauth_identity_lock.py b/api/tests/test_containers_integration_tests/services/test_account_oauth_identity_lock.py new file mode 100644 index 00000000000..e0c40d40576 --- /dev/null +++ b/api/tests/test_containers_integration_tests/services/test_account_oauth_identity_lock.py @@ -0,0 +1,64 @@ +"""Redis-backed integration coverage for Console OAuth account-claim leases.""" + +import time +from hashlib import sha256 +from uuid import uuid4 + +import pytest + +from extensions.ext_redis import redis_client +from services import account_oauth_adapters +from services.account_errors import OAuthIdentityLockUnavailableError +from services.account_oauth_adapters import RedisOAuthAccountClaimLock + + +@pytest.mark.usefixtures("flask_app_with_containers") +def test_account_claim_locks_remain_exclusive_beyond_their_initial_ttl(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(account_oauth_adapters, "_OAUTH_ACCOUNT_CLAIM_LOCK_TIMEOUT_SECONDS", 0.5) + monkeypatch.setattr(account_oauth_adapters, "_OAUTH_ACCOUNT_CLAIM_LOCK_RENEW_INTERVAL_SECONDS", 0.1) + monkeypatch.setattr(account_oauth_adapters, "_OAUTH_ACCOUNT_CLAIM_LOCK_BLOCKING_TIMEOUT_SECONDS", 0.2) + provider = "github" + open_id = f"identity-{uuid4().hex}" + email = f"account-{uuid4().hex}@example.com" + identity_digest = sha256("\0".join(("identity", provider, open_id)).encode()).hexdigest() + email_digest = sha256("\0".join(("email", email)).encode()).hexdigest() + lock_names = [ + f"oauth:account-claim:{identity_digest}", + f"oauth:account-claim:{email_digest}", + ] + account_claims = RedisOAuthAccountClaimLock(client=redis_client) + contenders = [ + redis_client.lock(lock_name, timeout=1, blocking=False, thread_local=False) for lock_name in lock_names + ] + + with account_claims.acquire(provider=provider, open_id=open_id, email=email): + time.sleep(1.2) + assert all(contender.acquire(blocking=False) is False for contender in contenders) + + for contender in contenders: + assert contender.acquire(blocking=False) is True + contender.release() + + +@pytest.mark.usefixtures("flask_app_with_containers") +def test_different_providers_with_the_same_email_contend_for_one_claim(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(account_oauth_adapters, "_OAUTH_ACCOUNT_CLAIM_LOCK_BLOCKING_TIMEOUT_SECONDS", 0.1) + email = f"account-{uuid4().hex}@example.com" + account_claims = RedisOAuthAccountClaimLock(client=redis_client) + + with account_claims.acquire(provider="github", open_id=f"github-{uuid4().hex}", email=email): + with pytest.raises(OAuthIdentityLockUnavailableError): + with account_claims.acquire(provider="google", open_id=f"google-{uuid4().hex}", email=email): + raise AssertionError("same-email claim body must not run concurrently") + + +@pytest.mark.usefixtures("flask_app_with_containers") +def test_final_account_claim_serializes_different_provider_identities(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(account_oauth_adapters, "_OAUTH_ACCOUNT_CLAIM_LOCK_BLOCKING_TIMEOUT_SECONDS", 0.1) + account_claims = RedisOAuthAccountClaimLock(client=redis_client) + account_id = f"account-{uuid4().hex}" + + with account_claims.acquire_account(account_id): + with pytest.raises(OAuthIdentityLockUnavailableError): + with account_claims.acquire_account(account_id): + raise AssertionError("same-account claim body must not run concurrently") diff --git a/api/tests/test_containers_integration_tests/services/test_account_service.py b/api/tests/test_containers_integration_tests/services/test_account_service.py index 30292d43826..f7953f8dbf9 100644 --- a/api/tests/test_containers_integration_tests/services/test_account_service.py +++ b/api/tests/test_containers_integration_tests/services/test_account_service.py @@ -280,86 +280,6 @@ class TestAccountService: session=db_session_with_containers, ) - def test_link_account_integrate_new_provider( - self, db_session_with_containers: Session, mock_external_service_dependencies - ): - """ - Test linking account with new OAuth provider. - """ - fake = Faker() - email = fake.email() - name = fake.name() - # Setup mocks - mock_external_service_dependencies["feature_service"].is_registration_allowed.return_value = True - mock_external_service_dependencies["billing_service"].is_email_in_freeze.return_value = False - - # Create account - account = AccountService.create_account( - email=email, - name=name, - interface_language="en-US", - password=None, - session=db_session_with_containers, - ) - - # Link with new provider - AccountService.link_account_integrate( - "new-google", "google_open_id_123", account, session=db_session_with_containers - ) - - # Verify integration was created - from models import AccountIntegrate - - integration = ( - db_session_with_containers.query(AccountIntegrate) - .filter_by(account_id=account.id, provider="new-google") - .first() - ) - assert integration is not None - assert integration.open_id == "google_open_id_123" - - def test_link_account_integrate_existing_provider( - self, db_session_with_containers: Session, mock_external_service_dependencies - ): - """ - Test linking account with existing provider (should update). - """ - fake = Faker() - email = fake.email() - name = fake.name() - # Setup mocks - mock_external_service_dependencies["feature_service"].is_registration_allowed.return_value = True - mock_external_service_dependencies["billing_service"].is_email_in_freeze.return_value = False - - # Create account - account = AccountService.create_account( - email=email, - name=name, - interface_language="en-US", - password=None, - session=db_session_with_containers, - ) - - # Link with provider first time - AccountService.link_account_integrate( - "exists-google", "google_open_id_123", account, session=db_session_with_containers - ) - - # Link with same provider but different open_id (should update) - AccountService.link_account_integrate( - "exists-google", "google_open_id_456", account, session=db_session_with_containers - ) - - # Verify integration was updated - from models import AccountIntegrate - - integration = ( - db_session_with_containers.query(AccountIntegrate) - .filter_by(account_id=account.id, provider="exists-google") - .first() - ) - assert integration.open_id == "google_open_id_456" - def test_update_login_info(self, db_session_with_containers: Session, mock_external_service_dependencies): """ Test updating login information. @@ -1967,52 +1887,6 @@ class TestRegisterService: assert account.current_tenant is not None assert account.current_tenant.name == f"{name}'s Workspace" - def test_register_with_oauth(self, db_session_with_containers: Session, mock_external_service_dependencies): - """ - Test account registration with OAuth integration. - """ - fake = Faker() - email = fake.email() - name = fake.name() - open_id = fake.uuid4() - provider = fake.random_element(elements=("google", "github", "microsoft")) - language = fake.random_element(elements=("en-US", "zh-CN")) - # Setup mocks - mock_external_service_dependencies["feature_service"].is_registration_allowed.return_value = True - mock_external_service_dependencies["feature_service"].is_workspace_creation_allowed.return_value = True - mock_external_service_dependencies[ - "feature_service" - ].get_license.return_value.workspaces.is_available.return_value = True - mock_external_service_dependencies["billing_service"].is_email_in_freeze.return_value = False - - # Execute registration with OAuth - account = RegisterService.register( - email=email, - name=name, - password=None, - open_id=open_id, - provider=provider, - language=language, - session=db_session_with_containers, - ) - - # Verify account was created - assert account.email == email - assert account.name == name - assert account.status == "active" - assert account.initialized_at is not None - - # Verify OAuth integration was created - from models import AccountIntegrate - - integration = ( - db_session_with_containers.query(AccountIntegrate) - .filter_by(account_id=account.id, provider=provider) - .first() - ) - assert integration is not None - assert integration.open_id == open_id - def test_register_with_pending_status( self, db_session_with_containers: Session, mock_external_service_dependencies ): @@ -2510,52 +2384,6 @@ class TestRegisterService: assert invitation_data["email"] == account.email assert invitation_data["workspace_id"] == tenant.id - def test_is_valid_invite_token_valid(self, db_session_with_containers: Session, mock_external_service_dependencies): - """ - Test validation of valid invite token. - """ - fake = Faker() - tenant_name = fake.company() - email = fake.email() - name = fake.name() - password = generate_valid_password(fake) - # Setup mocks - mock_external_service_dependencies["feature_service"].is_registration_allowed.return_value = True - mock_external_service_dependencies["billing_service"].is_email_in_freeze.return_value = False - - # Create tenant and account - tenant = TenantService.create_tenant(name=tenant_name, session=db_session_with_containers) - account = AccountService.create_account( - email=email, - name=name, - interface_language="en-US", - password=password, - session=db_session_with_containers, - ) - - # Generate a real token - token = RegisterService.generate_invite_token(tenant, account) - - # Execute validation - is_valid = RegisterService.is_valid_invite_token(token) - - # Verify token is valid - assert is_valid is True - - def test_is_valid_invite_token_invalid( - self, db_session_with_containers: Session, mock_external_service_dependencies - ): - """ - Test validation of invalid invite token. - """ - fake = Faker() - invalid_token = fake.uuid4() - # Execute validation with non-existent token - is_valid = RegisterService.is_valid_invite_token(invalid_token) - - # Verify token is invalid - assert is_valid is False - def test_revoke_token_with_workspace_and_email( self, db_session_with_containers: Session, mock_external_service_dependencies ): diff --git a/api/tests/unit_tests/controllers/console/auth/test_oauth.py b/api/tests/unit_tests/controllers/console/auth/test_oauth.py index 3a0d1b24310..38472867522 100644 --- a/api/tests/unit_tests/controllers/console/auth/test_oauth.py +++ b/api/tests/unit_tests/controllers/console/auth/test_oauth.py @@ -1,735 +1,303 @@ -"""Unit tests for OAuth controller endpoints.""" - -from __future__ import annotations - -from unittest.mock import ANY, MagicMock, patch +from collections.abc import Callable +from dataclasses import dataclass, field +from types import SimpleNamespace +from typing import NoReturn import pytest from flask import Flask -from sqlalchemy.orm import Session, scoped_session, sessionmaker +from werkzeug.exceptions import Forbidden, UnprocessableEntity -from controllers.console.auth.oauth import ( - OAuthCallback, - OAuthLogin, - _generate_account, - _get_account_by_openid_or_email, - get_oauth_providers, -) +from controllers.console import wraps as console_wraps +from controllers.console.auth import oauth as oauth_controller +from controllers.console.auth.oauth import OAuthCallback, OAuthLogin from enums import DeploymentEdition -from libs.oauth import OAuthUserInfo, encode_oauth_state -from models.account import Account, AccountIntegrate, AccountStatus, Tenant -from services.errors.account import AccountRegisterError -from services.errors.account import ( - EmailDomainSuspendedError as EmailDomainSuspendedRegistrationError, +from libs.oauth import encode_oauth_state +from services.account_errors import ( + InvalidOAuthProviderError, + OAuthIdentityLockUnavailableError, + OAuthInvitationAccountMismatchError, + OAuthProviderRequestError, + OAuthRegistrationError, ) -from tests.unit_tests.config_override import config_overrides_context +from services.entities.account_entities import AccountSessionTokens +from services.entities.account_oauth_entities import ( + OAuthAuthorizationRequest, + OAuthCallbackCommand, + OAuthCallbackResult, + OAuthInvitationResult, + OAuthSignInResult, +) +from services.system_feature_service import SystemFeatureService + +CONSOLE_WEB_URL = "https://console.example.com" + + +@dataclass +class FakeOAuthService: + authorization_url: str = "https://provider.example/authorize" + callback_result: OAuthCallbackResult = OAuthSignInResult( + tokens=AccountSessionTokens("access-token", "refresh-token", "csrf-token"), + oauth_new_user=False, + ) + authorization_error: Exception | None = None + callback_error: Exception | None = None + authorization_calls: list[tuple[str, OAuthAuthorizationRequest]] = field(default_factory=list) + callback_calls: list[OAuthCallbackCommand] = field(default_factory=list) + + def start_authorization(self, provider: str, request: OAuthAuthorizationRequest) -> str: + self.authorization_calls.append((provider, request)) + if self.authorization_error is not None: + raise self.authorization_error + return self.authorization_url + + def complete_authorization(self, command: OAuthCallbackCommand) -> OAuthCallbackResult: + self.callback_calls.append(command) + if self.callback_error is not None: + raise self.callback_error + return self.callback_result @pytest.fixture(autouse=True) -def _oauth_config(config_overrides) -> None: - config_overrides(CONSOLE_WEB_URL="http://localhost:3000") - - -class TestGetOAuthProviders: - @pytest.mark.parametrize( - ("github_config", "google_config", "expected_github", "expected_google"), - [ - # Both providers configured - ( - {"id": "github_id", "secret": "github_secret"}, - {"id": "google_id", "secret": "google_secret"}, - True, - True, - ), - # Only GitHub configured - ({"id": "github_id", "secret": "github_secret"}, {"id": None, "secret": None}, True, False), - # Only Google configured - ({"id": None, "secret": None}, {"id": "google_id", "secret": "google_secret"}, False, True), - # No providers configured - ({"id": None, "secret": None}, {"id": None, "secret": None}, False, False), - ], +def _oauth_admission( + config_overrides: Callable[..., None], +) -> None: + config_overrides( + DEPLOYMENT_EDITION=DeploymentEdition.CLOUD, + ENABLE_SOCIAL_OAUTH_LOGIN=True, + CONSOLE_WEB_URL=CONSOLE_WEB_URL, ) - def test_should_configure_oauth_providers_correctly( - self, app: Flask, github_config, google_config, expected_github, expected_google, config_overrides + + +def _install_service(monkeypatch: pytest.MonkeyPatch, service: FakeOAuthService) -> None: + services = SimpleNamespace(accounts=SimpleNamespace(oauth=service)) + monkeypatch.setattr(oauth_controller, "application_services", lambda: services) + + +def test_login_parses_input_and_delegates_to_application_service( + app: Flask, + monkeypatch: pytest.MonkeyPatch, +) -> None: + service = FakeOAuthService() + _install_service(monkeypatch, service) + + with app.test_request_context( + "/oauth/login/github?invite_token=invite&timezone=Asia%2FShanghai&language=zh-Hans&redirect_url=%2Fapps" ): - config_overrides( - GITHUB_CLIENT_ID=github_config["id"], - GITHUB_CLIENT_SECRET=github_config["secret"], - GOOGLE_CLIENT_ID=google_config["id"], - GOOGLE_CLIENT_SECRET=google_config["secret"], - CONSOLE_API_URL="http://localhost", - ) + response = OAuthLogin().get("github") - with app.app_context(): - providers = get_oauth_providers() - - assert (providers["github"] is not None) == expected_github - assert (providers["google"] is not None) == expected_google - - -class TestOAuthLogin: - @pytest.fixture - def resource(self): - return OAuthLogin() - - @pytest.fixture - def mock_oauth_provider(self): - provider = MagicMock() - provider.get_authorization_url.return_value = "https://github.com/login/oauth/authorize?..." - return provider - - @pytest.mark.parametrize( - ("invite_token", "expected_token"), - [ - (None, None), - ("test_invite_token", "test_invite_token"), - ("", None), - ], - ) - @patch("controllers.console.auth.oauth.get_oauth_providers") - @patch("controllers.console.auth.oauth.redirect") - def test_should_handle_oauth_login_with_various_tokens( - self, - mock_redirect, - mock_get_providers, - resource: OAuthLogin, - app: Flask, - mock_oauth_provider, - invite_token, - expected_token, - ): - mock_get_providers.return_value = {"github": mock_oauth_provider, "google": None} - - query_string = f"invite_token={invite_token}" if invite_token else "" - with app.test_request_context(f"/auth/oauth/github?{query_string}"): - resource.get("github") - - mock_oauth_provider.get_authorization_url.assert_called_once_with( - invite_token=expected_token, - timezone=None, - language=None, - redirect_url=None, - ) - mock_redirect.assert_called_once_with("https://github.com/login/oauth/authorize?...") - - @patch("controllers.console.auth.oauth.get_oauth_providers") - @patch("controllers.console.auth.oauth.redirect") - def test_should_pass_timezone_to_oauth_state( - self, - mock_redirect, - mock_get_providers, - resource: OAuthLogin, - app: Flask, - mock_oauth_provider, - ): - mock_get_providers.return_value = {"github": mock_oauth_provider, "google": None} - - with app.test_request_context("/auth/oauth/github?timezone=Asia/Shanghai"): - resource.get("github") - - mock_oauth_provider.get_authorization_url.assert_called_once_with( - invite_token=None, - timezone="Asia/Shanghai", - language=None, - redirect_url=None, - ) - mock_redirect.assert_called_once_with("https://github.com/login/oauth/authorize?...") - - @patch("controllers.console.auth.oauth.get_oauth_providers") - @patch("controllers.console.auth.oauth.redirect") - def test_should_pass_language_to_oauth_state( - self, - mock_redirect, - mock_get_providers, - resource: OAuthLogin, - app: Flask, - mock_oauth_provider, - ): - mock_get_providers.return_value = {"github": mock_oauth_provider, "google": None} - - with app.test_request_context("/auth/oauth/github?language=zh-Hans"): - resource.get("github") - - mock_oauth_provider.get_authorization_url.assert_called_once_with( - invite_token=None, - timezone=None, - language="zh-Hans", - redirect_url=None, - ) - mock_redirect.assert_called_once_with("https://github.com/login/oauth/authorize?...") - - @pytest.mark.parametrize( - ("provider", "expected_error"), - [ - ("invalid_provider", "Invalid provider"), - ("github", "Invalid provider"), # When GitHub is not configured - ("google", "Invalid provider"), # When Google is not configured - ], - ) - @patch("controllers.console.auth.oauth.get_oauth_providers") - def test_should_return_error_for_invalid_providers( - self, mock_get_providers, resource, app, provider, expected_error - ): - mock_get_providers.return_value = {"github": None, "google": None} - - with app.test_request_context(f"/auth/oauth/{provider}"): - response, status_code = resource.get(provider) - - assert status_code == 400 - assert response["error"] == expected_error - - -class TestOAuthCallback: - @pytest.fixture - def resource(self): - return OAuthCallback() - - @pytest.fixture - def oauth_setup(self): - """Common OAuth setup for callback tests""" - oauth_provider = MagicMock() - oauth_provider.get_access_token.return_value = "access_token" - oauth_provider.get_user_info.return_value = OAuthUserInfo(id="123", name="Test User", email="test@example.com") - - account = Account(name="Test User", email="test@example.com", status=AccountStatus.ACTIVE) - account.id = "123" - - token_pair = MagicMock() - token_pair.access_token = "jwt_access_token" - token_pair.refresh_token = "jwt_refresh_token" - - return {"provider": oauth_provider, "account": account, "token_pair": token_pair} - - @patch("controllers.console.auth.oauth.get_oauth_providers") - @patch("controllers.console.auth.oauth._generate_account") - @patch("controllers.console.auth.oauth.AccountService") - @patch("controllers.console.auth.oauth.TenantService") - @patch("controllers.console.auth.oauth.redirect") - def test_should_handle_successful_oauth_callback( - self, - mock_redirect, - mock_tenant_service, - mock_account_service, - mock_generate_account, - mock_get_providers, - resource: OAuthCallback, - app: Flask, - oauth_setup, - ): - mock_get_providers.return_value = {"github": oauth_setup["provider"]} - mock_generate_account.return_value = (oauth_setup["account"], True) - mock_account_service.login.return_value = oauth_setup["token_pair"] - - with ( - patch("controllers.console.auth.oauth.extract_remote_ip", return_value="203.0.113.10"), - app.test_request_context("/auth/oauth/github/callback?code=test_code"), - ): - resource.get("github") - - oauth_setup["provider"].get_access_token.assert_called_once_with("test_code") - oauth_setup["provider"].get_user_info.assert_called_once_with("access_token") - mock_generate_account.assert_called_once_with( + assert response.status_code == 302 + assert response.headers["Location"] == "https://provider.example/authorize" + assert service.authorization_calls == [ + ( "github", - oauth_setup["provider"].get_user_info.return_value, - timezone=None, - language=None, + OAuthAuthorizationRequest( + invite_token="invite", + timezone="Asia/Shanghai", + language="zh-Hans", + redirect_url="/apps", + ), + ) + ] + + +def test_login_returns_adapter_error_for_unknown_provider( + app: Flask, + monkeypatch: pytest.MonkeyPatch, +) -> None: + service = FakeOAuthService(authorization_error=InvalidOAuthProviderError()) + _install_service(monkeypatch, service) + + with app.test_request_context("/oauth/login/unknown"): + payload, status = OAuthLogin().get("unknown") + + assert status == 400 + assert payload == {"error": "Invalid provider"} + + +def test_oauth_admission_does_not_query_enterprise_features( + app: Flask, + monkeypatch: pytest.MonkeyPatch, + config_overrides: Callable[..., None], +) -> None: + config_overrides(DEPLOYMENT_EDITION=DeploymentEdition.ENTERPRISE) + monkeypatch.setattr(console_wraps, "_is_setup_completed", lambda: True) + service = FakeOAuthService() + _install_service(monkeypatch, service) + + def unexpected_feature_query() -> NoReturn: + raise AssertionError("OAuth admission must not query Enterprise features") + + monkeypatch.setattr(SystemFeatureService, "get_license", unexpected_feature_query) + + with app.test_request_context("/oauth/login/github"): + response = OAuthLogin().get("github") + + assert response.status_code == 302 + assert service.authorization_calls + + +def test_callback_passes_stable_values_and_serializes_session_cookies( + app: Flask, + monkeypatch: pytest.MonkeyPatch, +) -> None: + service = FakeOAuthService() + _install_service(monkeypatch, service) + cookie_calls: list[tuple[str, str]] = [] + monkeypatch.setattr(oauth_controller, "extract_remote_ip", lambda _request: "203.0.113.10") + monkeypatch.setattr( + oauth_controller, + "set_access_token_to_cookie", + lambda _request, _response, token: cookie_calls.append(("access", token)), + ) + monkeypatch.setattr( + oauth_controller, + "set_refresh_token_to_cookie", + lambda _request, _response, token: cookie_calls.append(("refresh", token)), + ) + monkeypatch.setattr( + oauth_controller, + "set_csrf_token_to_cookie", + lambda _request, _response, token: cookie_calls.append(("csrf", token)), + ) + state = encode_oauth_state( + invite_token="invite", + timezone="Asia/Shanghai", + language="zh-Hans", + redirect_url="/apps", + ) + + with app.test_request_context( + f"/oauth/authorize/github?code=code-1&state={state}", + headers={"Accept-Language": "en-US,en;q=0.9"}, + ): + response = OAuthCallback().get("github") + + assert response.status_code == 302 + assert response.headers["Location"] == "/apps?oauth_new_user=false" + assert service.callback_calls == [ + OAuthCallbackCommand( + provider="github", + code="code-1", + invite_token="invite", + timezone="Asia/Shanghai", + language="zh-Hans", + browser_language="en-US", ip_address="203.0.113.10", ) - mock_redirect.assert_called_once_with("http://localhost:3000?oauth_new_user=true") - - @pytest.mark.parametrize( - ("service_error", "expected_message"), - [ - ( - EmailDomainSuspendedRegistrationError(), - "This email domain has been suspended.", - ), - (AccountRegisterError("This email account is frozen."), "This email account is frozen."), - ], - ) - @patch("controllers.console.auth.oauth.get_oauth_providers") - @patch("controllers.console.auth.oauth._generate_account") - @patch("controllers.console.auth.oauth.redirect") - def test_should_translate_registration_freeze_errors( - self, - mock_redirect, - mock_generate_account, - mock_get_providers, - resource: OAuthCallback, - app: Flask, - oauth_setup, - service_error, - expected_message, - ): - mock_get_providers.return_value = {"github": oauth_setup["provider"]} - mock_generate_account.side_effect = service_error - - with app.test_request_context("/auth/oauth/github/callback?code=test_code"): - resource.get("github") - - mock_redirect.assert_called_once_with(f"http://localhost:3000/signin?message={expected_message}") - - @pytest.mark.parametrize( - ("exception", "expected_error"), - [ - (Exception("OAuth error"), "OAuth process failed"), - (ValueError("Invalid token"), "OAuth process failed"), - (KeyError("Missing key"), "OAuth process failed"), - ], - ) - @patch("controllers.console.auth.oauth.get_oauth_providers") - def test_should_handle_oauth_exceptions( - self, mock_get_providers, resource: OAuthCallback, app: Flask, exception, expected_error - ): - # Import the real requests module to create a proper exception - import httpx - - request_exception = httpx.RequestError("OAuth error") - request_exception.response = MagicMock() - request_exception.response.text = str(exception) - - mock_oauth_provider = MagicMock() - mock_oauth_provider.get_access_token.side_effect = request_exception - mock_get_providers.return_value = {"github": mock_oauth_provider} - - with app.test_request_context("/auth/oauth/github/callback?code=test_code"): - response, status_code = resource.get("github") - - assert status_code == 400 - assert response["error"] == expected_error - - @patch("controllers.console.auth.oauth.get_oauth_providers") - @patch("controllers.console.auth.oauth.RegisterService") - @patch("controllers.console.auth.oauth.AccountService") - @patch("controllers.console.auth.oauth.redirect") - def test_invitation_comparison_is_case_insensitive( - self, - mock_redirect, - mock_account_service, - mock_register_service, - mock_get_providers, - resource: OAuthCallback, - app: Flask, - oauth_setup, - ): - oauth_setup["provider"].get_user_info.return_value = OAuthUserInfo( - id="123", name="Test User", email="User@Example.com" - ) - mock_get_providers.return_value = {"github": oauth_setup["provider"]} - mock_register_service.is_valid_invite_token.return_value = True - mock_register_service.get_invitation_if_token_valid.return_value = { - "account": oauth_setup["account"], - "data": {"email": "user@example.com"}, - "tenant": Tenant(name="Invited Workspace"), - } - mock_account_service.login.return_value = oauth_setup["token_pair"] - - state = encode_oauth_state(invite_token="invite123", timezone="Asia/Shanghai") - with app.test_request_context(f"/auth/oauth/github/callback?code=test_code&state={state}"): - resource.get("github") - - mock_register_service.get_invitation_if_token_valid.assert_called_once_with( - None, None, "invite123", session=ANY - ) - mock_redirect.assert_called_once_with("http://localhost:3000/signin/invite-settings?invite_token=invite123") - - @pytest.mark.parametrize( - ("account_status", "expected_redirect"), - [ - (AccountStatus.BANNED, "http://localhost:3000/signin?message=Account is banned."), - # CLOSED status: Currently NOT handled, will proceed to login (security issue) - # This documents actual behavior. See test_defensive_check_for_closed_account_status for details - ( - AccountStatus.CLOSED.value, - "http://localhost:3000?oauth_new_user=false", - ), - ], - ) - @patch("controllers.console.auth.oauth.AccountService") - @patch("controllers.console.auth.oauth.TenantService") - @patch("controllers.console.auth.oauth.get_oauth_providers") - @patch("controllers.console.auth.oauth._generate_account") - @patch("controllers.console.auth.oauth.redirect") - def test_should_redirect_based_on_account_status( - self, - mock_redirect, - mock_generate_account, - mock_get_providers, - mock_tenant_service, - mock_account_service, - resource: OAuthCallback, - app: Flask, - oauth_setup, - account_status, - expected_redirect, - ): - - mock_get_providers.return_value = {"github": oauth_setup["provider"]} - - account = Account(name="Test User", email="test@example.com", status=account_status) - account.id = "123" - mock_generate_account.return_value = (account, False) - - # Mock login for CLOSED status - mock_token_pair = MagicMock() - mock_token_pair.access_token = "jwt_access_token" - mock_token_pair.refresh_token = "jwt_refresh_token" - mock_token_pair.csrf_token = "csrf_token" - mock_account_service.login.return_value = mock_token_pair - - with app.test_request_context("/auth/oauth/github/callback?code=test_code"): - resource.get("github") - - mock_redirect.assert_called_once_with(expected_redirect) - - @patch("controllers.console.auth.oauth.get_oauth_providers") - @patch("controllers.console.auth.oauth._generate_account") - @patch("controllers.console.auth.oauth.TenantService") - @patch("controllers.console.auth.oauth.AccountService") - def test_should_activate_pending_account( - self, - mock_account_service, - mock_tenant_service, - mock_generate_account, - mock_get_providers, - resource: OAuthCallback, - app: Flask, - oauth_setup, - ): - mock_get_providers.return_value = {"github": oauth_setup["provider"]} - - mock_account = Account(name="Test User", email="test@example.com", status=AccountStatus.PENDING) - mock_generate_account.return_value = (mock_account, False) - - mock_token_pair = MagicMock() - mock_token_pair.access_token = "jwt_access_token" - mock_token_pair.refresh_token = "jwt_refresh_token" - mock_token_pair.csrf_token = "csrf_token" - mock_account_service.login.return_value = mock_token_pair - - with app.test_request_context("/auth/oauth/github/callback?code=test_code"): - resource.get("github") - - assert mock_account.status == AccountStatus.ACTIVE - assert mock_account.initialized_at is not None - - @patch("controllers.console.auth.oauth.get_oauth_providers") - @patch("controllers.console.auth.oauth._generate_account") - @patch("controllers.console.auth.oauth.TenantService") - @patch("controllers.console.auth.oauth.AccountService") - @patch("controllers.console.auth.oauth.redirect") - def test_defensive_check_for_closed_account_status( - self, - mock_redirect, - mock_account_service, - mock_tenant_service, - mock_generate_account, - mock_get_providers, - resource: OAuthCallback, - app: Flask, - oauth_setup, - ): - """Defensive test for CLOSED account status handling in OAuth callback. - - This is a defensive test documenting expected security behavior for CLOSED accounts. - - Current behavior: CLOSED status is NOT checked, allowing closed accounts to login. - Expected behavior: CLOSED accounts should be rejected like BANNED accounts. - - Context: - - AccountStatus.CLOSED is defined in the enum but never used in production - - No production service path sets accounts to CLOSED - - Account deletion uses external service instead of status change - - All authentication services (OAuth, password, email) don't check CLOSED status - - TODO: If CLOSED status is implemented in the future: - 1. Update OAuth callback to check for CLOSED status - 2. Add similar checks to all authentication services for consistency - 3. Update this test to verify the rejection behavior - - Security consideration: Until properly implemented, CLOSED status provides no protection. - """ - # Setup - mock_get_providers.return_value = {"github": oauth_setup["provider"]} - - # Create account with CLOSED status - closed_account = Account(name="Closed Account", email="closed@example.com", status=AccountStatus.CLOSED) - closed_account.id = "123" - mock_generate_account.return_value = (closed_account, False) - - # Mock successful login (current behavior) - mock_token_pair = MagicMock() - mock_token_pair.access_token = "jwt_access_token" - mock_token_pair.refresh_token = "jwt_refresh_token" - mock_token_pair.csrf_token = "csrf_token" - mock_account_service.login.return_value = mock_token_pair - - # Execute OAuth callback - with app.test_request_context("/auth/oauth/github/callback?code=test_code"): - resource.get("github") - - # Verify current behavior: login succeeds (this is NOT ideal) - mock_redirect.assert_called_once_with("http://localhost:3000?oauth_new_user=false") - mock_account_service.login.assert_called_once() - - # Document expected behavior in comments: - # Expected: mock_redirect.assert_called_once_with( - # "http://localhost:3000/signin?message=Account is closed." - # ) - # Expected: mock_account_service.login.assert_not_called() + ] + assert cookie_calls == [ + ("access", "access-token"), + ("refresh", "refresh-token"), + ("csrf", "csrf-token"), + ] -class TestAccountGeneration: - @pytest.fixture - def user_info(self): - return OAuthUserInfo(id="123", name="Test User", email="test@example.com") +@pytest.mark.parametrize( + ("redirect_url", "expected"), + [ + ("https://console.example.com/apps", "https://console.example.com/apps?oauth_new_user=false"), + ("https://console.example.com.malicious.example/apps", f"{CONSOLE_WEB_URL}?oauth_new_user=false"), + ("//malicious.example.com/apps", f"{CONSOLE_WEB_URL}?oauth_new_user=false"), + ("///malicious.example.com/apps", f"{CONSOLE_WEB_URL}?oauth_new_user=false"), + (r"\\malicious.example.com/apps", f"{CONSOLE_WEB_URL}?oauth_new_user=false"), + ], +) +def test_callback_serializes_safe_redirect_target( + app: Flask, + monkeypatch: pytest.MonkeyPatch, + redirect_url: str, + expected: str, +) -> None: + service = FakeOAuthService() + _install_service(monkeypatch, service) + state = encode_oauth_state(redirect_url=redirect_url) - @pytest.fixture - def mock_account(self) -> Account: - return Account(name="Test User", email="test@example.com") + with app.test_request_context(f"/oauth/authorize/github?code=code-1&state={state}"): + response = OAuthCallback().get("github") - @patch("controllers.console.auth.oauth.AccountService.get_account_by_email_with_case_fallback") - def test_should_get_account_by_openid_or_email( - self, - mock_get_account, - app: Flask, - user_info: OAuthUserInfo, - sqlite_session: Session, - ): - account = Account(name="Test User", email="test@example.com") - sqlite_session.add(account) - sqlite_session.flush() - sqlite_session.add( - AccountIntegrate( - account_id=account.id, - provider="github", - open_id="123", - encrypted_token="encrypted-token", - ) - ) - sqlite_session.commit() - database_session = scoped_session(sessionmaker(bind=sqlite_session.get_bind(), expire_on_commit=False)) + assert response.headers["Location"] == expected - with patch("controllers.console.auth.oauth.db.session", database_session), app.test_request_context("/"): - # Test OpenID found - result = _get_account_by_openid_or_email("github", user_info) - assert result is not None - assert result.id == account.id - mock_get_account.assert_not_called() - # Test fallback to email lookup - mock_get_account.return_value = account +def test_callback_serializes_invitation_completion_target( + app: Flask, + monkeypatch: pytest.MonkeyPatch, +) -> None: + tokens = AccountSessionTokens("access-token", "refresh-token", "csrf-token") + service = FakeOAuthService(callback_result=OAuthInvitationResult(tokens=tokens, invite_token="invite token")) + _install_service(monkeypatch, service) - result = _get_account_by_openid_or_email("google", user_info) - assert result is account - mock_get_account.assert_called_once() - database_session.remove() + with app.test_request_context("/oauth/authorize/github?code=code-1"): + response = OAuthCallback().get("github") - @pytest.mark.parametrize( - ("allow_register", "existing_account", "should_create"), - [ - (True, None, True), # New account creation allowed - (True, "existing", False), # Existing account - (False, None, False), # Registration not allowed - ], - ) - @patch("controllers.console.auth.oauth._get_account_by_openid_or_email") - @patch("controllers.console.auth.oauth.SystemFeatureService") - @patch("controllers.console.auth.oauth.RegisterService") - @patch("controllers.console.auth.oauth.AccountService") - @patch("controllers.console.auth.oauth.TenantService") - def test_should_handle_account_generation_scenarios( - self, - mock_tenant_service: MagicMock, - mock_account_service: MagicMock, - mock_register_service: MagicMock, - mock_feature_service: MagicMock, - mock_get_account: MagicMock, - app: Flask, - user_info: OAuthUserInfo, - mock_account, - allow_register, - existing_account, - should_create, - ): - mock_get_account.return_value = mock_account if existing_account else None - mock_feature_service.is_registration_allowed.return_value = allow_register - mock_register_service.register.return_value = mock_account + assert response.headers["Location"] == f"{CONSOLE_WEB_URL}/signin/invite-settings?invite_token=invite+token" - with app.test_request_context(headers={"Accept-Language": "en-US,en;q=0.9"}): - if not allow_register and not existing_account: - with pytest.raises(AccountRegisterError): - _generate_account("github", user_info) - else: - result, oauth_new_user = _generate_account("github", user_info) - assert result == mock_account - assert oauth_new_user == should_create - if should_create: - mock_register_service.register.assert_called_once_with( - email="test@example.com", - name="Test User", - password=None, - open_id="123", - provider="github", - language="en-US", - timezone=None, - ip_address=None, - session=ANY, - ) - else: - mock_register_service.register.assert_not_called() +def test_callback_rejects_missing_code_before_service_call( + app: Flask, + monkeypatch: pytest.MonkeyPatch, +) -> None: + service = FakeOAuthService() + _install_service(monkeypatch, service) - @pytest.mark.parametrize( - ("freeze_type", "expected_error"), - [ - ("email_domain_suspended", EmailDomainSuspendedRegistrationError), - ("freeze", AccountRegisterError), - ], - ) - @config_overrides_context(DEPLOYMENT_EDITION=DeploymentEdition.CLOUD) - @patch("controllers.console.auth.oauth.BillingService.get_email_freeze_type") - @patch("controllers.console.auth.oauth._get_account_by_openid_or_email", return_value=None) - @patch("controllers.console.auth.oauth.SystemFeatureService") - def test_should_reject_registration_for_frozen_email( - self, - mock_feature_service, - mock_get_account, - mock_get_freeze_type, - freeze_type, - expected_error, - app: Flask, - user_info: OAuthUserInfo, - ): - mock_feature_service.is_registration_allowed.return_value = False - mock_get_freeze_type.return_value = freeze_type + with app.test_request_context("/oauth/authorize/github"), pytest.raises(UnprocessableEntity): + OAuthCallback().get("github") - with app.test_request_context("/"): - with pytest.raises(expected_error): - _generate_account("github", user_info) + assert service.callback_calls == [] - mock_get_freeze_type.assert_called_once_with("test@example.com") - @patch("controllers.console.auth.oauth._get_account_by_openid_or_email", return_value=None) - @patch("controllers.console.auth.oauth.SystemFeatureService") - @patch("controllers.console.auth.oauth.RegisterService") - @patch("controllers.console.auth.oauth.AccountService") - @patch("controllers.console.auth.oauth.TenantService") - def test_should_register_with_lowercase_email( - self, - mock_tenant_service: MagicMock, - mock_account_service: MagicMock, - mock_register_service: MagicMock, - mock_feature_service: MagicMock, - mock_get_account: MagicMock, - app: Flask, - ): - user_info = OAuthUserInfo(id="123", name="Test User", email="Upper@Example.com") - mock_feature_service.is_registration_allowed.return_value = True - mock_register_service.register.return_value = Account(name="Test User", email="upper@example.com") +@pytest.mark.parametrize("error", [OAuthProviderRequestError(), OAuthIdentityLockUnavailableError()]) +def test_callback_maps_oauth_processing_error_to_bad_request( + app: Flask, + monkeypatch: pytest.MonkeyPatch, + error: Exception, +) -> None: + service = FakeOAuthService(callback_error=error) + _install_service(monkeypatch, service) - with app.test_request_context(headers={"Accept-Language": "en-US"}): - _generate_account("github", user_info) + with app.test_request_context("/oauth/authorize/github?code=code-1"): + payload, status = OAuthCallback().get("github") - mock_register_service.register.assert_called_once_with( - email="upper@example.com", - name="Test User", - password=None, - open_id="123", - provider="github", - language="en-US", - timezone=None, - ip_address=None, - session=ANY, - ) + assert status == 400 + assert payload == {"error": "OAuth process failed"} - @patch("controllers.console.auth.oauth._get_account_by_openid_or_email", return_value=None) - @patch("controllers.console.auth.oauth.SystemFeatureService") - @patch("controllers.console.auth.oauth.RegisterService") - @patch("controllers.console.auth.oauth.AccountService") - @patch("controllers.console.auth.oauth.TenantService") - def test_should_register_with_browser_timezone( - self, - mock_tenant_service: MagicMock, - mock_account_service: MagicMock, - mock_register_service: MagicMock, - mock_feature_service: MagicMock, - mock_get_account: MagicMock, - app: Flask, - user_info: OAuthUserInfo, - ): - mock_feature_service.is_registration_allowed.return_value = True - mock_register_service.register.return_value = Account(name="Test User", email="test@example.com") - with app.test_request_context(headers={"Accept-Language": "zh-Hans,zh;q=0.9"}): - _generate_account("github", user_info, timezone="Asia/Shanghai") +@pytest.mark.parametrize( + ("error", "expected_query"), + [ + ( + OAuthInvitationAccountMismatchError("invite-token"), + "message=This+invitation+was+sent+to+another+account.+Please+sign+in+with+the+invited+account." + "&invite_token=invite-token", + ), + (OAuthRegistrationError("Registration failed"), "message=Registration+failed"), + ], +) +def test_callback_serializes_application_errors_as_signin_redirects( + app: Flask, + monkeypatch: pytest.MonkeyPatch, + error: Exception, + expected_query: str, +) -> None: + service = FakeOAuthService(callback_error=error) + _install_service(monkeypatch, service) - mock_register_service.register.assert_called_once_with( - email="test@example.com", - name="Test User", - password=None, - open_id="123", - provider="github", - language="zh-Hans", - timezone="Asia/Shanghai", - ip_address=None, - session=ANY, - ) + with app.test_request_context("/oauth/authorize/github?code=code-1"): + response = OAuthCallback().get("github") - @patch("controllers.console.auth.oauth._get_account_by_openid_or_email", return_value=None) - @patch("controllers.console.auth.oauth.SystemFeatureService") - @patch("controllers.console.auth.oauth.RegisterService") - @patch("controllers.console.auth.oauth.AccountService") - @patch("controllers.console.auth.oauth.TenantService") - def test_should_register_with_state_language( - self, - mock_tenant_service: MagicMock, - mock_account_service: MagicMock, - mock_register_service: MagicMock, - mock_feature_service: MagicMock, - mock_get_account: MagicMock, - app: Flask, - user_info: OAuthUserInfo, - ): - mock_feature_service.is_registration_allowed.return_value = True - mock_register_service.register.return_value = Account(name="Test User", email="test@example.com") + assert response.status_code == 302 + assert response.headers["Location"] == f"{CONSOLE_WEB_URL}/signin?{expected_query}" - with app.test_request_context(headers={"Accept-Language": "en-US,en;q=0.9"}): - _generate_account("github", user_info, language="zh-Hans") - mock_register_service.register.assert_called_once_with( - email="test@example.com", - name="Test User", - password=None, - open_id="123", - provider="github", - language="zh-Hans", - timezone=None, - ip_address=None, - session=ANY, - ) +def test_oauth_admission_rejects_disabled_social_login( + app: Flask, + monkeypatch: pytest.MonkeyPatch, + config_overrides: Callable[..., None], +) -> None: + service = FakeOAuthService() + _install_service(monkeypatch, service) + config_overrides(ENABLE_SOCIAL_OAUTH_LOGIN=False) - @patch("controllers.console.auth.oauth._get_account_by_openid_or_email") - @patch("controllers.console.auth.oauth.TenantService") - @patch("controllers.console.auth.oauth.SystemFeatureService") - @patch("controllers.console.auth.oauth.AccountService") - def test_should_create_workspace_for_account_without_tenant( - self, - mock_account_service: MagicMock, - mock_feature_service: MagicMock, - mock_tenant_service: MagicMock, - mock_get_account: MagicMock, - app: Flask, - user_info: OAuthUserInfo, - mock_account, - ): - mock_get_account.return_value = mock_account - mock_tenant_service.get_join_tenants.return_value = [] - mock_feature_service.is_workspace_creation_allowed.return_value = True + with app.test_request_context("/oauth/login/github"), pytest.raises(Forbidden): + OAuthLogin().get("github") - with app.test_request_context(headers={"Accept-Language": "en-US,en;q=0.9"}): - result, oauth_new_user = _generate_account("github", user_info) - - assert result == mock_account - assert oauth_new_user is False - mock_tenant_service.create_owner_tenant.assert_called_once_with(mock_account, session=ANY) + assert service.authorization_calls == [] diff --git a/api/tests/unit_tests/controllers/console/auth/test_oauth_redirect.py b/api/tests/unit_tests/controllers/console/auth/test_oauth_redirect.py deleted file mode 100644 index e3910d4a348..00000000000 --- a/api/tests/unit_tests/controllers/console/auth/test_oauth_redirect.py +++ /dev/null @@ -1,190 +0,0 @@ -import urllib.parse -from unittest.mock import ANY, MagicMock, patch - -import pytest -from flask import Flask - -from controllers.console.auth.oauth import OAuthCallback, OAuthLogin -from libs.oauth import OAuthUserInfo, encode_oauth_state -from models.account import Account, AccountStatus, Tenant -from tests.unit_tests.config_override import config_overrides_context - -REDIRECT_URL = "/apps?category=workflow" -CONSOLE_WEB_URL = "https://console.example.com" - - -@pytest.fixture -def app() -> Flask: - app = Flask(__name__) - app.config["TESTING"] = True - return app - - -def test_oauth_login_passes_relative_redirect_url_through(app: Flask) -> None: - oauth_provider = MagicMock() - oauth_provider.get_authorization_url.return_value = "https://accounts.google.com/o/oauth2/v2/auth?state=..." - query = urllib.parse.urlencode({"redirect_url": REDIRECT_URL}) - - with ( - patch("controllers.console.auth.oauth.get_oauth_providers", return_value={"google": oauth_provider}), - app.test_request_context(f"/oauth/login/google?{query}"), - ): - response = OAuthLogin().get("google") - - oauth_provider.get_authorization_url.assert_called_once_with( - invite_token=None, - timezone=None, - language=None, - redirect_url=REDIRECT_URL, - ) - assert response.status_code == 302 - assert response.headers["Location"] == "https://accounts.google.com/o/oauth2/v2/auth?state=..." - - -@pytest.mark.parametrize( - ("redirect_url", "expected_target_url"), - [ - (REDIRECT_URL, REDIRECT_URL), - (f"{CONSOLE_WEB_URL}{REDIRECT_URL}", f"{CONSOLE_WEB_URL}{REDIRECT_URL}"), - ("https://console.example.com.malicious.example/apps", CONSOLE_WEB_URL), - ("//malicious.example.com/apps", CONSOLE_WEB_URL), - ("///malicious.example.com/apps", CONSOLE_WEB_URL), - (r"\\malicious.example.com/apps", CONSOLE_WEB_URL), - ], -) -@pytest.mark.parametrize("oauth_new_user", [False, True]) -def test_oauth_callback_validates_redirect_url_and_appends_new_user_flag( - app: Flask, - redirect_url: str, - expected_target_url: str, - oauth_new_user: bool, -) -> None: - oauth_provider = MagicMock() - oauth_provider.get_access_token.return_value = "google-access-token" - oauth_provider.get_user_info.return_value = OAuthUserInfo( - id="google-user-123", - name="Test User", - email="test@example.com", - ) - account = Account(name="Test User", email="test@example.com", status=AccountStatus.ACTIVE) - token_pair = MagicMock() - token_pair.access_token = "dify-access-token" - token_pair.refresh_token = "dify-refresh-token" - token_pair.csrf_token = "dify-csrf-token" - state = encode_oauth_state(redirect_url=redirect_url) - - with ( - patch("controllers.console.auth.oauth.get_oauth_providers", return_value={"google": oauth_provider}), - config_overrides_context(CONSOLE_WEB_URL=CONSOLE_WEB_URL), - patch("controllers.console.auth.oauth._generate_account", return_value=(account, oauth_new_user)), - patch("controllers.console.auth.oauth.TenantService.create_owner_tenant_if_not_exist"), - patch("controllers.console.auth.oauth.AccountService.login", return_value=token_pair), - patch("controllers.console.auth.oauth.set_access_token_to_cookie"), - patch("controllers.console.auth.oauth.set_refresh_token_to_cookie"), - patch("controllers.console.auth.oauth.set_csrf_token_to_cookie"), - app.test_request_context(f"/oauth/authorize/google?code=test-code&state={state}"), - ): - response = OAuthCallback().get("google") - - assert response.status_code == 302 - query_char = "&" if "?" in expected_target_url else "?" - assert response.headers["Location"] == ( - f"{expected_target_url}{query_char}oauth_new_user={str(oauth_new_user).lower()}" - ) - - -def test_oauth_callback_with_invitation_establishes_console_session(app: Flask) -> None: - oauth_provider = MagicMock() - oauth_provider.get_access_token.return_value = "google-access-token" - oauth_provider.get_user_info.return_value = OAuthUserInfo( - id="google-user-123", - name="Test User", - email="Invitee@Example.com", - ) - account = Account(name="Test User", email="invitee@example.com", status=AccountStatus.ACTIVE) - token_pair = MagicMock() - token_pair.access_token = "dify-access-token" - token_pair.refresh_token = "dify-refresh-token" - token_pair.csrf_token = "dify-csrf-token" - state = encode_oauth_state(invite_token="invite-token") - - with ( - patch("controllers.console.auth.oauth.get_oauth_providers", return_value={"google": oauth_provider}), - config_overrides_context(CONSOLE_WEB_URL=CONSOLE_WEB_URL), - patch("controllers.console.auth.oauth.RegisterService") as register_service, - patch("controllers.console.auth.oauth.AccountService.link_account_integrate") as link_account, - patch("controllers.console.auth.oauth.AccountService.login", return_value=token_pair) as login, - patch("controllers.console.auth.oauth.TenantService.create_owner_tenant_if_not_exist") as create_workspace, - patch("controllers.console.auth.oauth.set_access_token_to_cookie") as set_access_cookie, - patch("controllers.console.auth.oauth.set_refresh_token_to_cookie") as set_refresh_cookie, - patch("controllers.console.auth.oauth.set_csrf_token_to_cookie") as set_csrf_cookie, - app.test_request_context(f"/oauth/authorize/google?code=test-code&state={state}"), - ): - register_service.is_valid_invite_token.return_value = True - register_service.get_invitation_if_token_valid.return_value = { - "account": account, - "data": { - "account_id": "account-id", - "email": "invitee@example.com", - "workspace_id": "workspace-id", - }, - "tenant": Tenant(name="Invited Workspace"), - } - - response = OAuthCallback().get("google") - - assert response.status_code == 302 - assert response.headers["Location"] == (f"{CONSOLE_WEB_URL}/signin/invite-settings?invite_token=invite-token") - link_account.assert_called_once_with("google", "google-user-123", account, session=ANY) - login.assert_called_once_with(account=account, session=ANY, ip_address=ANY) - create_workspace.assert_not_called() - set_access_cookie.assert_called_once_with(ANY, response, "dify-access-token") - set_refresh_cookie.assert_called_once_with(ANY, response, "dify-refresh-token") - set_csrf_cookie.assert_called_once_with(ANY, response, "dify-csrf-token") - - -def test_oauth_callback_with_invitation_rejects_another_account(app: Flask) -> None: - oauth_provider = MagicMock() - oauth_provider.get_access_token.return_value = "google-access-token" - oauth_provider.get_user_info.return_value = OAuthUserInfo( - id="google-user-123", - name="Test User", - email="another@example.com", - ) - account = Account(name="Test User", email="another@example.com", status=AccountStatus.ACTIVE) - state = encode_oauth_state(invite_token="invite-token") - - with ( - patch("controllers.console.auth.oauth.get_oauth_providers", return_value={"google": oauth_provider}), - config_overrides_context(CONSOLE_WEB_URL=CONSOLE_WEB_URL), - patch("controllers.console.auth.oauth.RegisterService") as register_service, - patch("controllers.console.auth.oauth.AccountService.link_account_integrate") as link_account, - patch("controllers.console.auth.oauth.AccountService.login") as login, - patch("controllers.console.auth.oauth.set_access_token_to_cookie") as set_access_cookie, - patch("controllers.console.auth.oauth.set_refresh_token_to_cookie") as set_refresh_cookie, - patch("controllers.console.auth.oauth.set_csrf_token_to_cookie") as set_csrf_cookie, - app.test_request_context(f"/oauth/authorize/google?code=test-code&state={state}"), - ): - register_service.is_valid_invite_token.return_value = True - register_service.get_invitation_if_token_valid.return_value = { - "account": account, - "data": { - "account_id": "account-id", - "email": "invitee@example.com", - "workspace_id": "workspace-id", - }, - "tenant": Tenant(name="Invited Workspace"), - } - - response = OAuthCallback().get("google") - - query = urllib.parse.parse_qs(urllib.parse.urlparse(response.headers["Location"]).query) - assert response.status_code == 302 - assert query["message"] == ["This invitation was sent to another account. Please sign in with the invited account."] - assert query["invite_token"] == ["invite-token"] - link_account.assert_not_called() - login.assert_not_called() - register_service.revoke_token.assert_not_called() - set_access_cookie.assert_not_called() - set_refresh_cookie.assert_not_called() - set_csrf_cookie.assert_not_called() diff --git a/api/tests/unit_tests/controllers/console/auth/test_oauth_timezone.py b/api/tests/unit_tests/controllers/console/auth/test_oauth_timezone.py deleted file mode 100644 index f4a332305cf..00000000000 --- a/api/tests/unit_tests/controllers/console/auth/test_oauth_timezone.py +++ /dev/null @@ -1,131 +0,0 @@ -from unittest.mock import ANY, MagicMock, patch - -import pytest -from flask import Flask - -from controllers.console.auth.oauth import OAuthLogin, _generate_account -from enums import DeploymentEdition -from libs.oauth import OAuthUserInfo -from models.account import Account -from services.errors.account import AccountRegisterError - - -@pytest.fixture -def app() -> Flask: - app = Flask(__name__) - app.config["TESTING"] = True - return app - - -@patch("controllers.console.auth.oauth.redirect") -@patch("controllers.console.auth.oauth.get_oauth_providers") -def test_oauth_login_passes_language_and_timezone_to_authorization_url( - mock_get_oauth_providers, - mock_redirect, - app: Flask, -): - oauth_provider = MagicMock() - oauth_provider.get_authorization_url.return_value = "https://github.com/login/oauth/authorize?state=..." - mock_get_oauth_providers.return_value = {"github": oauth_provider} - - with app.test_request_context("/oauth/login/github?language=zh-Hans&timezone=Asia/Shanghai"): - OAuthLogin().get("github") - - oauth_provider.get_authorization_url.assert_called_once_with( - invite_token=None, - timezone="Asia/Shanghai", - language="zh-Hans", - redirect_url=None, - ) - mock_redirect.assert_called_once_with("https://github.com/login/oauth/authorize?state=...") - - -@patch("controllers.console.auth.oauth.AccountService.link_account_integrate") -@patch("controllers.console.auth.oauth.RegisterService") -@patch("controllers.console.auth.oauth.SystemFeatureService") -@patch("controllers.console.auth.oauth._get_account_by_openid_or_email", return_value=None) -def test_generate_account_registers_with_browser_timezone( - mock_get_account, - mock_feature_service, - mock_register_service, - mock_link_account, - app: Flask, -): - account = Account(name="Test User", email="user@example.com") - mock_register_service.register.return_value = account - mock_feature_service.is_registration_allowed.return_value = True - user_info = OAuthUserInfo(id="github-123", name="Test User", email="User@Example.com") - - with app.test_request_context(headers={"Accept-Language": "zh-Hans,zh;q=0.9"}): - result, oauth_new_user = _generate_account( - "github", user_info, timezone="Asia/Shanghai", ip_address="203.0.113.10" - ) - - assert result is account - assert oauth_new_user is True - mock_register_service.register.assert_called_once_with( - email="user@example.com", - name="Test User", - password=None, - open_id="github-123", - provider="github", - language="zh-Hans", - timezone="Asia/Shanghai", - ip_address="203.0.113.10", - session=ANY, - ) - mock_link_account.assert_called_once_with("github", "github-123", account, session=ANY) - - -@patch("controllers.console.auth.oauth.AccountService.link_account_integrate") -@patch("controllers.console.auth.oauth.RegisterService") -@patch("controllers.console.auth.oauth.SystemFeatureService") -@patch("controllers.console.auth.oauth._get_account_by_openid_or_email", return_value=None) -def test_generate_account_prefers_state_language_over_accept_language( - mock_get_account, - mock_feature_service, - mock_register_service, - mock_link_account, - app: Flask, -): - account = Account(name="Test User", email="user@example.com") - mock_register_service.register.return_value = account - mock_feature_service.is_registration_allowed.return_value = True - user_info = OAuthUserInfo(id="github-123", name="Test User", email="User@Example.com") - - with app.test_request_context(headers={"Accept-Language": "en-US,en;q=0.9"}): - _generate_account("github", user_info, language="zh-Hans") - - mock_register_service.register.assert_called_once_with( - email="user@example.com", - name="Test User", - password=None, - open_id="github-123", - provider="github", - language="zh-Hans", - timezone=None, - ip_address=None, - session=ANY, - ) - mock_link_account.assert_called_once_with("github", "github-123", account, session=ANY) - - -@patch("controllers.console.auth.oauth.RegisterService") -@patch("controllers.console.auth.oauth.SystemFeatureService") -@patch("controllers.console.auth.oauth._get_account_by_openid_or_email", return_value=None) -def test_generate_account_rejects_new_user_when_registration_disabled( - mock_get_account, - mock_feature_service, - mock_register_service, - app: Flask, - config_overrides, -): - mock_feature_service.is_registration_allowed.return_value = False - config_overrides(DEPLOYMENT_EDITION=DeploymentEdition.COMMUNITY) - user_info = OAuthUserInfo(id="github-123", name="Test User", email="user@example.com") - - with app.test_request_context(headers={"Accept-Language": "en-US,en;q=0.9"}): - with pytest.raises(AccountRegisterError): - _generate_account("github", user_info) - - mock_register_service.register.assert_not_called() diff --git a/api/tests/unit_tests/controllers/console/workspace/test_workspace.py b/api/tests/unit_tests/controllers/console/workspace/test_workspace.py index 76a87aeac66..dadfa2a23a5 100644 --- a/api/tests/unit_tests/controllers/console/workspace/test_workspace.py +++ b/api/tests/unit_tests/controllers/console/workspace/test_workspace.py @@ -205,6 +205,7 @@ class TestWorkspaceQueryRepository: ), TenantAccountJoin(tenant_id=later.id, account_id="account-1"), TenantAccountJoin(tenant_id=archived.id, account_id="account-1"), + TenantAccountJoin(tenant_id=archived.id, account_id="account-3"), TenantAccountJoin(tenant_id=other_account.id, account_id="account-2"), ] ) @@ -233,6 +234,8 @@ class TestWorkspaceQueryRepository: ), ) assert set(membership_ids) == {earlier.id, later.id, archived.id} + assert repository.has_active_membership("account-1") is True + assert repository.has_active_membership("account-3") is False class TestDeploymentWorkspacePlanGateway: diff --git a/api/tests/unit_tests/extensions/test_ext_application_services.py b/api/tests/unit_tests/extensions/test_ext_application_services.py index d0c60c0f4e2..ade8e369fa1 100644 --- a/api/tests/unit_tests/extensions/test_ext_application_services.py +++ b/api/tests/unit_tests/extensions/test_ext_application_services.py @@ -21,6 +21,12 @@ from models.account import Account from models.model import AccountTrialAppRecord, DifySetup from repositories.account_activation_repository import SQLAlchemyAccountActivationRepository from repositories.account_integration_repository import SQLAlchemyAccountIntegrationRepository +from repositories.account_oauth_repository import ( + AccountServiceOAuthAccountRegistrationGateway, + AccountServiceOAuthSessionGateway, + AccountServiceOAuthWorkspaceGateway, + RegisterServiceOAuthInvitationGateway, +) from repositories.account_repository import SQLAlchemyAccountRepository from repositories.app_site_command_repository import AppSiteCommandRepository from repositories.workflow_run_archive_repository import WorkflowRunArchiveBundleQueryRepository @@ -44,6 +50,10 @@ from services.account_forgot_password_adapters import ( RedisForgotPasswordSecurityGateway, RedisForgotPasswordTokenGateway, ) +from services.account_oauth_adapters import ( + DeploymentOAuthPolicyGateway, + RedisOAuthAccountClaimLock, +) from services.app_site_service import AppSiteService from services.auth.data_source_api_key_auth_service import DataSourceApiKeyAuthService from services.billing_portal_service import BillingPortalService @@ -430,6 +440,18 @@ def test_build_application_services_wires_account_profile_repository( integrations = services.accounts.integrations._integrations assert isinstance(integrations, SQLAlchemyAccountIntegrationRepository) assert integrations._session_factory is sqlite_session_factory + oauth = services.accounts.oauth + assert oauth._accounts is accounts + assert oauth._integrations is integrations + assert oauth._memberships is services.workspace_queries._workspaces + assert isinstance(oauth._invitations, RegisterServiceOAuthInvitationGateway) + assert isinstance(oauth._account_claims, RedisOAuthAccountClaimLock) + assert isinstance(oauth._registration, AccountServiceOAuthAccountRegistrationGateway) + assert isinstance(oauth._workspaces, AccountServiceOAuthWorkspaceGateway) + assert isinstance(oauth._sessions, AccountServiceOAuthSessionGateway) + assert oauth._sessions is not oauth._workspaces + assert isinstance(oauth._registration_policy, DeploymentOAuthPolicyGateway) + assert oauth._workspace_policy is oauth._registration_policy avatar_files = services.accounts.avatar._files assert isinstance(avatar_files, SQLAlchemyAccountAvatarFileGateway) assert avatar_files._session_factory is sqlite_session_factory diff --git a/api/tests/unit_tests/repositories/test_account_repository.py b/api/tests/unit_tests/repositories/test_account_repository.py index df182e62818..f41c2fadda4 100644 --- a/api/tests/unit_tests/repositories/test_account_repository.py +++ b/api/tests/unit_tests/repositories/test_account_repository.py @@ -195,6 +195,52 @@ def test_account_repository_finds_email_with_lowercase_fallback( assert account.email == "account@example.com" +def test_account_repositories_resolve_oauth_identity_and_email_fallback( + sqlite_session: Session, + sqlite_session_factory: sessionmaker[Session], +) -> None: + _persist_account(sqlite_session) + sqlite_session.add( + AccountIntegrate( + account_id="account-1", + provider="github", + open_id="github-user", + encrypted_token="", + ) + ) + sqlite_session.commit() + account_repository = SQLAlchemyAccountRepository(sqlite_session_factory) + integration_repository = SQLAlchemyAccountIntegrationRepository(sqlite_session_factory) + + oauth_account_id = integration_repository.find_account_id(provider="github", open_id="github-user") + oauth_account = account_repository.get(oauth_account_id) if oauth_account_id is not None else None + email_account = account_repository.find_by_email("ACCOUNT@Example.com") + + assert oauth_account is not None + assert oauth_account.id == "account-1" + assert email_account is not None + assert email_account.id == "account-1" + + +def test_account_repository_activates_only_pending_account( + sqlite_session: Session, + sqlite_session_factory: sessionmaker[Session], +) -> None: + account = _persist_account(sqlite_session) + account.status = AccountStatus.PENDING + sqlite_session.commit() + initialized_at = datetime(2026, 8, 24, 12, 0) + repository = SQLAlchemyAccountRepository(sqlite_session_factory) + + repository.activate_pending("account-1", initialized_at=initialized_at) + + sqlite_session.expire_all() + persisted = sqlite_session.get(Account, "account-1") + assert persisted is not None + assert persisted.status == AccountStatus.ACTIVE + assert persisted.initialized_at == initialized_at + + def test_account_repository_fails_closed_for_duplicate_email( sqlite_session: Session, sqlite_session_factory: sessionmaker[Session], @@ -234,6 +280,24 @@ def test_account_integration_repository_lists_integrations( assert integrations[0].provider == "github" +def test_account_integration_repository_upserts_provider_binding( + sqlite_session: Session, + sqlite_session_factory: sessionmaker[Session], +) -> None: + _persist_account(sqlite_session) + repository = SQLAlchemyAccountIntegrationRepository(sqlite_session_factory) + + repository.link("account-1", provider="github", open_id="first-user") + repository.link("account-1", provider="github", open_id="second-user") + + sqlite_session.expire_all() + integrations = list(sqlite_session.query(AccountIntegrate).all()) + assert len(integrations) == 1 + assert integrations[0].account_id == "account-1" + assert integrations[0].provider == "github" + assert integrations[0].open_id == "second-user" + + def test_account_repository_initializes_account_and_consumes_invitation_atomically( sqlite_session: Session, sqlite_session_factory: sessionmaker[Session], diff --git a/api/tests/unit_tests/services/test_account_oauth_adapters.py b/api/tests/unit_tests/services/test_account_oauth_adapters.py new file mode 100644 index 00000000000..18d5e1068bd --- /dev/null +++ b/api/tests/unit_tests/services/test_account_oauth_adapters.py @@ -0,0 +1,318 @@ +from threading import Event +from typing import override + +import httpx +import pytest +from redis.exceptions import LockNotOwnedError +from sqlalchemy import func, select +from sqlalchemy.orm import Session, sessionmaker + +from libs.oauth import JsonObject, OAuth, OAuthUserInfo +from models.account import Account, AccountStatus, Tenant, TenantAccountJoin +from repositories import account_oauth_repository +from repositories.account_oauth_repository import ( + AccountServiceOAuthAccountRegistrationGateway, + AccountServiceOAuthWorkspaceGateway, +) +from services import account_oauth_adapters +from services.account_errors import ( + OAuthIdentityLockUnavailableError, + OAuthProviderAuthorizationError, + OAuthProviderRequestError, + OAuthWorkspaceCreationNotAllowedError, +) +from services.account_oauth_adapters import DifyOAuthProviderGateway, RedisOAuthAccountClaimLock +from services.account_service import AccountService, TenantService +from services.entities.account_oauth_entities import OAuthAccountRegistration, OAuthAuthorizationRequest, OAuthIdentity +from services.errors.workspace import WorkspacesLimitExceededError + + +class StubOAuthClient(OAuth): + def __init__(self) -> None: + super().__init__("client-id", "client-secret", "https://api.example/callback") + self.authorization_args: tuple[str | None, str | None, str | None, str | None] | None = None + self.access_codes: list[str] = [] + self.user_tokens: list[str] = [] + self.failure: Exception | None = None + + @override + def get_authorization_url( + self, + invite_token: str | None = None, + timezone: str | None = None, + language: str | None = None, + redirect_url: str | None = None, + ) -> str: + self.authorization_args = (invite_token, timezone, language, redirect_url) + return "https://provider.example/authorize" + + @override + def get_access_token(self, code: str) -> str: + self.access_codes.append(code) + if self.failure is not None: + raise self.failure + return "provider-token" + + @override + def get_user_info(self, token: str) -> OAuthUserInfo: + self.user_tokens.append(token) + return OAuthUserInfo(id="provider-user", name="User", email="user@example.com") + + @override + def get_raw_user_info(self, token: str) -> JsonObject: + raise AssertionError(token) + + @override + def _transform_user_info(self, raw_info: JsonObject) -> OAuthUserInfo: + raise AssertionError(raw_info) + + +class StubRedisLock: + def __init__(self, *, acquire_result: bool = True, reacquire_error: Exception | None = None) -> None: + self.acquire_result = acquire_result + self.reacquire_error = reacquire_error + self.acquire_calls = 0 + self.reacquire_calls = 0 + self.release_calls = 0 + self.reacquired = Event() + + def acquire(self) -> bool: + self.acquire_calls += 1 + return self.acquire_result + + def reacquire(self) -> bool: + self.reacquire_calls += 1 + self.reacquired.set() + if self.reacquire_error is not None: + raise self.reacquire_error + return True + + def release(self) -> None: + self.release_calls += 1 + + +class StubRedisClient: + def __init__(self, *locks: StubRedisLock) -> None: + self._locks = locks + self.lock_calls: list[tuple[str, float | None, float | None, bool]] = [] + + def lock( + self, + name: str, + timeout: float | None = None, + sleep: float = 0.1, + blocking: bool = True, + blocking_timeout: float | None = None, + thread_local: bool = True, + ) -> StubRedisLock: + del sleep, blocking + self.lock_calls.append((name, timeout, blocking_timeout, thread_local)) + return self._locks[len(self.lock_calls) - 1] + + +def test_account_claim_lock_uses_redis_without_exposing_identity_or_email() -> None: + locks = (StubRedisLock(), StubRedisLock()) + client = StubRedisClient(*locks) + account_claims = RedisOAuthAccountClaimLock(client=client) # type: ignore[arg-type] + + with account_claims.acquire(provider="github", open_id="provider-user", email="user@example.com"): + assert all(lock.acquire_calls == 1 for lock in locks) + + assert all(lock.release_calls == 1 for lock in locks) + assert len(client.lock_calls) == 2 + for lock_name, timeout, blocking_timeout, thread_local in client.lock_calls: + assert lock_name.startswith("oauth:account-claim:") + assert "provider-user" not in lock_name + assert "user@example.com" not in lock_name + assert timeout == 60 + assert blocking_timeout == 10 + assert thread_local is False + assert [call[0] for call in client.lock_calls] == sorted(call[0] for call in client.lock_calls) + + +def test_account_claim_lock_hashes_final_account_id() -> None: + lock = StubRedisLock() + client = StubRedisClient(lock) + account_claims = RedisOAuthAccountClaimLock(client=client) # type: ignore[arg-type] + + with account_claims.acquire_account("account-1"): + assert lock.acquire_calls == 1 + + assert lock.release_calls == 1 + assert len(client.lock_calls) == 1 + lock_name, _, _, _ = client.lock_calls[0] + assert lock_name.startswith("oauth:account-claim:") + assert "account-1" not in lock_name + + +def test_account_claim_lock_renews_both_leases_while_the_flow_is_running(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(account_oauth_adapters, "_OAUTH_ACCOUNT_CLAIM_LOCK_RENEW_INTERVAL_SECONDS", 0.01) + locks = (StubRedisLock(), StubRedisLock()) + client = StubRedisClient(*locks) + account_claims = RedisOAuthAccountClaimLock(client=client) # type: ignore[arg-type] + + with account_claims.acquire(provider="github", open_id="provider-user", email="user@example.com"): + assert all(lock.reacquired.wait(timeout=1) for lock in locks) + + assert all(lock.reacquire_calls >= 1 for lock in locks) + assert all(lock.release_calls == 1 for lock in locks) + + +def test_account_claim_lease_notifies_caller_when_heartbeat_loses_ownership( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(account_oauth_adapters, "_OAUTH_ACCOUNT_CLAIM_LOCK_RENEW_INTERVAL_SECONDS", 0.01) + lock = StubRedisLock(reacquire_error=LockNotOwnedError("lease lost")) + client = StubRedisClient(lock) + account_claims = RedisOAuthAccountClaimLock(client=client) # type: ignore[arg-type] + writes: list[str] = [] + + def use_lost_lease() -> None: + with account_claims.acquire_account("account-1") as lease: + assert lock.reacquired.wait(timeout=1) + lease.ensure_owned() + writes.append("must-not-run") + + with pytest.raises(OAuthIdentityLockUnavailableError): + use_lost_lease() + + assert writes == [] + assert lock.release_calls == 1 + + +def test_account_claim_lock_releases_partial_acquisition_on_failure() -> None: + first_lock = StubRedisLock() + failed_lock = StubRedisLock(acquire_result=False) + client = StubRedisClient(first_lock, failed_lock) + account_claims = RedisOAuthAccountClaimLock(client=client) # type: ignore[arg-type] + + with pytest.raises(OAuthIdentityLockUnavailableError): + with account_claims.acquire(provider="github", open_id="provider-user", email="user@example.com"): + raise AssertionError("lock body must not run") + + assert first_lock.release_calls == 1 + assert failed_lock.release_calls == 0 + + +def test_registration_gateway_creates_only_the_account( + sqlite_session_factory: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[tuple[str, str, str, str | None, str | None, bool]] = [] + + def create_account( + email: str, + name: str, + interface_language: str, + password: str | None = None, + interface_theme: str = "light", + is_setup: bool | None = False, + timezone: str | None = None, + ip_address: str | None = None, + check_normalized_email: bool = False, + *, + session: Session, + ) -> Account: + del password, interface_theme, is_setup + calls.append((email, name, interface_language, timezone, ip_address, check_normalized_email)) + account = Account( + email=email, + name=name, + interface_language=interface_language, + timezone=timezone, + last_login_ip=ip_address, + ) + session.add(account) + session.flush() + return account + + def unexpected_default_workspace_join(account_id: str) -> None: + raise AssertionError(account_id) + + monkeypatch.setattr(AccountService, "create_account", create_account) + monkeypatch.setattr(account_oauth_repository, "try_join_default_workspace", unexpected_default_workspace_join) + gateway = AccountServiceOAuthAccountRegistrationGateway(session_factory=sqlite_session_factory) + + account_id = gateway.register( + OAuthAccountRegistration( + email="user@example.com", + name="User", + language="en-US", + timezone="Asia/Singapore", + ip_address="203.0.113.10", + ) + ) + + assert calls == [("user@example.com", "User", "en-US", "Asia/Singapore", "203.0.113.10", True)] + with sqlite_session_factory() as session: + account = session.get(Account, account_id) + assert account is not None + assert account.status == AccountStatus.ACTIVE + assert account.initialized_at is not None + assert session.scalar(select(func.count()).select_from(Tenant)) == 0 + assert session.scalar(select(func.count()).select_from(TenantAccountJoin)) == 0 + + +def test_workspace_gateway_maps_workspace_quota_failure( + sqlite_session_factory: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, +) -> None: + with sqlite_session_factory.begin() as session: + account = Account(name="User", email="user@example.com") + session.add(account) + session.flush() + account_id = account.id + + def raise_workspace_limit(account: Account, *, session: Session) -> None: + assert account.id == account_id + assert session.get(Account, account_id) is account + raise WorkspacesLimitExceededError + + monkeypatch.setattr(TenantService, "create_owner_tenant", raise_workspace_limit) + gateway = AccountServiceOAuthWorkspaceGateway(session_factory=sqlite_session_factory) + + with pytest.raises(OAuthWorkspaceCreationNotAllowedError): + gateway.create_owner_workspace(account_id) + + +def test_provider_gateway_adapts_authorization_and_identity_contracts() -> None: + client = StubOAuthClient() + gateway = DifyOAuthProviderGateway(provider_name="github", client=client) + request = OAuthAuthorizationRequest( + invite_token="invite", + timezone="Asia/Shanghai", + language="zh-Hans", + redirect_url="/apps", + ) + + authorization_url = gateway.get_authorization_url(request) + identity = gateway.get_identity("authorization-code") + + assert authorization_url == "https://provider.example/authorize" + assert client.authorization_args == ("invite", "Asia/Shanghai", "zh-Hans", "/apps") + assert client.access_codes == ["authorization-code"] + assert client.user_tokens == ["provider-token"] + assert identity == OAuthIdentity("provider-user", "User", "user@example.com") + + +def test_provider_gateway_translates_transport_failure() -> None: + client = StubOAuthClient() + client.failure = httpx.ConnectError("provider unavailable") + gateway = DifyOAuthProviderGateway(provider_name="github", client=client) + + with pytest.raises(OAuthProviderRequestError) as raised: + gateway.get_identity("authorization-code") + + assert raised.value.__cause__ is client.failure + + +def test_provider_gateway_translates_provider_rejection() -> None: + client = StubOAuthClient() + client.failure = ValueError("invalid authorization code") + gateway = DifyOAuthProviderGateway(provider_name="github", client=client) + + with pytest.raises(OAuthProviderAuthorizationError) as raised: + gateway.get_identity("authorization-code") + + assert raised.value.description == "invalid authorization code" + assert raised.value.__cause__ is client.failure diff --git a/api/tests/unit_tests/services/test_account_oauth_service.py b/api/tests/unit_tests/services/test_account_oauth_service.py new file mode 100644 index 00000000000..d2231064107 --- /dev/null +++ b/api/tests/unit_tests/services/test_account_oauth_service.py @@ -0,0 +1,762 @@ +from _thread import LockType +from collections.abc import Callable, Generator +from concurrent.futures import ThreadPoolExecutor +from contextlib import contextmanager +from dataclasses import dataclass, field +from datetime import datetime +from threading import Barrier, Lock +from typing import NoReturn + +import pytest + +from services.account_errors import ( + AccountEmailDomainSuspendedError, + AccountEmailFrozenError, + InvalidOAuthInvitationError, + InvalidOAuthProviderError, + OAuthAccountBannedError, + OAuthIdentityLockUnavailableError, + OAuthInvitationAccountMismatchError, + OAuthRegistrationError, + OAuthWorkspaceCreationNotAllowedError, +) +from services.account_oauth_service import AccountOAuthService +from services.entities.account_entities import AccountSessionTokens, AccountSnapshot +from services.entities.account_oauth_entities import ( + OAuthAccountRegistration, + OAuthAuthorizationRequest, + OAuthCallbackCommand, + OAuthIdentity, + OAuthInvitation, + OAuthInvitationResult, + OAuthSignInResult, +) + +NOW = datetime(2026, 8, 24, 12, 0) + + +def _account( + *, + account_id: str = "account-1", + email: str = "user@example.com", + status: str = "active", +) -> AccountSnapshot: + return AccountSnapshot( + id=account_id, + name="User", + email=email, + avatar=None, + is_password_set=False, + interface_language="en-US", + interface_theme="light", + timezone="UTC", + last_login_at=None, + last_login_ip=None, + status=status, + initialized_at=None, + created_at=NOW, + ) + + +class FakeProvider: + def __init__(self, identity: OAuthIdentity | None = None) -> None: + self.identity = identity or OAuthIdentity(id="provider-user", name="User", email="user@example.com") + self.authorization_requests: list[OAuthAuthorizationRequest] = [] + self.codes: list[str] = [] + self.identity_hook: Callable[[], None] | None = None + + def get_authorization_url(self, request: OAuthAuthorizationRequest) -> str: + self.authorization_requests.append(request) + return "https://provider.example/authorize" + + def get_identity(self, code: str) -> OAuthIdentity: + self.codes.append(code) + if self.identity_hook is not None: + self.identity_hook() + return self.identity + + +@dataclass +class FakeAccounts: + email_account: AccountSnapshot | None = None + stored: dict[str, AccountSnapshot] = field(default_factory=dict) + get_calls: list[str] = field(default_factory=list) + email_lookups: list[str] = field(default_factory=list) + activations: list[tuple[str, datetime]] = field(default_factory=list) + + def get(self, account_id: str) -> AccountSnapshot | None: + self.get_calls.append(account_id) + return self.stored.get(account_id) + + def find_by_email(self, email: str) -> AccountSnapshot | None: + self.email_lookups.append(email) + return self.email_account + + def activate_pending(self, account_id: str, *, initialized_at: datetime) -> None: + self.activations.append((account_id, initialized_at)) + + def get_credentials(self, account_id: str) -> NoReturn: + raise AssertionError(account_id) + + def update_profile(self, account_id: str, changes: object) -> NoReturn: + raise AssertionError((account_id, changes)) + + def update_password(self, account_id: str, password: object) -> NoReturn: + raise AssertionError((account_id, password)) + + def initialize( + self, + account_id: str, + initialization: object, + *, + invitation_code: str | None, + workspace_id: str | None, + ) -> NoReturn: + raise AssertionError((account_id, initialization, invitation_code, workspace_id)) + + def email_exists(self, email: str) -> bool: + raise AssertionError(email) + + def reset_email(self, account_id: str, *, expected_old_email: str, new_email: str) -> NoReturn: + raise AssertionError((account_id, expected_old_email, new_email)) + + +@dataclass +class FakeIntegrations: + accounts: FakeAccounts + account_ids_by_identity: dict[tuple[str, str], str] = field(default_factory=dict) + identity_lookups: list[tuple[str, str]] = field(default_factory=list) + links: list[tuple[str, str, str]] = field(default_factory=list) + + def find_account_id(self, *, provider: str, open_id: str) -> str | None: + self.identity_lookups.append((provider, open_id)) + return self.account_ids_by_identity.get((provider, open_id)) + + def list_for_account(self, account_id: str) -> NoReturn: + raise AssertionError(account_id) + + def link(self, account_id: str, *, provider: str, open_id: str) -> None: + self.links.append((account_id, provider, open_id)) + self.account_ids_by_identity[(provider, open_id)] = account_id + account = self.accounts.get(account_id) or self.accounts.email_account + if account is not None: + self.accounts.email_account = account + + +@dataclass +class FakeAccountClaimLease: + lost: bool = False + checks: int = 0 + + def ensure_owned(self) -> None: + self.checks += 1 + if self.lost: + raise OAuthIdentityLockUnavailableError + + +@dataclass +class FakeAccountClaims: + claims: list[tuple[str, str, str]] = field(default_factory=list) + account_ids: list[str] = field(default_factory=list) + identity_leases: list[FakeAccountClaimLease] = field(default_factory=list) + account_leases: list[FakeAccountClaimLease] = field(default_factory=list) + lose_identity_on_acquire: bool = False + _locks: dict[str, LockType] = field(default_factory=dict, repr=False) + _registry_lock: LockType = field(default_factory=Lock, repr=False) + + @contextmanager + def acquire(self, *, provider: str, open_id: str, email: str) -> Generator[FakeAccountClaimLease, None, None]: + self.claims.append((provider, open_id, email)) + lease = FakeAccountClaimLease(lost=self.lose_identity_on_acquire) + self.identity_leases.append(lease) + with self._acquire_keys((f"email:{email}", f"identity:{provider}:{open_id}")): + yield lease + lease.ensure_owned() + + @contextmanager + def acquire_account(self, account_id: str) -> Generator[FakeAccountClaimLease, None, None]: + self.account_ids.append(account_id) + lease = FakeAccountClaimLease() + self.account_leases.append(lease) + with self._acquire_keys((f"account:{account_id}",)): + yield lease + lease.ensure_owned() + + @contextmanager + def _acquire_keys(self, keys: tuple[str, ...]) -> Generator[None, None, None]: + with self._registry_lock: + locks = [self._locks.setdefault(key, Lock()) for key in sorted(keys)] + for lock in locks: + lock.acquire() + try: + yield + finally: + for lock in reversed(locks): + lock.release() + + +@dataclass +class FakeMemberships: + workspace_ids: tuple[str, ...] = ("workspace-1",) + account_ids: list[str] = field(default_factory=list) + check_hook: Callable[[], None] | None = None + + def list_ids_for_account(self, account_id: str) -> tuple[str, ...]: + self.account_ids.append(account_id) + return self.workspace_ids + + def has_active_membership(self, account_id: str) -> bool: + self.account_ids.append(account_id) + if self.check_hook is not None: + self.check_hook() + return bool(self.workspace_ids) + + +@dataclass +class FakeInvitations: + invitation: OAuthInvitation | None = None + resolutions: list[str] = field(default_factory=list) + + def resolve(self, invite_token: str) -> OAuthInvitation | None: + self.resolutions.append(invite_token) + return self.invitation + + +@dataclass +class FakeRegistration: + account_id: str = "new-account" + registrations: list[OAuthAccountRegistration] = field(default_factory=list) + registration_hook: Callable[[], None] | None = None + + def register(self, registration: OAuthAccountRegistration) -> str: + self.registrations.append(registration) + if self.registration_hook is not None: + self.registration_hook() + return self.account_id + + +@dataclass +class FakeRuntime: + memberships: FakeMemberships + integrations: FakeIntegrations + created_accounts: list[str] = field(default_factory=list) + default_workspace_accounts: list[str] = field(default_factory=list) + workspace_operations: list[tuple[str, str]] = field(default_factory=list) + logins: list[tuple[str, str]] = field(default_factory=list) + default_workspace_id: str | None = None + workspace_creation_error: Exception | None = None + + def create_owner_workspace(self, account_id: str) -> None: + self._assert_identity_linked(account_id) + if self.workspace_creation_error is not None: + raise self.workspace_creation_error + self.created_accounts.append(account_id) + self.workspace_operations.append(("owner", account_id)) + self.memberships.workspace_ids = (*self.memberships.workspace_ids, f"owner-{account_id}") + + def try_join_default_workspace(self, account_id: str) -> None: + self._assert_identity_linked(account_id) + self.default_workspace_accounts.append(account_id) + self.workspace_operations.append(("default", account_id)) + if self.default_workspace_id is not None: + self.memberships.workspace_ids = (*self.memberships.workspace_ids, self.default_workspace_id) + + def login(self, account_id: str, *, ip_address: str) -> AccountSessionTokens: + self.logins.append((account_id, ip_address)) + return AccountSessionTokens("access", "refresh", "csrf") + + def _assert_identity_linked(self, account_id: str) -> None: + if not any(linked_account_id == account_id for linked_account_id, _, _ in self.integrations.links): + raise AssertionError(f"workspace provisioning preceded identity link for {account_id}") + + +@dataclass +class FakePolicy: + registration_allowed: bool = True + creation_allowed: bool = True + freeze_type: str | None = None + freeze_lookups: list[str] = field(default_factory=list) + + def is_registration_allowed(self) -> bool: + return self.registration_allowed + + def get_freeze_type(self, email: str) -> str | None: + self.freeze_lookups.append(email) + return self.freeze_type + + def is_creation_allowed(self) -> bool: + return self.creation_allowed + + +@dataclass +class Harness: + service: AccountOAuthService + provider: FakeProvider + providers: dict[str, FakeProvider] + accounts: FakeAccounts + integrations: FakeIntegrations + account_claims: FakeAccountClaims + memberships: FakeMemberships + invitations: FakeInvitations + registration: FakeRegistration + runtime: FakeRuntime + policy: FakePolicy + + +def _harness( + *, + identity: OAuthIdentity | None = None, + additional_identities: dict[str, OAuthIdentity] | None = None, +) -> Harness: + provider = FakeProvider(identity) + providers = {"github": provider} + providers.update( + {name: FakeProvider(additional_identity) for name, additional_identity in (additional_identities or {}).items()} + ) + accounts = FakeAccounts() + integrations = FakeIntegrations(accounts=accounts) + account_claims = FakeAccountClaims() + memberships = FakeMemberships() + invitations = FakeInvitations() + registration = FakeRegistration() + runtime = FakeRuntime(memberships=memberships, integrations=integrations) + policy = FakePolicy() + service = AccountOAuthService( + providers=providers, + accounts=accounts, + integrations=integrations, + memberships=memberships, + invitations=invitations, + account_claims=account_claims, + registration=registration, + workspaces=runtime, + sessions=runtime, + registration_policy=policy, + workspace_policy=policy, + supported_languages=("en-US", "zh-Hans"), + now=lambda: NOW, + ) + return Harness( + service=service, + provider=provider, + providers=providers, + accounts=accounts, + integrations=integrations, + account_claims=account_claims, + memberships=memberships, + invitations=invitations, + registration=registration, + runtime=runtime, + policy=policy, + ) + + +def _bind_identity( + harness: Harness, + account: AccountSnapshot, + *, + provider: str = "github", + open_id: str = "provider-user", +) -> None: + harness.accounts.stored[account.id] = account + harness.integrations.account_ids_by_identity[(provider, open_id)] = account.id + + +def _command(**overrides: object) -> OAuthCallbackCommand: + values: dict[str, object] = { + "provider": "github", + "code": "code-1", + "invite_token": None, + "timezone": None, + "language": None, + "browser_language": "en-US", + "ip_address": "203.0.113.10", + } + values.update(overrides) + return OAuthCallbackCommand(**values) # type: ignore[arg-type] + + +def test_start_authorization_delegates_to_configured_provider() -> None: + harness = _harness() + request = OAuthAuthorizationRequest(invite_token="invite", timezone="Asia/Shanghai") + + result = harness.service.start_authorization("github", request) + + assert result == "https://provider.example/authorize" + assert harness.provider.authorization_requests == [request] + + +def test_unknown_provider_is_rejected_before_any_account_work() -> None: + harness = _harness() + + with pytest.raises(InvalidOAuthProviderError): + harness.service.complete_authorization(_command(provider="unknown")) + + assert harness.integrations.identity_lookups == [] + + +def test_existing_account_login_uses_repositories_and_runtime_gateways() -> None: + harness = _harness() + _bind_identity(harness, _account()) + + result = harness.service.complete_authorization(_command()) + + assert isinstance(result, OAuthSignInResult) + assert result.oauth_new_user is False + assert harness.integrations.identity_lookups == [("github", "provider-user")] + assert harness.accounts.get_calls[0] == "account-1" + assert harness.accounts.email_lookups == [] + assert harness.integrations.links == [("account-1", "github", "provider-user")] + assert harness.runtime.created_accounts == [] + assert harness.runtime.logins == [("account-1", "203.0.113.10")] + assert harness.registration.registrations == [] + + +def test_existing_account_without_workspace_obeys_creation_policy() -> None: + harness = _harness() + harness.accounts.email_account = _account() + harness.memberships.workspace_ids = () + harness.policy.creation_allowed = False + + with pytest.raises(OAuthWorkspaceCreationNotAllowedError): + harness.service.complete_authorization(_command()) + + assert harness.integrations.links == [("account-1", "github", "provider-user")] + assert harness.runtime.created_accounts == [] + + +def test_existing_account_without_active_workspace_creates_owner_workspace() -> None: + harness = _harness() + harness.accounts.email_account = _account() + harness.memberships.workspace_ids = () + + harness.service.complete_authorization(_command()) + + assert harness.runtime.created_accounts == ["account-1"] + + +def test_new_account_registration_normalizes_email_and_prefers_state_language() -> None: + identity = OAuthIdentity(id="provider-user", name="", email="User@Example.com") + harness = _harness(identity=identity) + harness.accounts.stored["new-account"] = _account(account_id="new-account", email="user@example.com") + harness.memberships.workspace_ids = () + harness.runtime.default_workspace_id = "enterprise-default" + + result = harness.service.complete_authorization( + _command(language="zh-Hans", browser_language="en-US", timezone="Asia/Shanghai") + ) + + assert isinstance(result, OAuthSignInResult) + assert result.oauth_new_user is True + assert harness.registration.registrations == [ + OAuthAccountRegistration( + email="user@example.com", + name="Dify", + language="zh-Hans", + timezone="Asia/Shanghai", + ip_address="203.0.113.10", + ) + ] + assert harness.memberships.account_ids == ["new-account"] + assert harness.runtime.created_accounts == ["new-account"] + assert harness.runtime.default_workspace_accounts == ["new-account"] + assert harness.runtime.workspace_operations == [("owner", "new-account"), ("default", "new-account")] + assert harness.memberships.workspace_ids == ("owner-new-account", "enterprise-default") + assert harness.integrations.links == [("new-account", "github", "provider-user")] + + +def test_new_account_workspace_provisioning_obeys_the_same_policy_as_existing_accounts() -> None: + harness = _harness() + harness.accounts.stored["new-account"] = _account(account_id="new-account") + harness.memberships.workspace_ids = () + harness.policy.creation_allowed = False + + with pytest.raises(OAuthWorkspaceCreationNotAllowedError): + harness.service.complete_authorization(_command()) + + assert len(harness.registration.registrations) == 1 + assert harness.runtime.created_accounts == [] + assert harness.runtime.default_workspace_accounts == ["new-account"] + assert harness.integrations.links == [("new-account", "github", "provider-user")] + + +def test_new_account_uses_default_workspace_fallback_when_creation_is_disallowed() -> None: + harness = _harness() + harness.accounts.stored["new-account"] = _account(account_id="new-account") + harness.memberships.workspace_ids = () + harness.runtime.default_workspace_id = "enterprise-default" + harness.policy.creation_allowed = False + + result = harness.service.complete_authorization(_command()) + + assert isinstance(result, OAuthSignInResult) + assert result.oauth_new_user is True + assert harness.runtime.default_workspace_accounts == ["new-account"] + assert harness.memberships.account_ids == ["new-account", "new-account"] + assert harness.memberships.workspace_ids == ("enterprise-default",) + assert harness.runtime.created_accounts == [] + assert harness.runtime.workspace_operations == [("default", "new-account")] + + +def test_new_account_default_workspace_membership_bypasses_personal_workspace_quota() -> None: + harness = _harness() + harness.accounts.stored["new-account"] = _account(account_id="new-account") + harness.memberships.workspace_ids = () + harness.runtime.default_workspace_id = "enterprise-default" + harness.runtime.workspace_creation_error = OAuthWorkspaceCreationNotAllowedError() + + harness.service.complete_authorization(_command()) + + assert harness.runtime.default_workspace_accounts == ["new-account"] + assert harness.memberships.workspace_ids == ("enterprise-default",) + assert harness.runtime.created_accounts == [] + assert harness.runtime.workspace_operations == [("default", "new-account")] + + +def test_concurrent_callbacks_claim_identity_before_creating_account_or_workspace() -> None: + harness = _harness() + harness.accounts.stored["new-account"] = _account(account_id="new-account") + harness.memberships.workspace_ids = () + identity_resolved = Barrier(2) + + def synchronize_callbacks() -> None: + identity_resolved.wait(timeout=5) + + harness.provider.identity_hook = synchronize_callbacks + with ThreadPoolExecutor(max_workers=2) as executor: + futures = [executor.submit(harness.service.complete_authorization, _command()) for _ in range(2)] + results = [future.result(timeout=5) for future in futures] + + assert len(harness.registration.registrations) == 1 + assert harness.account_claims.claims == [ + ("github", "provider-user", "user@example.com"), + ("github", "provider-user", "user@example.com"), + ] + assert harness.integrations.links == [ + ("new-account", "github", "provider-user"), + ("new-account", "github", "provider-user"), + ] + assert harness.runtime.default_workspace_accounts == ["new-account"] + assert harness.runtime.created_accounts == ["new-account"] + assert sorted(result.oauth_new_user for result in results if isinstance(result, OAuthSignInResult)) == [False, True] + + +def test_concurrent_provider_callbacks_claim_normalized_email_before_registration() -> None: + harness = _harness( + identity=OAuthIdentity("github-user", "User", "Shared.User+github@GoogleMail.com"), + additional_identities={"google": OAuthIdentity("google-user", "User", "shareduser@gmail.COM")}, + ) + harness.accounts.stored["new-account"] = _account(account_id="new-account", email="shared@example.com") + harness.memberships.workspace_ids = () + identity_resolved = Barrier(2) + + def synchronize_callbacks() -> None: + identity_resolved.wait(timeout=5) + + harness.providers["github"].identity_hook = synchronize_callbacks + harness.providers["google"].identity_hook = synchronize_callbacks + commands = [ + _command(provider="github"), + _command(provider="google"), + ] + with ThreadPoolExecutor(max_workers=2) as executor: + futures = [executor.submit(harness.service.complete_authorization, command) for command in commands] + results = [future.result(timeout=5) for future in futures] + + assert len(harness.registration.registrations) == 1 + assert sorted(harness.account_claims.claims) == [ + ("github", "github-user", "shareduser@gmail.com"), + ("google", "google-user", "shareduser@gmail.com"), + ] + assert sorted(harness.integrations.links) == [ + ("new-account", "github", "github-user"), + ("new-account", "google", "google-user"), + ] + assert harness.runtime.default_workspace_accounts == ["new-account"] + assert harness.runtime.created_accounts == ["new-account"] + assert sorted(result.oauth_new_user for result in results if isinstance(result, OAuthSignInResult)) == [False, True] + + +def test_concurrent_provider_callbacks_for_one_account_serialize_workspace_provisioning() -> None: + harness = _harness( + identity=OAuthIdentity("github-user", "User", "github@example.com"), + additional_identities={"google": OAuthIdentity("google-user", "User", "google@example.com")}, + ) + account = _account(email="primary@example.com") + _bind_identity(harness, account, provider="github", open_id="github-user") + _bind_identity(harness, account, provider="google", open_id="google-user") + harness.memberships.workspace_ids = () + identity_resolved = Barrier(2) + + def synchronize_callbacks() -> None: + identity_resolved.wait(timeout=5) + + harness.providers["github"].identity_hook = synchronize_callbacks + harness.providers["google"].identity_hook = synchronize_callbacks + commands = [ + _command(provider="github"), + _command(provider="google"), + ] + with ThreadPoolExecutor(max_workers=2) as executor: + futures = [executor.submit(harness.service.complete_authorization, command) for command in commands] + results = [future.result(timeout=5) for future in futures] + + assert harness.registration.registrations == [] + assert sorted(harness.account_claims.claims) == [ + ("github", "github-user", "github@example.com"), + ("google", "google-user", "google@example.com"), + ] + assert harness.account_claims.account_ids == ["account-1", "account-1"] + assert harness.runtime.created_accounts == ["account-1"] + assert all(isinstance(result, OAuthSignInResult) and not result.oauth_new_user for result in results) + + +def test_lost_identity_claim_stops_before_registration() -> None: + harness = _harness() + harness.account_claims.lose_identity_on_acquire = True + + with pytest.raises(OAuthIdentityLockUnavailableError): + harness.service.complete_authorization(_command()) + + assert harness.registration.registrations == [] + assert harness.integrations.links == [] + assert harness.runtime.created_accounts == [] + + +def test_identity_claim_lost_during_registration_stops_follow_up_writes() -> None: + harness = _harness() + harness.accounts.stored["new-account"] = _account(account_id="new-account") + + def lose_identity_claim() -> None: + harness.account_claims.identity_leases[0].lost = True + + harness.registration.registration_hook = lose_identity_claim + + with pytest.raises(OAuthIdentityLockUnavailableError): + harness.service.complete_authorization(_command()) + + assert len(harness.registration.registrations) == 1 + assert harness.integrations.links == [] + assert harness.runtime.default_workspace_accounts == [] + assert harness.runtime.created_accounts == [] + + +def test_lost_account_claim_stops_before_workspace_creation() -> None: + harness = _harness() + _bind_identity(harness, _account()) + harness.memberships.workspace_ids = () + + def lose_account_claim() -> None: + harness.account_claims.account_leases[0].lost = True + + harness.memberships.check_hook = lose_account_claim + + with pytest.raises(OAuthIdentityLockUnavailableError): + harness.service.complete_authorization(_command()) + + assert harness.runtime.created_accounts == [] + assert harness.runtime.logins == [] + + +@pytest.mark.parametrize( + ("freeze_type", "expected_error"), + [ + ("email_domain_suspended", AccountEmailDomainSuspendedError), + ("freeze", AccountEmailFrozenError), + (None, OAuthRegistrationError), + ], +) +def test_disabled_registration_applies_account_policy( + freeze_type: str | None, + expected_error: type[Exception], +) -> None: + harness = _harness() + harness.policy.registration_allowed = False + harness.policy.freeze_type = freeze_type + + with pytest.raises(expected_error): + harness.service.complete_authorization(_command()) + + assert harness.policy.freeze_lookups == ["user@example.com"] + assert harness.registration.registrations == [] + + +def test_pending_account_is_activated_through_repository() -> None: + harness = _harness() + _bind_identity(harness, _account(status="pending")) + + harness.service.complete_authorization(_command()) + + assert harness.accounts.activations == [("account-1", NOW)] + + +def test_pending_account_is_not_activated_when_workspace_creation_is_disallowed() -> None: + harness = _harness() + _bind_identity(harness, _account(status="pending")) + harness.memberships.workspace_ids = () + harness.policy.creation_allowed = False + + with pytest.raises(OAuthWorkspaceCreationNotAllowedError): + harness.service.complete_authorization(_command()) + + assert harness.accounts.activations == [] + assert harness.runtime.logins == [] + + +def test_pending_account_is_not_activated_when_workspace_creation_fails() -> None: + harness = _harness() + _bind_identity(harness, _account(status="pending")) + harness.memberships.workspace_ids = () + harness.runtime.workspace_creation_error = RuntimeError("workspace quota exceeded") + + with pytest.raises(RuntimeError, match="workspace quota exceeded"): + harness.service.complete_authorization(_command()) + + assert harness.accounts.activations == [] + assert harness.runtime.logins == [] + + +def test_valid_invitation_links_and_logs_in_invited_account() -> None: + harness = _harness(identity=OAuthIdentity("provider-user", "User", "Invitee@Example.com")) + harness.invitations.invitation = OAuthInvitation("invited-account", "invitee@example.com", "active") + + result = harness.service.complete_authorization(_command(invite_token="invite-token")) + + assert isinstance(result, OAuthInvitationResult) + assert result.invite_token == "invite-token" + assert harness.integrations.links == [("invited-account", "github", "provider-user")] + assert harness.runtime.logins == [("invited-account", "203.0.113.10")] + assert harness.integrations.identity_lookups == [] + assert harness.invitations.resolutions == ["invite-token"] + + +def test_resolvable_invitation_requires_matching_email() -> None: + harness = _harness() + harness.invitations.invitation = OAuthInvitation("invited-account", "other@example.com", "active") + + with pytest.raises(OAuthInvitationAccountMismatchError) as raised: + harness.service.complete_authorization(_command(invite_token="invite-token")) + + assert raised.value.invite_token == "invite-token" + assert harness.integrations.links == [] + + +def test_stale_invitation_is_rejected() -> None: + harness = _harness() + + with pytest.raises(InvalidOAuthInvitationError): + harness.service.complete_authorization(_command(invite_token="invite-token")) + + assert harness.invitations.resolutions == ["invite-token"] + assert harness.integrations.identity_lookups == [] + assert harness.registration.registrations == [] + + +def test_banned_account_is_rejected_before_writes() -> None: + harness = _harness() + _bind_identity(harness, _account(status="banned")) + + with pytest.raises(OAuthAccountBannedError): + harness.service.complete_authorization(_command()) + + assert harness.integrations.links == [] diff --git a/api/tests/unit_tests/services/test_account_service.py b/api/tests/unit_tests/services/test_account_service.py index aad8e9d05c8..64ffe961cd6 100644 --- a/api/tests/unit_tests/services/test_account_service.py +++ b/api/tests/unit_tests/services/test_account_service.py @@ -1998,59 +1998,6 @@ class TestRegisterService: mock_join_default_workspace.assert_called_once_with(mock_account.id) - def test_register_with_oauth( - self, sqlite_session: Session, mock_external_service_dependencies: _MockDependencies - ) -> None: - """Test account registration with OAuth integration.""" - # Setup mocks - mock_external_service_dependencies["feature_service"].is_registration_allowed.return_value = True - mock_external_service_dependencies["feature_service"].is_workspace_creation_allowed.return_value = True - mock_external_service_dependencies[ - "feature_service" - ].get_license.return_value.workspaces.is_available.return_value = True - mock_external_service_dependencies["billing_service"].is_email_in_freeze.return_value = False - - # Mock AccountService.create_account and link_account_integrate - mock_account = TestAccountAssociatedDataFactory.create_account_mock() - with ( - patch("services.account_service.AccountService.create_account") as mock_create_account, - patch("services.account_service.AccountService.link_account_integrate") as mock_link_account, - ): - mock_create_account.return_value = mock_account - - # Mock TenantService methods - with ( - patch("services.account_service.TenantService.create_tenant") as mock_create_tenant, - patch("services.account_service.TenantService.create_tenant_member") as mock_create_member, - patch("services.account_service.tenant_was_created") as mock_event, - ): - mock_tenant = Tenant(name="Test User's Workspace") - sqlite_session.add(mock_tenant) - sqlite_session.flush() - mock_create_tenant.return_value = mock_tenant - mock_create_member.side_effect = lambda tenant, account, session, role: session.add( - TenantAccountJoin( - tenant_id=tenant.id, - account_id=account.id, - role=TenantAccountRole(role), - ) - ) - - # Execute test - result = RegisterService.register( - email="test@example.com", - name="Test User", - password=None, - open_id="oauth123", - provider="google", - language="en-US", - session=sqlite_session, - ) - - # Verify results - assert result == mock_account - mock_link_account.assert_called_once_with("google", "oauth123", mock_account, session=sqlite_session) - def test_register_with_pending_status( self, sqlite_session: Session, mock_external_service_dependencies: _MockDependencies ) -> None: @@ -2664,30 +2611,6 @@ class TestRegisterService: assert stored_data["role"] == "admin" assert stored_data["requires_setup"] is True - def test_is_valid_invite_token_valid(self, mock_redis_dependencies: MagicMock) -> None: - """Test checking valid invite token.""" - # Setup mock - mock_redis_dependencies.get.return_value = b'{"test": "data"}' - - # Execute test - result = RegisterService.is_valid_invite_token("valid-token") - - # Verify results - assert result is True - mock_redis_dependencies.get.assert_called_once_with("member_invite:token:valid-token") - - def test_is_valid_invite_token_invalid(self, mock_redis_dependencies: MagicMock) -> None: - """Test checking invalid invite token.""" - # Setup mock - mock_redis_dependencies.get.return_value = None - - # Execute test - result = RegisterService.is_valid_invite_token("invalid-token") - - # Verify results - assert result is False - mock_redis_dependencies.get.assert_called_once_with("member_invite:token:invalid-token") - def test_revoke_token_with_workspace_and_email(self, mock_redis_dependencies: MagicMock) -> None: """Test revoking token with workspace ID and email.""" # Execute test From a702b3bb2baa99ba88ef753448d52b3805030b9c Mon Sep 17 00:00:00 2001 From: zyssyz123 <916125788@qq.com> Date: Thu, 3 Sep 2026 08:53:55 +0000 Subject: [PATCH 02/50] fix(agent): surface missing tool credentials (#41726) --- api/core/tools/tool_manager.py | 44 +++++++--- .../core/tools/test_tool_manager.py | 86 ++++++++++++++++++- .../nodes/agent_v2/test_dify_tools_builder.py | 1 + .../__tests__/authorized-in-node.spec.tsx | 18 ++++ .../plugin-auth/authorized-in-node.tsx | 7 +- 5 files changed, 139 insertions(+), 17 deletions(-) diff --git a/api/core/tools/tool_manager.py b/api/core/tools/tool_manager.py index 0f33b104914..fc02ecaf259 100644 --- a/api/core/tools/tool_manager.py +++ b/api/core/tools/tool_manager.py @@ -42,7 +42,7 @@ from core.tools.entities.tool_entities import ( ToolProviderType, emoji_icon_adapter, ) -from core.tools.errors import ToolProviderNotFoundError +from core.tools.errors import ToolProviderCredentialValidationError, ToolProviderNotFoundError from core.tools.mcp_tool.provider import MCPToolProviderController from core.tools.mcp_tool.tool import MCPTool from core.tools.plugin_tool.provider import PluginToolProviderController @@ -233,7 +233,10 @@ class ToolManager: builtin_provider = None logger.info("Error getting builtin provider %s:%s", credential_id, e, exc_info=True) if builtin_provider is None: - raise ToolProviderNotFoundError(f"provider has been deleted: {credential_id}") + raise ToolProviderCredentialValidationError( + f"Tool credential {credential_id} has been deleted. " + "Select or authorize another credential." + ) if builtin_provider is None: with Session(db.engine) as session: @@ -247,7 +250,10 @@ class ToolManager: .order_by(BuiltinToolProvider.is_default.desc(), BuiltinToolProvider.created_at.asc()) ) if builtin_provider is None: - raise ToolProviderNotFoundError(f"no default provider for {provider_id}") + raise ToolProviderCredentialValidationError( + f"No workspace credential is configured for tool provider {provider_id}. " + "Authorize the provider or select a credential." + ) else: builtin_provider = db.session.scalar( select(BuiltinToolProvider) @@ -259,7 +265,10 @@ class ToolManager: ) if builtin_provider is None: - raise ToolProviderNotFoundError(f"builtin provider {provider_id} not found") + raise ToolProviderCredentialValidationError( + f"No credential is configured for built-in tool provider {provider_id}. " + "Authorize the provider or select a credential." + ) from core.helper.credential_utils import runtime_check_credential_policy_compliance @@ -294,15 +303,24 @@ class ToolManager: system_credentials = BuiltinToolManageService.get_oauth_client(tenant_id, provider_id) oauth_handler = OAuthHandler() - refreshed_credentials = oauth_handler.refresh_credentials( - tenant_id=tenant_id, - user_id=builtin_provider.user_id, - plugin_id=tool_provider.plugin_id, - provider=provider_name, - redirect_uri=redirect_uri, - system_credentials=system_credentials or {}, - credentials=decrypted_credentials, - ) + try: + refreshed_credentials = oauth_handler.refresh_credentials( + tenant_id=tenant_id, + user_id=builtin_provider.user_id, + plugin_id=tool_provider.plugin_id, + provider=provider_name, + redirect_uri=redirect_uri, + system_credentials=system_credentials or {}, + credentials=decrypted_credentials, + ) + except Exception as exc: + logger.warning( + "Failed to refresh OAuth credentials for tool provider %s", provider_id, exc_info=True + ) + raise ToolProviderCredentialValidationError( + f"OAuth credential for tool provider {provider_id} could not be refreshed. " + "Reauthorize or select another credential." + ) from exc # update the credentials builtin_provider.encrypted_credentials = json.dumps( encrypter.encrypt(refreshed_credentials.credentials) diff --git a/api/tests/unit_tests/core/tools/test_tool_manager.py b/api/tests/unit_tests/core/tools/test_tool_manager.py index a299fe465dc..eeac6f35365 100644 --- a/api/tests/unit_tests/core/tools/test_tool_manager.py +++ b/api/tests/unit_tests/core/tools/test_tool_manager.py @@ -25,7 +25,7 @@ from core.tools.entities.tool_entities import ( ToolParameter, ToolProviderType, ) -from core.tools.errors import ToolProviderNotFoundError +from core.tools.errors import ToolProviderCredentialValidationError, ToolProviderNotFoundError from core.tools.plugin_tool.provider import PluginToolProviderController from core.tools.tool_manager import ToolManager from models.base import TypeBase @@ -399,7 +399,49 @@ def test_get_tool_runtime_builtin_refreshes_expired_oauth_credentials( cache.delete.assert_called_once() -def test_get_tool_runtime_builtin_plugin_provider_deleted_raises( +def test_get_tool_runtime_builtin_maps_oauth_refresh_failure_to_credential_error( + monkeypatch: pytest.MonkeyPatch, tool_database: _ToolDatabase +): + tool = Mock() + controller = SimpleNamespace( + get_tool=Mock(return_value=tool), + need_credentials=True, + get_credentials_schema_by_type=Mock(return_value=[]), + ) + tenant_id = "00000000-0000-0000-0000-000000000001" + builtin_provider = _builtin_provider( + provider_id="00000000-0000-0000-0000-000000000002", + tenant_id=tenant_id, + credential_type=CredentialType.OAUTH2, + expires_at=1, + ) + tool_database.session.add(builtin_provider) + tool_database.session.commit() + monkeypatch.setattr("core.tools.tool_manager.db", tool_database) + + encrypter = Mock() + encrypter.decrypt.return_value = {"token": "expired"} + with ( + patch.object(ToolManager, "get_builtin_provider", return_value=controller), + patch("core.tools.tool_manager.create_provider_encrypter", return_value=(encrypter, Mock())), + patch("core.tools.tool_manager.time.time", return_value=1000), + patch( + "services.tools.builtin_tools_manage_service.BuiltinToolManageService.get_oauth_client", + return_value={"client_id": "id"}, + ), + patch("core.plugin.impl.oauth.OAuthHandler") as oauth_handler_cls, + ): + oauth_handler_cls.return_value.refresh_credentials.side_effect = ValueError("refresh token revoked") + with pytest.raises(ToolProviderCredentialValidationError, match="could not be refreshed"): + ToolManager.get_tool_runtime( + provider_type=ToolProviderType.BUILT_IN, + provider_id="time", + tool_name="weekday", + tenant_id=tenant_id, + ) + + +def test_get_tool_runtime_builtin_plugin_credential_deleted_raises( monkeypatch: pytest.MonkeyPatch, tool_database: _ToolDatabase ): plugin_controller = object.__new__(PluginToolProviderController) @@ -409,7 +451,7 @@ def test_get_tool_runtime_builtin_plugin_provider_deleted_raises( monkeypatch.setattr("core.tools.tool_manager.db", tool_database) with patch.object(ToolManager, "get_builtin_provider", return_value=plugin_controller): - with pytest.raises(ToolProviderNotFoundError, match="provider has been deleted"): + with pytest.raises(ToolProviderCredentialValidationError, match="credential .* has been deleted"): ToolManager.get_tool_runtime( provider_type=ToolProviderType.BUILT_IN, provider_id="time", @@ -419,6 +461,44 @@ def test_get_tool_runtime_builtin_plugin_provider_deleted_raises( ) +def test_get_tool_runtime_builtin_plugin_without_workspace_credential_raises( + monkeypatch: pytest.MonkeyPatch, tool_database: _ToolDatabase +): + plugin_controller = object.__new__(PluginToolProviderController) + plugin_controller.entity = SimpleNamespace(credentials_schema=[{"name": "k"}], oauth_schema=None) + plugin_controller.get_tool = Mock(return_value=Mock()) + plugin_controller.get_credentials_schema_by_type = Mock(return_value=[]) + + monkeypatch.setattr("core.tools.tool_manager.db", tool_database) + with patch.object(ToolManager, "get_builtin_provider", return_value=plugin_controller): + with pytest.raises(ToolProviderCredentialValidationError, match="No workspace credential is configured"): + ToolManager.get_tool_runtime( + provider_type=ToolProviderType.BUILT_IN, + provider_id="langgenius/dify-gmail/dify-gmail", + tool_name="send_draft", + tenant_id="00000000-0000-0000-0000-000000000001", + ) + + +def test_get_tool_runtime_hardcoded_provider_without_credential_raises( + monkeypatch: pytest.MonkeyPatch, tool_database: _ToolDatabase +): + controller = SimpleNamespace( + get_tool=Mock(return_value=Mock()), + need_credentials=True, + ) + + monkeypatch.setattr("core.tools.tool_manager.db", tool_database) + with patch.object(ToolManager, "get_builtin_provider", return_value=controller): + with pytest.raises(ToolProviderCredentialValidationError, match="No credential is configured"): + ToolManager.get_tool_runtime( + provider_type=ToolProviderType.BUILT_IN, + provider_id="legacy-provider", + tool_name="legacy-tool", + tenant_id="00000000-0000-0000-0000-000000000001", + ) + + def test_get_tool_runtime_api_path(): api_tool = Mock() api_tool.fork_tool_runtime.return_value = "api-runtime" diff --git a/api/tests/unit_tests/core/workflow/nodes/agent_v2/test_dify_tools_builder.py b/api/tests/unit_tests/core/workflow/nodes/agent_v2/test_dify_tools_builder.py index f13032325ec..87ce8ae065c 100644 --- a/api/tests/unit_tests/core/workflow/nodes/agent_v2/test_dify_tools_builder.py +++ b/api/tests/unit_tests/core/workflow/nodes/agent_v2/test_dify_tools_builder.py @@ -839,6 +839,7 @@ def test_credential_validation_error_maps_to_credential_invalid(): with pytest.raises(WorkflowAgentDifyToolsBuildError) as exc_info: _build(builder, _standard_tools_payload()) assert exc_info.value.error_code == "agent_tool_credential_invalid" + assert "credential validation failed" in str(exc_info.value) def test_generic_value_error_maps_to_config_invalid(): diff --git a/web/app/components/plugins/plugin-auth/__tests__/authorized-in-node.spec.tsx b/web/app/components/plugins/plugin-auth/__tests__/authorized-in-node.spec.tsx index 153e669b95e..b0c8297af63 100644 --- a/web/app/components/plugins/plugin-auth/__tests__/authorized-in-node.spec.tsx +++ b/web/app/components/plugins/plugin-auth/__tests__/authorized-in-node.spec.tsx @@ -243,4 +243,22 @@ describe('AuthorizedInNode Component', () => { const button = screen.getByRole('button') expect(button.textContent).toContain('plugin.auth.unavailable') }) + + it('should show unavailable when workspace default credential is missing', async () => { + const AuthorizedInNode = (await import('../authorized-in-node')).default + mockGetPluginCredentialInfo.mockReturnValue({ + credentials: [], + supported_credential_types: [CredentialTypeEnum.API_KEY], + allow_custom_token: true, + }) + const pluginPayload = createPluginPayload() + + render(, { + wrapper: createWrapper(), + }) + + const button = screen.getByRole('button') + expect(button.textContent).toContain('plugin.auth.workspaceDefault') + expect(button.textContent).toContain('plugin.auth.unavailable') + }) }) diff --git a/web/app/components/plugins/plugin-auth/authorized-in-node.tsx b/web/app/components/plugins/plugin-auth/authorized-in-node.tsx index 782f920d767..7f0f084ab29 100644 --- a/web/app/components/plugins/plugin-auth/authorized-in-node.tsx +++ b/web/app/components/plugins/plugin-auth/authorized-in-node.tsx @@ -48,7 +48,12 @@ const AuthorizedInNode = ({ const defaultCredential = credentials.find((c) => c.is_default) - if (defaultCredential?.not_allowed_to_use) { + if (isLoading) { + color = 'disabled' + } else if (!defaultCredential) { + color = 'error' + defaultUnavailable = true + } else if (defaultCredential.not_allowed_to_use) { color = 'disabled' defaultUnavailable = true } From 863930836b93b313ea5199c29772ba2cd86e3d16 Mon Sep 17 00:00:00 2001 From: yyh <92089059+lyzno1@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:00:30 +0000 Subject: [PATCH 03/50] chore(dify-ui): enable purity lint in tests (#41736) --- lint.config.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lint.config.ts b/lint.config.ts index 2528c3e7c38..e0cd7283be0 100644 --- a/lint.config.ts +++ b/lint.config.ts @@ -1299,15 +1299,6 @@ export const lintConfig = { ], }, }, - { - files: [ - 'packages/dify-ui/**/__tests__/**/*.{js,cjs,mjs,jsx,ts,cts,mts,tsx}', - 'packages/dify-ui/**/*.spec.{js,cjs,mjs,jsx,ts,cts,mts,tsx}', - ], - rules: { - 'eslint-react/purity': 'off', - }, - }, { files: ['cli/bin/**'], rules: { From aee0062cb200ad6bfe49e03b39d6b62fb0e2cfa5 Mon Sep 17 00:00:00 2001 From: Joel Date: Thu, 3 Sep 2026 09:10:16 +0000 Subject: [PATCH 04/50] chore: remove redundant aria-label attributes (#41724) Co-authored-by: yyh --- .../references/accessibility-ui.md | 21 ++- packages/dify-ui/AGENTS.md | 2 + packages/dify-ui/README.md | 18 +- .../docs/accessible-names-and-descriptions.md | 175 ++++++++++++++++++ packages/dify-ui/src/button/README.md | 2 + packages/dify-ui/src/icon-button/README.md | 7 +- web/AGENTS.md | 2 + .../dataset-info/__tests__/index.spec.tsx | 4 +- .../app-sidebar/dataset-info/index.tsx | 2 +- .../model-parameter-trigger.spec.tsx | 41 +++- .../model-parameter-trigger.tsx | 7 +- .../app/overview/customize/index.tsx | 9 - web/app/components/apps/app-sort-filter.tsx | 5 +- .../__tests__/index.spec.tsx | 9 +- .../base/inline-delete-confirm/index.tsx | 21 +-- .../__tests__/indexing-progress-item.spec.tsx | 13 +- .../indexing-progress-item.tsx | 12 +- .../dataset-metadata-picker.tsx | 7 +- .../__tests__/popup-item.spec.tsx | 21 ++- .../model-selector/popup-item.tsx | 7 +- web/app/components/integrations/index.tsx | 1 - .../integrations/sidebar-actions.tsx | 9 +- .../integrations/sidebar-nav-item.tsx | 3 - .../__tests__/icon-with-tooltip.spec.tsx | 4 +- .../plugins/base/badges/icon-with-tooltip.tsx | 5 +- .../authorize/permission-selector.tsx | 1 - .../__tests__/plugin-source-badge.spec.tsx | 2 +- .../components/plugin-source-badge.tsx | 7 +- .../operation-dropdown.tsx | 1 - .../subscription-list/subscription-card.tsx | 1 - .../plugin-page/category-empty-state.tsx | 1 - .../plugin-page/install-plugin-dropdown.tsx | 1 - .../components/snippet-collapsed-preview.tsx | 13 +- web/app/components/tools/mcp/create-card.tsx | 1 - .../mcp/detail/__tests__/content.spec.tsx | 5 +- .../components/tools/mcp/detail/content.tsx | 8 +- .../tools/provider/create-entry-card.tsx | 2 - .../tools/provider/custom-create-card.tsx | 1 - web/app/components/tools/provider/detail.tsx | 1 - .../workflow/header/view-history.tsx | 1 - .../agent-orchestrate-panel-content.spec.tsx | 8 +- .../agent-output-variables/edit-card.tsx | 11 +- .../components/__tests__/model-bar.spec.tsx | 4 +- .../components/__tests__/tool-icon.spec.tsx | 4 +- .../nodes/agent/components/model-bar.tsx | 6 +- .../nodes/agent/components/tool-icon.tsx | 3 +- .../components/class-item.tsx | 1 - .../components/service-api-access-card.tsx | 1 - .../components/orchestrate/advanced/env.tsx | 3 - .../common/missing-reference-warning.tsx | 2 +- .../orchestrate/tools/provider-tool/item.tsx | 7 +- .../preview/__tests__/header.spec.tsx | 10 +- .../configure/components/preview/header.tsx | 12 +- .../preview/working-directory-panel.tsx | 7 +- .../__tests__/new-knowledge-list.spec.tsx | 2 +- .../components/knowledge-space-card.tsx | 6 +- web/features/new-rag/document-chunk-tree.tsx | 8 +- .../skills/__tests__/detail-page.spec.tsx | 6 +- web/features/skills/detail/file-tree-dnd.tsx | 6 +- .../skills/detail/skill-pdf-preview.tsx | 3 +- 60 files changed, 380 insertions(+), 183 deletions(-) create mode 100644 packages/dify-ui/docs/accessible-names-and-descriptions.md diff --git a/.agents/skills/frontend-code-review/references/accessibility-ui.md b/.agents/skills/frontend-code-review/references/accessibility-ui.md index 92764786b15..eb9cdd47728 100644 --- a/.agents/skills/frontend-code-review/references/accessibility-ui.md +++ b/.agents/skills/frontend-code-review/references/accessibility-ui.md @@ -2,6 +2,8 @@ Accessibility findings are first-class review findings. Treat broken keyboard access, missing accessible names, focus loss, and unreachable popup content as correctness bugs, not polish. +## Review Evidence + Before finalizing UI or accessibility findings, fetch the latest Web Interface Guidelines as a required baseline: ```text @@ -21,13 +23,26 @@ Flag: - Clickable `div` or `span` used for actions. - Router navigation implemented with button or `onClick` when a `Link` / `` is the real semantic element. -- Icon-only buttons without `aria-label` or `aria-labelledby`. +- Icon-only controls without an accessible name; follow the naming rules below and the Dify UI `IconButton` contract. - Decorative icons missing `aria-hidden="true"`. - Images without `alt`; use `alt=""` only when truly decorative. - Heading levels that skip hierarchy in page-level content. Prefer semantic HTML before ARIA. +## Accessible Names And Descriptions + +Read [Accessible names and descriptions] when a change affects labels, ARIA naming, help/error relationships, or hidden text. That document owns the shared implementation and review contract. + +Flag violations supported by the final rendered behavior: + +- Missing or insufficient names, redundant name overrides, or naming attributes prohibited by the element's role. +- Overrides that omit visible label wording or suppress necessary descendant information. +- Broken or stale label/description references, including relationships lost when responsive content or overlays unmount. +- Descriptions used instead of names, repeated help text, or essential structured content available only as a flattened description. + +Inspect the computed name and description in the relevant state. Matching an `aria-label` string to nearby text alone does not prove redundancy. + ## Keyboard And Focus Flag: @@ -56,7 +71,7 @@ Flag: - Placeholder text used as the only label. - Password managers accidentally triggered on non-auth fields because autocomplete is missing or wrong. -Prefer visible labels. If visible surrounding text already labels the control, use a visually hidden label or a precise `aria-label`. +Prefer visible labels and associate them through the appropriate field primitive, a native `label`, or `aria-labelledby`. Do not duplicate an existing label with hidden text or `aria-label`; follow [Accessible names and descriptions] when no suitable visible label exists. ## Disabled, Loading, And Async States @@ -107,3 +122,5 @@ Flag: - Images without dimensions. - Loading copy using `...` instead of `…`. - Hardcoded dates, times, numbers, or currency formats instead of `Intl.*`. + +[Accessible names and descriptions]: ../../../../packages/dify-ui/docs/accessible-names-and-descriptions.md diff --git a/packages/dify-ui/AGENTS.md b/packages/dify-ui/AGENTS.md index 2cd05f418f9..5e17256c562 100644 --- a/packages/dify-ui/AGENTS.md +++ b/packages/dify-ui/AGENTS.md @@ -19,6 +19,7 @@ then read only the guide for the contract being changed. - Imports, exports, naming, public types, generics, and anatomy: [Public API authoring] - Button and icon-only action behavior: [Button contract] and [Icon Button contract] +- Cross-component accessible-name and description choices: [Accessible names and descriptions] - Compound input behavior: [Input Group contract] - Form structure, labels, and value ownership: [Forms] - Picker choice and typed values: [Selection] @@ -29,6 +30,7 @@ then read only the guide for the contract being changed. A component needs a local README only when it owns a substantial Dify-specific contract that its types, stories, and upstream documentation do not express. Do not create one for completeness. +[Accessible names and descriptions]: docs/accessible-names-and-descriptions.md [Button contract]: src/button/README.md [Forms]: docs/forms.md [Icon Button contract]: src/icon-button/README.md diff --git a/packages/dify-ui/README.md b/packages/dify-ui/README.md index 622b4f556b4..645665fb89e 100644 --- a/packages/dify-ui/README.md +++ b/packages/dify-ui/README.md @@ -68,20 +68,22 @@ Upstream behavior remains owned by the [Base UI documentation]. ### Cross-component guides -| Guide | Scope | -| ------------------------- | ------------------------------------------------------------------------------ | -| [Forms] | Native submit boundaries, value ownership, fields, labels, and errors. | -| [Selection] | Typed values and choosing among segmented controls, pickers, and radio groups. | -| [Overlays] | Portals, presence lifecycles, layering, trigger composition, and semantics. | -| [Styling] | Tailwind CSS integration and the Figma radius mapping. | -| [Public API authoring] | Subpath exports, naming, public types, generics, and private helpers. | -| [Testing and development] | Package commands, test ownership, accessibility, and animation setup. | +| Guide | Scope | +| ----------------------------------- | ------------------------------------------------------------------------------ | +| [Accessible names and descriptions] | Naming sources, descriptions, overrides, and safe label removal. | +| [Forms] | Native submit boundaries, value ownership, fields, labels, and errors. | +| [Selection] | Typed values and choosing among segmented controls, pickers, and radio groups. | +| [Overlays] | Portals, presence lifecycles, layering, trigger composition, and semantics. | +| [Styling] | Tailwind CSS integration and the Figma radius mapping. | +| [Public API authoring] | Subpath exports, naming, public types, generics, and private helpers. | +| [Testing and development] | Package commands, test ownership, accessibility, and animation setup. | ## Contributing Read [component authoring rules] before modifying the package, then open only the matching owner guide. This index intentionally does not duplicate those contracts. +[Accessible names and descriptions]: ./docs/accessible-names-and-descriptions.md [Base UI documentation]: https://base-ui.com/llms.txt [Base UI]: https://base-ui.com/react [Button]: ./src/button/README.md diff --git a/packages/dify-ui/docs/accessible-names-and-descriptions.md b/packages/dify-ui/docs/accessible-names-and-descriptions.md new file mode 100644 index 00000000000..d948c3290ff --- /dev/null +++ b/packages/dify-ui/docs/accessible-names-and-descriptions.md @@ -0,0 +1,175 @@ +# Accessible Names and Descriptions + +This cross-component contract is owned by Dify UI. It applies to Dify UI primitives and to +consumers composing those primitives. It depends only on Dify UI component contracts and upstream +web standards; application packages may add localization, testing, and product-specific rules +without redefining this contract. + +[Base UI accessibility] owns the primitive mechanics it implements, such as roles, relationships, +keyboard interaction, and focus management. Dify UI and its consumers still own the final element, +label content, composition, and product meaning. Use this guide to choose those naming and +description sources. Open a component guide only when the decision reaches that component. + +Use [ARIA in HTML][html-naming] for authoring conformance and the [name and description computation +specification][accname] to understand the current computation model. APG and MDN provide authoring +guidance; Base UI documents the behavior and usage guidance of the primitives Dify wraps. The Dify +conventions below choose among valid options without making a prohibited naming relationship valid. + +## Start Here + +An accessible name is the flat string that identifies a named element to assistive technology; not +every role permits one. An accessible description adds optional help, instructions, or consequences. +State such as checked, expanded, or disabled remains separate from the name, and changing status +remains with the feature's status or live-region owner. + +For each changed element: + +1. Inspect the final rendered element, role, text, and props forwarded by its primitive. +1. Prefer meaningful visible text or a native label relationship. +1. Use `aria-labelledby` when suitable visible text exists elsewhere in the DOM. +1. Use `aria-label` only when the role permits naming and no visible text can provide the name. +1. Add `aria-describedby` only for useful supplemental text; do not repeat the name. +1. Verify the computed name and description in every changed responsive and interaction state. + +These choices follow the [W3C APG naming techniques][apg] and [MDN `aria-label` guidance]. Nearby +text is not a label relationship by proximity alone. + +## Common Decisions + +| Surface | Contract | +| -------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------- | +| Text button or link | Follow [Button]. Let meaningful child text name the action; do not repeat it in `aria-label`. | +| Form control | Follow [Forms]. Use its label primitive or an associated native `label`, preserving label activation. | +| Icon-only command | Follow [IconButton]. Its component-specific contract requires one accessible-name source and a decorative glyph. | +| Dialog or named region | Reuse the visible title through the primitive title API or `aria-labelledby`; use `aria-label` only when no suitable visible title exists. | +| Related form-control group | Follow [Forms]. Use `Fieldset` with `FieldsetLegend` and preserve each control's own label. Other composite widgets follow their owning primitive. | +| Table or figure | Prefer `caption` or `figcaption` when appropriate. See the [APG caption guidance][captions] for name and description differences. | +| Image | Supply meaningful `alt`, or `alt=""` for a decorative image. | +| Plain `div` or `span` | Keep readable content; do not add `aria-label` or `aria-labelledby` to the default `generic` role. | + +Naming permission comes from semantics, not the presence of an `aria-*` prop. Other roles also +prohibit naming. Do not invent a role merely to permit a label. A plain span may contain text +referenced by another element's `aria-labelledby`; that relationship names the referencing element, +not the span. Check [ARIA in HTML][html-naming] for restrictions on the final element. + +## Names, Descriptions, and State + +A description is optional when the name is sufficient. For a file action, the name might identify +the operation and file, while the description explains retention or recovery. Avoid repeating the +same sentence in both. See the [name and description computation specification][accname]. + +A name or description attribute is not an announcement mechanism. Keep progress and asynchronous +updates with their existing feature owner. Follow [Button] for loading behavior and [Forms] for +field error relationships. + +## Overrides and References + +Authoring preference differs from computation priority. An `aria-labelledby` value with at least +one valid ID reference is evaluated first. If its computed text is non-empty, it takes precedence +over `aria-label` and normal native or content naming. If its result is empty, name computation +continues to lower-priority sources; do not rely on that fallback to excuse a broken reference. A +non-empty `aria-label` also overrides normal native or content naming; these sources are not +concatenated. See the [computation steps][computation]. + +- With `aria-labelledby`, reference the intended text directly. Multiple IDs are read in attribute + order; do not build chains of elements that each use `aria-labelledby`. +- Overriding a button or link's content-derived name can suppress meaningful descendant content in + its accessible representation. Preserve the necessary visible wording in the resulting name. +- Inspect IDs generated by primitives before overriding them. Keep IDs unique across repeated rows + and simultaneous dialogs, and ensure referenced nodes exist in relevant open, closed, and + responsive states. Preserve existing description IDs when adding another relationship. +- Do not use `title`, `placeholder`, or Tooltip content as the only naming source. Native `title` + does not replace an intentional name or description relationship. + +## Write Useful Names + +Keep the visible label's wording in the accessible name, preferably at the beginning. Add target +context when identical visible actions would otherwise be ambiguous. Matching visible words also +lets speech-input users invoke what they see. See [WCAG Label in Name][label-in-name]. + +Use concise action or purpose wording. Avoid appending role words already announced by assistive +technology or duplicating state exposed by the control. Consumers own localization and pass the +complete localized text through public props or children; Dify UI primitives do not import +application i18n. + +The following fragment assumes localized strings and owner-scoped unique IDs. It combines the +visible action with the file it affects: + +```tsx +<> + {fileName} + + +``` + +## Associate Descriptions + +Use `aria-describedby` to associate concise help or consequences with a named control. The +referenced content becomes a plain string: headings, lists, and interactive links do not retain +their structure in the description. Keep rich instructions reachable as normal content or through +the [Overlay] contract. `aria-details` may supplement structured content where supported; it does +not replace that reachable content. See [MDN `aria-describedby` guidance][describedby]. + +In Dify fields, compose `FieldDescription` and `FieldError` with the appropriate label and control. +These primitives own their relationships, including invalid-state feedback. Do not overwrite them +with a second label or a competing error association: + +```tsx + + {fileNameLabel} + + {formatHint} + {requiredMessage} + +``` + +Use `DialogTitle` and, when useful, `DialogDescription` for a short dialog summary. Do not turn a +whole form or rich dialog body into one description. Per [Base UI Tooltip guidance], Tooltip is a +supplemental visual label, not the trigger's accessible-name source. Base UI specifically recommends +an `aria-label` that closely matches the Tooltip content; apply that to icon-only triggers. When +persistent visible trigger text already supplies the name, preserve the content-derived name per +W3C and MDN guidance instead of adding a redundant override merely because Tooltip is present. Use +[Overlay] choices for essential, structured, interactive, or touch-reachable information. + +Prefer descriptions associated with DOM text. When considering `aria-description`, verify target +browser and assistive-technology behavior. The [AccName 1.2 working draft] gives +`aria-describedby` precedence over `aria-description`, followed by applicable native description +sources and unused `title` fallback. It specifies using only the first applicable source, even when +that source computes to an empty description. Do not stack mechanisms to force repeated output. + +## Hidden Text and Safe Removal + +- `sr-only` hides text visually while retaining it for assistive technology. It can contribute to + a content-derived name or serve as a referenced label or description. A standalone span does not + name a sibling control, and `sr-only` is not an automatic replacement for `aria-label`. +- `hidden`, `display: none`, `visibility: hidden`, and `aria-hidden="true"` normally exclude content + during name calculation. Explicitly referenced hidden nodes can still contribute; inspect the + reference and its subtree instead of assuming all hidden text is ignored. See the [computation + steps][computation] and [description reference][describedby]. +- Before removing a label, inspect the resulting name and description in collapsed navigation, + responsive icon-only layouts, loading, and disabled states. CSS truncation alone does not remove + underlying text. Preserve primitive relationships and necessary status information. Add hidden + text only when information would otherwise be missing. +- Verify observable names, descriptions, and keyboard behavior at the changed boundary. Follow the + test policy owned by that package or consumer. Dify UI changes follow [Package testing]; complex + overrides may also require inspecting the rendered accessibility tree and relevant screen-reader + behavior. + +[APG]: https://www.w3.org/WAI/ARIA/apg/practices/names-and-descriptions#namingtechniques +[AccName 1.2 working draft]: https://www.w3.org/TR/accname-1.2#mapping_additional_nd_description +[Base UI Tooltip guidance]: https://base-ui.com/react/components/tooltip#usage-guidelines +[Base UI accessibility]: https://base-ui.com/react/overview/accessibility +[Button]: ../src/button/README.md +[Forms]: forms.md +[IconButton]: ../src/icon-button/README.md +[MDN `aria-label` guidance]: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-label +[Overlay]: overlays.md +[Package testing]: testing.md +[accname]: https://www.w3.org/TR/accname-1.2#name_and_description +[captions]: https://www.w3.org/WAI/ARIA/apg/practices/names-and-descriptions +[computation]: https://www.w3.org/TR/accname-1.2#computation-steps +[describedby]: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-describedby +[html-naming]: https://www.w3.org/TR/html-aria#requirements-for-use-of-aria-attributes-to-name-elements +[label-in-name]: https://www.w3.org/WAI/WCAG22/Understanding/label-in-name.html diff --git a/packages/dify-ui/src/button/README.md b/packages/dify-ui/src/button/README.md index 95c9468e0b0..8ece6c6bdc0 100644 --- a/packages/dify-ui/src/button/README.md +++ b/packages/dify-ui/src/button/README.md @@ -119,8 +119,10 @@ for the other variants. Use a `className` override only for a documented layout ## Related guides - Read [`IconButton`] for icon-only actions. +- Read [Accessible names and descriptions] when choosing or changing a naming source. - Read [Base UI Button] for the upstream interaction and composition contract. +[Accessible names and descriptions]: ../../docs/accessible-names-and-descriptions.md [Base UI Button]: https://base-ui.com/react/components/button [WAI-ARIA `aria-busy`]: https://www.w3.org/TR/wai-aria#aria-busy [`IconButton`]: ../icon-button/README.md diff --git a/packages/dify-ui/src/icon-button/README.md b/packages/dify-ui/src/icon-button/README.md index 1d72a9b6865..3e15f5ae641 100644 --- a/packages/dify-ui/src/icon-button/README.md +++ b/packages/dify-ui/src/icon-button/README.md @@ -10,8 +10,9 @@ icon-specific appearance, size, and tone variants. ## Accessible name and glyph Every icon button must provide exactly one accessible-name source: `aria-label` or -`aria-labelledby`, preserving its [name, role, and value]. A tooltip is a visual enhancement, not -the button's accessible name. +`aria-labelledby`, preserving its [name, role, and value]. Follow [Accessible names and +descriptions] when choosing between those sources. A tooltip is a visual enhancement, not the +button's accessible name. Pass exactly one React element containing the decorative glyph and hide that glyph from the accessibility tree: @@ -40,8 +41,10 @@ the icon button. ## Related guides - Read [`Button`] for visible-label actions, submit semantics, and loading state. +- Read [Accessible names and descriptions] for the cross-component naming and description contract. - Read [Base UI Button] for the upstream interaction and composition contract. +[Accessible names and descriptions]: ../../docs/accessible-names-and-descriptions.md [Base UI Button]: https://base-ui.com/react/components/button [`Button`]: ../button/README.md [name, role, and value]: https://www.w3.org/WAI/WCAG22/Understanding/name-role-value.html diff --git a/web/AGENTS.md b/web/AGENTS.md index 111d60cdff0..96eee69151d 100644 --- a/web/AGENTS.md +++ b/web/AGENTS.md @@ -12,6 +12,7 @@ - Reuse the Web `SearchInput` composite when its search, clear, and IME contract matches the feature; otherwise follow the canonical [Input Group contract]. - Give save and submit flows a real form boundary with visible labels and accessible errors. Use Dify UI `Form` when its structured submission and validation contract is the owner; otherwise use a native form. Follow the canonical [form contract]. - Follow the canonical [Button contract] and [IconButton contract] for action semantics, loading, accessible names, and primitive composition. Do not add a Web wrapper that hides those contracts. +- Follow [Accessible names and descriptions] when choosing or changing visible labels, ARIA naming, descriptions, or visually hidden text. Web owns localization and feature-specific status announcements; do not redefine the Dify UI naming contract locally. - Follow the [Dify UI overlay contract] for primitive selection, portals, focus, and layering. Reuse the Web `Infotip` composite for an info glyph that opens explanatory content. Do not introduce a generic Web wrapper that recreates Dify UI overlay behavior. - For custom SVG icons, follow `../packages/iconify-collections/README.md`; do not add generated React icons under `app/components/base/icons/src/`. - `docs/test.md` is the single source of truth for Web automated-test policy. Skills may route and execute that policy but must not redefine it. @@ -26,6 +27,7 @@ This block is written and re-added by `next dev` — verify at `node_modules/nex +[Accessible names and descriptions]: ../packages/dify-ui/docs/accessible-names-and-descriptions.md [Button contract]: ../packages/dify-ui/src/button/README.md [Dify UI overlay contract]: ../packages/dify-ui/docs/overlays.md [Dify UI package index]: ../packages/dify-ui/README.md diff --git a/web/app/components/app-sidebar/dataset-info/__tests__/index.spec.tsx b/web/app/components/app-sidebar/dataset-info/__tests__/index.spec.tsx index dd31a2787aa..434c824ec11 100644 --- a/web/app/components/app-sidebar/dataset-info/__tests__/index.spec.tsx +++ b/web/app/components/app-sidebar/dataset-info/__tests__/index.spec.tsx @@ -211,12 +211,12 @@ describe('DatasetInfo', () => { expect(screen.queryByText('dataset.chunkingMode.general')).not.toBeInTheDocument() }) - it('should hide detailed fields when collapsed', () => { + it('should keep the dataset name available when collapsed and omit detailed fields', () => { // Arrange render() // Assert - expect(screen.queryByText('Dataset Name')).not.toBeInTheDocument() + expect(screen.getByText('Dataset Name')).toBeInTheDocument() expect(screen.queryByText('Dataset description')).not.toBeInTheDocument() }) }) diff --git a/web/app/components/app-sidebar/dataset-info/index.tsx b/web/app/components/app-sidebar/dataset-info/index.tsx index c71ce99b28d..f10e7a2383f 100644 --- a/web/app/components/app-sidebar/dataset-info/index.tsx +++ b/web/app/components/app-sidebar/dataset-info/index.tsx @@ -31,7 +31,6 @@ const DatasetInfo = ({ expand }: DatasetInfoProps) => { 'relative overflow-hidden rounded-xl', expand ? 'p-2 hover:bg-state-base-hover' : 'flex items-center justify-center px-1 py-1.5', )} - aria-label={!expand ? dataset.name : undefined} >
{ imageUrl={iconInfo.icon_url} />
+ {!expand && {dataset.name}} {expand && ( <>
diff --git a/web/app/components/app/configuration/debug/debug-with-multiple-model/__tests__/model-parameter-trigger.spec.tsx b/web/app/components/app/configuration/debug/debug-with-multiple-model/__tests__/model-parameter-trigger.spec.tsx index 2913542f8ec..edbd7b0f374 100644 --- a/web/app/components/app/configuration/debug/debug-with-multiple-model/__tests__/model-parameter-trigger.spec.tsx +++ b/web/app/components/app/configuration/debug/debug-with-multiple-model/__tests__/model-parameter-trigger.spec.tsx @@ -398,39 +398,64 @@ describe('ModelParameterTrigger', () => { }) it('should render configured model id and incompatible tooltip when model is missing from the provider list', async () => { + const user = userEvent.setup() renderComponent() expect(screen.getByText('gpt-3.5-turbo')).toBeInTheDocument() - await userEvent.hover(screen.getByLabelText('common.modelProvider.selector.incompatibleTip')) + const trigger = screen.getByRole('button', { + name: /common.modelProvider.selector.incompatibleTip/, + }) + await user.hover(trigger) expect( - await screen.findByText('common.modelProvider.selector.incompatibleTip'), + await screen.findByText( + (content, element) => + content === 'common.modelProvider.selector.incompatibleTip' && + !!element && + !trigger.contains(element), + ), ).toBeInTheDocument() }) it('should render configure required tooltip for no-configure status', async () => { + const user = userEvent.setup() mockUseCurrentModel.mockReturnValue({ currentProvider: { provider: 'openai' }, currentModel: { model: 'gpt-3.5-turbo', status: ModelStatusEnum.noConfigure }, }) renderComponent() - await userEvent.hover( - screen.getByLabelText('common.modelProvider.selector.configureRequired'), - ) + const trigger = screen.getByRole('button', { + name: /common.modelProvider.selector.configureRequired/, + }) + await user.hover(trigger) expect( - await screen.findByText('common.modelProvider.selector.configureRequired'), + await screen.findByText( + (content, element) => + content === 'common.modelProvider.selector.configureRequired' && + !!element && + !trigger.contains(element), + ), ).toBeInTheDocument() }) it('should render disabled tooltip for disabled status', async () => { + const user = userEvent.setup() mockUseCurrentModel.mockReturnValue({ currentProvider: { provider: 'openai' }, currentModel: { model: 'gpt-3.5-turbo', status: ModelStatusEnum.disabled }, }) renderComponent() - await userEvent.hover(screen.getByLabelText('common.modelProvider.selector.disabled')) - expect(await screen.findByText('common.modelProvider.selector.disabled')).toBeInTheDocument() + const trigger = screen.getByRole('button', { name: /common.modelProvider.selector.disabled/ }) + await user.hover(trigger) + expect( + await screen.findByText( + (content, element) => + content === 'common.modelProvider.selector.disabled' && + !!element && + !trigger.contains(element), + ), + ).toBeInTheDocument() }) }) diff --git a/web/app/components/app/configuration/debug/debug-with-multiple-model/model-parameter-trigger.tsx b/web/app/components/app/configuration/debug/debug-with-multiple-model/model-parameter-trigger.tsx index d9cd7d97572..886b2a41a43 100644 --- a/web/app/components/app/configuration/debug/debug-with-multiple-model/model-parameter-trigger.tsx +++ b/web/app/components/app/configuration/debug/debug-with-multiple-model/model-parameter-trigger.tsx @@ -108,10 +108,9 @@ const DebugModelParameterTrigger: FC = ({ className={`i-ri-arrow-down-s-line size-3 ${isEmpty ? 'text-text-accent' : 'text-text-tertiary'}`} /> {statusTooltipLabel && ( - + + {statusTooltipLabel} + )} } diff --git a/web/app/components/app/overview/customize/index.tsx b/web/app/components/app/overview/customize/index.tsx index b77bcdeb06f..bd148f55819 100644 --- a/web/app/components/app/overview/customize/index.tsx +++ b/web/app/components/app/overview/customize/index.tsx @@ -107,9 +107,6 @@ const CustomizeModal: FC = ({ href={`https://github.com/langgenius/${repository}`} target="_blank" rel="noopener noreferrer" - aria-label={t(($) => $[`${prefixCustomize}.way1.step1Operation`], { - ns: 'appOverview', - })} className={buttonVariants()} > @@ -130,9 +127,6 @@ const CustomizeModal: FC = ({ href="https://vercel.com/docs/concepts/deployments/git/vercel-for-github" target="_blank" rel="noopener noreferrer" - aria-label={t(($) => $[`${prefixCustomize}.way1.step2Operation`], { - ns: 'appOverview', - })} className={buttonVariants()} >
@@ -173,9 +167,6 @@ const CustomizeModal: FC = ({ href={apiDocLink} target="_blank" rel="noopener noreferrer" - aria-label={t(($) => $[`${prefixCustomize}.way2.operation`], { - ns: 'appOverview', - })} className={cn(buttonVariants(), 'mt-2')} > diff --git a/web/app/components/apps/app-sort-filter.tsx b/web/app/components/apps/app-sort-filter.tsx index 8aaa8c77dbb..be7c96da6b1 100644 --- a/web/app/components/apps/app-sort-filter.tsx +++ b/web/app/components/apps/app-sort-filter.tsx @@ -44,10 +44,7 @@ export function AppSortFilter({ value, onChange }: AppSortFilterProps) { return ( - + {sortByLabel}{' '} {activeOption.text} diff --git a/web/app/components/base/inline-delete-confirm/__tests__/index.spec.tsx b/web/app/components/base/inline-delete-confirm/__tests__/index.spec.tsx index a8158d334ee..691529c6912 100644 --- a/web/app/components/base/inline-delete-confirm/__tests__/index.spec.tsx +++ b/web/app/components/base/inline-delete-confirm/__tests__/index.spec.tsx @@ -47,16 +47,15 @@ describe('InlineDeleteConfirm', () => { expect(getByText('Confirm')).toBeInTheDocument() }) - it('should have proper ARIA attributes', () => { + it('should expose the prompt title and description on a semantic group', () => { const onConfirm = vi.fn() const onCancel = vi.fn() - const { container } = render( + const { getByRole } = render( , ) - const wrapper = container.firstChild as HTMLElement - expect(wrapper).toHaveAttribute('aria-labelledby', 'inline-delete-confirm-title') - expect(wrapper).toHaveAttribute('aria-describedby', 'inline-delete-confirm-description') + const prompt = getByRole('group', { name: 'Delete?' }) + expect(prompt).toHaveAccessibleDescription('Please confirm your action.') }) }) diff --git a/web/app/components/base/inline-delete-confirm/index.tsx b/web/app/components/base/inline-delete-confirm/index.tsx index 850c2990f91..bc9e1258527 100644 --- a/web/app/components/base/inline-delete-confirm/index.tsx +++ b/web/app/components/base/inline-delete-confirm/index.tsx @@ -2,6 +2,7 @@ import type { FC } from 'react' import { Button } from '@langgenius/dify-ui/button' import { cn } from '@langgenius/dify-ui/cn' +import { useId } from 'react' import { useTranslation } from 'react-i18next' type InlineDeleteConfirmProps = { @@ -24,6 +25,8 @@ const InlineDeleteConfirm: FC = ({ variant = 'delete', }) => { const { t } = useTranslation() + const titleId = useId() + const descriptionId = useId() const titleText = title || t(($) => $['operation.deleteConfirmTitle'], { ns: 'common', defaultValue: 'Delete?' }) @@ -33,8 +36,9 @@ const InlineDeleteConfirm: FC = ({ return (
= ({ className, )} > -
+
{titleText}
-
- + {t(($) => $['operation.confirmAction'], { ns: 'common', defaultValue: 'Please confirm your action.', diff --git a/web/app/components/datasets/create/embedding-process/__tests__/indexing-progress-item.spec.tsx b/web/app/components/datasets/create/embedding-process/__tests__/indexing-progress-item.spec.tsx index fa85e99a23c..825fe39e3ab 100644 --- a/web/app/components/datasets/create/embedding-process/__tests__/indexing-progress-item.spec.tsx +++ b/web/app/components/datasets/create/embedding-process/__tests__/indexing-progress-item.spec.tsx @@ -93,7 +93,18 @@ describe('IndexingProgressItem', () => { />, ) - expect(screen.getByLabelText('Parse failed')).toBeInTheDocument() + expect(screen.getByText('Parse failed')).toBeInTheDocument() + }) + + it('should use the localized fallback when an error has no message', () => { + render( + , + ) + + expect(screen.getByText('common.error')).toBeInTheDocument() }) it('should show priority label when billing is enabled', () => { diff --git a/web/app/components/datasets/create/embedding-process/indexing-progress-item.tsx b/web/app/components/datasets/create/embedding-process/indexing-progress-item.tsx index be57451e50b..571a8ced063 100644 --- a/web/app/components/datasets/create/embedding-process/indexing-progress-item.tsx +++ b/web/app/components/datasets/create/embedding-process/indexing-progress-item.tsx @@ -3,6 +3,7 @@ import type { IndexingStatusResponse } from '@/models/datasets' import { cn } from '@langgenius/dify-ui/cn' import { Tooltip, TooltipContent, TooltipTrigger } from '@langgenius/dify-ui/tooltip' import { RiCheckboxCircleFill, RiErrorWarningFill } from '@remixicon/react' +import { useTranslation } from 'react-i18next' import NotionIcon from '@/app/components/base/notion-icon' import PriorityLabel from '@/app/components/billing/priority-label' import { DataSourceType } from '@/models/datasets' @@ -19,20 +20,25 @@ type IndexingProgressItemProps = { // Status icon component for completed/error states const StatusIcon: FC<{ status: string; error?: string }> = ({ status, error }) => { + const { t } = useTranslation() + if (status === 'completed') return if (status === 'error') { + const errorLabel = error || t(($) => $.error, { ns: 'common' }) + return ( - }> - + }> + + {errorLabel} - {error} + {errorLabel} ) diff --git a/web/app/components/datasets/metadata/metadata-dataset/dataset-metadata-picker.tsx b/web/app/components/datasets/metadata/metadata-dataset/dataset-metadata-picker.tsx index e7a16969720..f3b5c6b121c 100644 --- a/web/app/components/datasets/metadata/metadata-dataset/dataset-metadata-picker.tsx +++ b/web/app/components/datasets/metadata/metadata-dataset/dataset-metadata-picker.tsx @@ -107,12 +107,7 @@ export function DatasetMetadataPicker({ $['metadata.addMetadata'], { ns: 'dataset' })} - className="w-full px-2 py-0" - > + } /> @@ -164,10 +164,8 @@ const MCPDetailContent: FC = ({ +
+ {`${serverUrlLabel}: `} {detail.server_url}
} diff --git a/web/app/components/tools/provider/create-entry-card.tsx b/web/app/components/tools/provider/create-entry-card.tsx index 07c879ba077..9ec6e114c3e 100644 --- a/web/app/components/tools/provider/create-entry-card.tsx +++ b/web/app/components/tools/provider/create-entry-card.tsx @@ -29,7 +29,6 @@ const CreateEntryCard = ({ > - diff --git a/web/app/components/workflow/nodes/agent/components/__tests__/model-bar.spec.tsx b/web/app/components/workflow/nodes/agent/components/__tests__/model-bar.spec.tsx index 46f450684db..da370451599 100644 --- a/web/app/components/workflow/nodes/agent/components/__tests__/model-bar.spec.tsx +++ b/web/app/components/workflow/nodes/agent/components/__tests__/model-bar.spec.tsx @@ -54,7 +54,7 @@ describe('agent/model-bar', () => { expect(emptySelector).toBeInTheDocument() expect(screen.getByText('indicator:error')).toBeInTheDocument() - expect(screen.getByLabelText('workflow.nodes.agent.modelNotSelected')).toBeInTheDocument() + expect(screen.getByText('workflow.nodes.agent.modelNotSelected')).toBeInTheDocument() }) it('should render the selected model without warning when it is installed', () => { @@ -69,6 +69,6 @@ describe('agent/model-bar', () => { expect(screen.getByText('openai/gpt-4.1:1')).toBeInTheDocument() expect(screen.getByText('indicator:error')).toBeInTheDocument() - expect(screen.getByLabelText('workflow.nodes.agent.modelNotInstallTooltip')).toBeInTheDocument() + expect(screen.getByText('workflow.nodes.agent.modelNotInstallTooltip')).toBeInTheDocument() }) }) diff --git a/web/app/components/workflow/nodes/agent/components/__tests__/tool-icon.spec.tsx b/web/app/components/workflow/nodes/agent/components/__tests__/tool-icon.spec.tsx index 465b4517e11..65a4f687f37 100644 --- a/web/app/components/workflow/nodes/agent/components/__tests__/tool-icon.spec.tsx +++ b/web/app/components/workflow/nodes/agent/components/__tests__/tool-icon.spec.tsx @@ -93,7 +93,7 @@ describe('agent/tool-icon', () => { expect(screen.getByText('indicator:warning')).toBeInTheDocument() expect( - screen.getByLabelText('workflow.nodes.agent.toolNotAuthorizedTooltip:{"tool":"tool-b"}'), + screen.getByText('workflow.nodes.agent.toolNotAuthorizedTooltip:{"tool":"tool-b"}'), ).toBeInTheDocument() mockWorkflowTools = [] @@ -104,7 +104,7 @@ describe('agent/tool-icon', () => { expect(marketplaceIcon).toHaveAttribute('src', 'https://example.com/market-tool.png') expect(screen.getByText('indicator:error')).toBeInTheDocument() expect( - screen.getByLabelText('workflow.nodes.agent.toolNotInstallTooltip:{"tool":"tool-c"}'), + screen.getByText('workflow.nodes.agent.toolNotInstallTooltip:{"tool":"tool-c"}'), ).toBeInTheDocument() }) diff --git a/web/app/components/workflow/nodes/agent/components/model-bar.tsx b/web/app/components/workflow/nodes/agent/components/model-bar.tsx index b82e1847af1..e8c38cdb3ad 100644 --- a/web/app/components/workflow/nodes/agent/components/model-bar.tsx +++ b/web/app/components/workflow/nodes/agent/components/model-bar.tsx @@ -47,7 +47,7 @@ export const ModelBar: FC = (props) => { +
= (props) => { disabled /> + {tooltip}
} /> @@ -76,7 +77,7 @@ export const ModelBar: FC = (props) => { ns: 'workflow', }) const modelSelector = ( -
+
= (props) => { disabled /> {showWarn && } + {showWarn && {modelNotInstalledTooltip}}
) diff --git a/web/app/components/workflow/nodes/agent/components/tool-icon.tsx b/web/app/components/workflow/nodes/agent/components/tool-icon.tsx index 34f121f8e10..08e6b1e4a88 100644 --- a/web/app/components/workflow/nodes/agent/components/tool-icon.tsx +++ b/web/app/components/workflow/nodes/agent/components/tool-icon.tsx @@ -92,11 +92,12 @@ export const ToolIcon = memo(({ providerName }: ToolIconProps) => { } const iconNode = ( -
+
{iconContent}
{indicator && } + {tooltip && {tooltip}}
) diff --git a/web/app/components/workflow/nodes/question-classifier/components/class-item.tsx b/web/app/components/workflow/nodes/question-classifier/components/class-item.tsx index f69cc24012c..ca0ff00ebfd 100644 --- a/web/app/components/workflow/nodes/question-classifier/components/class-item.tsx +++ b/web/app/components/workflow/nodes/question-classifier/components/class-item.tsx @@ -122,7 +122,6 @@ const ClassItem: FC = ({ ) : ( } /> - {label} + {label} ) } diff --git a/web/features/agent-v2/agent-detail/configure/components/orchestrate/tools/provider-tool/item.tsx b/web/features/agent-v2/agent-detail/configure/components/orchestrate/tools/provider-tool/item.tsx index f967f6b33b9..54bca92f511 100644 --- a/web/features/agent-v2/agent-detail/configure/components/orchestrate/tools/provider-tool/item.tsx +++ b/web/features/agent-v2/agent-detail/configure/components/orchestrate/tools/provider-tool/item.tsx @@ -328,12 +328,7 @@ export const AgentProviderToolItem = memo( {!readOnly && ( - $['agentDetail.configure.tools.moreActions'], { - name: tool.name, - })} - className="flex size-6 shrink-0 items-center justify-center rounded-md text-text-tertiary hover:bg-state-base-hover hover:text-text-secondary focus-visible:ring-2 focus-visible:ring-state-accent-solid focus-visible:outline-hidden data-popup-open:bg-state-base-hover" - > + {t(($) => $['agentDetail.configure.tools.moreActions'], { name: tool.name })} diff --git a/web/features/agent-v2/agent-detail/configure/components/preview/__tests__/header.spec.tsx b/web/features/agent-v2/agent-detail/configure/components/preview/__tests__/header.spec.tsx index 52b78fff071..a6f8cc915e7 100644 --- a/web/features/agent-v2/agent-detail/configure/components/preview/__tests__/header.spec.tsx +++ b/web/features/agent-v2/agent-detail/configure/components/preview/__tests__/header.spec.tsx @@ -122,7 +122,7 @@ describe('AgentPreviewHeader', () => { renderHeader({ mode: 'build', onOpenWorkingDirectory, showWorkingDirectoryAction: true }) const fileSystemButton = screen.getByRole('button', { - name: 'agentV2.agentDetail.configure.workingDirectory.open', + name: 'agentV2.agentDetail.configure.workingDirectory.fileSystem', }) expect(fileSystemButton).toHaveTextContent( 'agentV2.agentDetail.configure.workingDirectory.fileSystem', @@ -137,7 +137,9 @@ describe('AgentPreviewHeader', () => { renderHeader({ mode: 'build' }) expect( - screen.queryByRole('button', { name: 'agentV2.agentDetail.configure.workingDirectory.open' }), + screen.queryByRole('button', { + name: 'agentV2.agentDetail.configure.workingDirectory.fileSystem', + }), ).not.toBeInTheDocument() }) @@ -171,7 +173,9 @@ describe('AgentPreviewHeader', () => { }) await user.hover( - screen.getByLabelText('agentV2.agentDetail.configure.rightPanel.previewDisabledTip'), + screen.getByLabelText( + 'agentV2.agentDetail.configure.rightPanel.preview. agentV2.agentDetail.configure.rightPanel.previewDisabledTip', + ), ) expect( diff --git a/web/features/agent-v2/agent-detail/configure/components/preview/header.tsx b/web/features/agent-v2/agent-detail/configure/components/preview/header.tsx index 4ba43994f03..d0380318b35 100644 --- a/web/features/agent-v2/agent-detail/configure/components/preview/header.tsx +++ b/web/features/agent-v2/agent-detail/configure/components/preview/header.tsx @@ -66,10 +66,12 @@ function ModeInfoTip({ children, ariaLabel }: { children: ReactNode; ariaLabel: function PreviewModeItem({ previewEnabled, + label, disabledTip, children, }: { previewEnabled: boolean + label: string disabledTip: string children: ReactNode }) { @@ -90,7 +92,7 @@ function PreviewModeItem({ } > {item} @@ -165,7 +167,11 @@ export function AgentPreviewHeader({ {t(($) => $['agentDetail.configure.rightPanel.build'])} - + {t(($) => $['agentDetail.configure.rightPanel.preview'])} @@ -222,7 +228,6 @@ export function AgentPreviewHeader({ type="button" onClick={onOpenWorkingDirectory} className="flex h-8 items-center justify-center gap-0.5 rounded-lg px-3 py-2 text-text-tertiary hover:bg-state-base-hover hover:text-text-secondary focus-visible:ring-2 focus-visible:ring-state-accent-solid focus-visible:outline-hidden" - aria-label={t(($) => $['agentDetail.configure.workingDirectory.open'])} > @@ -242,7 +247,6 @@ export function AgentPreviewHeader({ 'flex h-8 items-center justify-center gap-1 rounded-lg px-2 py-2 text-text-tertiary hover:bg-state-base-hover hover:text-text-secondary focus-visible:ring-2 focus-visible:ring-state-accent-solid focus-visible:outline-hidden', isChatFeaturesOpen && 'bg-state-base-hover text-text-secondary', )} - aria-label={t(($) => $['agentDetail.configure.preview.chatFeatures'])} > diff --git a/web/features/agent-v2/agent-detail/configure/components/preview/working-directory-panel.tsx b/web/features/agent-v2/agent-detail/configure/components/preview/working-directory-panel.tsx index 3abf047d8fe..d4115e756e8 100644 --- a/web/features/agent-v2/agent-detail/configure/components/preview/working-directory-panel.tsx +++ b/web/features/agent-v2/agent-detail/configure/components/preview/working-directory-panel.tsx @@ -673,10 +673,9 @@ export function AgentWorkingDirectoryPanel({ onSelectFile: (selectedFile) => setSelectedFileId(selectedFile.id), renderFolderSuffix: ({ file }) => loadingFolderPaths.has(file.id) ? ( - $.loading)} - className="ms-auto i-ri-loader-4-line size-4 shrink-0 animate-spin text-text-tertiary" - /> + + {tCommon(($) => $.loading)} + ) : null, selectedFileId: selectedWorkingDirectoryFile?.id, sections: [], diff --git a/web/features/new-rag/__tests__/new-knowledge-list.spec.tsx b/web/features/new-rag/__tests__/new-knowledge-list.spec.tsx index 0ab12ddc07c..3c46e3a6c43 100644 --- a/web/features/new-rag/__tests__/new-knowledge-list.spec.tsx +++ b/web/features/new-rag/__tests__/new-knowledge-list.spec.tsx @@ -195,7 +195,7 @@ describe('NewKnowledgeList', () => { ).toHaveAttribute('href', '/datasets/new/space-2/sources') expect(within(list).getByText('Answers for customer support')).toBeInTheDocument() expect(within(list).getByText('dataset.newKnowledge.noDescription')).toBeInTheDocument() - expect(within(supportCard).getByLabelText('camera')).toBeInTheDocument() + expect(within(supportCard).getByTitle('camera')).toBeInTheDocument() expect(within(list).getAllByText('dataset.newKnowledge.cardType')).toHaveLength(2) expect(within(list).getAllByText('dataset.newKnowledge.tags')).toHaveLength(2) expect(within(list).getAllByText('dataset.newKnowledge.documentsUnavailable')).toHaveLength(2) diff --git a/web/features/new-rag/components/knowledge-space-card.tsx b/web/features/new-rag/components/knowledge-space-card.tsx index 587c372c385..d3f32d992ca 100644 --- a/web/features/new-rag/components/knowledge-space-card.tsx +++ b/web/features/new-rag/components/knowledge-space-card.tsx @@ -27,7 +27,6 @@ export function KnowledgeSpaceCard({ knowledgeSpace }: { knowledgeSpace: Knowled >
$['newKnowledge.cardType'])} title={iconName} className="flex size-10 shrink-0 items-center justify-center rounded-[10px] border-[0.5px] border-divider-regular bg-components-icon-bg-orange-dark-soft" > @@ -51,10 +50,7 @@ export function KnowledgeSpaceCard({ knowledgeSpace }: { knowledgeSpace: Knowled

{knowledgeSpace.description || t(($) => $['newKnowledge.noDescription'])}

-
$['newKnowledge.tags'])}. ${unavailable}`} - className="mt-1 flex min-w-0 items-center gap-1 px-4" - > +
{t(($) => $['newKnowledge.tags'])} diff --git a/web/features/new-rag/document-chunk-tree.tsx b/web/features/new-rag/document-chunk-tree.tsx index 90f49c70d51..bc32b68221b 100644 --- a/web/features/new-rag/document-chunk-tree.tsx +++ b/web/features/new-rag/document-chunk-tree.tsx @@ -4,7 +4,7 @@ import type { DocumentChunkTree } from './document-detail-model' import { Button } from '@langgenius/dify-ui/button' import { cn } from '@langgenius/dify-ui/cn' import { defaultRangeExtractor, useVirtualizer } from '@tanstack/react-virtual' -import { useEffect, useMemo, useRef, useState } from 'react' +import { useEffect, useId, useMemo, useRef, useState } from 'react' import { useTranslation } from 'react-i18next' import Loading from '@/app/components/base/loading' import { chunkTreeLabel, visibleDocumentChunkNodes } from './document-detail-model' @@ -39,6 +39,7 @@ export function DocumentChunkTreePanel({ }) { const { t } = useTranslation('dataset') const { t: tCommon } = useTranslation('common') + const treeHeadingId = useId() const [collapsedChunkIds, setCollapsedChunkIds] = useState>(() => new Set()) const [focusedChunkId, setFocusedChunkId] = useState() const [treeHasFocus, setTreeHasFocus] = useState(false) @@ -159,7 +160,6 @@ export function DocumentChunkTreePanel({ key={chunk.id} id={`document-chunk-treeitem-${chunk.id}`} aria-expanded={hasChildren ? expanded : undefined} - aria-label={label} aria-level={depth + 1} aria-posinset={positionInSet} aria-selected={selectedChunkId === chunk.id} @@ -195,7 +195,7 @@ export function DocumentChunkTreePanel({ return (