fix: Update deploy files#30458
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
dosubot
Bot
added
the
size:XXL
Summary of ChangesHello @Germey, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands Dify's authentication capabilities by integrating AceDataCloud OAuth, providing a new, streamlined login and registration experience. Concurrently, it introduces a complete set of Kubernetes deployment configurations, enabling robust and scalable production deployments. Additionally, a new example plugin, 'nano-banana', has been added to showcase Dify's extensibility for custom tooling. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for AceDataCloud as a new OAuth provider, which can be configured to be the exclusive login method. The changes span the backend API, frontend, and deployment configurations. Key modifications include adding the AceDataCloud OAuth flow, persisting tokens, and allowing for automatic user registration and workspace creation. The frontend is updated to handle automatic redirection to the OAuth provider. Additionally, new Kubernetes deployment files and a nano-banana plugin have been added. The review focuses on improving error handling, data integrity, and code maintainability.
| except Exception: | ||
| logger.exception("Failed to load AceDataCloud token for account %s", account.id) | ||
| return {"result": "success", "data": None} |
There was a problem hiding this comment.
This except Exception: block is too broad. It can catch unexpected programming errors (like TypeError, AttributeError, etc.) and mask them as a generic 'Failed to load AceDataCloud token' error, which makes debugging more difficult. It's better to catch more specific exceptions that you anticipate during the token loading and decryption process, such as IOError, json.JSONDecodeError, or any custom exceptions from your decryption logic. If a catch-all is truly necessary, consider logging the specific exception type to provide more context for debugging.
| name = raw_info.get("username") or "Ace Data Cloud User" | ||
| email = raw_info.get("email") or f"{user_id}@acedata.cloud" | ||
| return OAuthUserInfo(id=str(user_id), name=name, email=email) |
There was a problem hiding this comment.
The fallback logic for the user's email address, email = raw_info.get("email") or f"{user_id}@acedata.cloud", could create a non-existent email address if the OAuth provider does not return one. This might cause issues in other parts of the application that rely on a valid, deliverable email address for notifications or other communications. It would be safer to either make the email a required field from the OAuth provider or handle cases where the email is missing more explicitly, perhaps by raising an error or marking the account as needing email verification.
| if resp.status_code >= 400: | ||
| trace_id = body.get("trace_id") if isinstance(body.get("trace_id"), str) else None | ||
| error = body.get("error") if isinstance(body.get("error"), dict) else {} | ||
| code = error.get("code") if isinstance(error.get("code"), str) else None | ||
| message = error.get("message") if isinstance(error.get("message"), str) else None | ||
| trace_suffix = f" (trace_id={trace_id})" if trace_id else "" | ||
| raise AceDataNanoBananaError( | ||
| f"HTTP {resp.status_code}: {code or 'error'}: {message or body}{trace_suffix}" | ||
| ) | ||
|
|
||
| if body.get("success") is not True: | ||
| trace_id = body.get("trace_id") if isinstance(body.get("trace_id"), str) else None | ||
| error = body.get("error") if isinstance(body.get("error"), dict) else {} | ||
| code = error.get("code") if isinstance(error.get("code"), str) else None | ||
| message = error.get("message") if isinstance(error.get("message"), str) else None | ||
| trace_suffix = f" (trace_id={trace_id})" if trace_id else "" | ||
| raise AceDataNanoBananaError(f"{code or 'api_error'}: {message or body}{trace_suffix}") |
There was a problem hiding this comment.
The error handling logic for resp.status_code >= 400 and body.get("success") is not True is very similar and appears to be duplicated. This could be refactored into a private helper method to improve maintainability and reduce code duplication. This helper could be responsible for parsing the error details from the response body and raising a consistent AceDataNanoBananaError. This would make the error handling more robust and easier to manage.
There was a problem hiding this comment.
Pull request overview
This PR integrates AceDataCloud OAuth authentication into the Dify platform and updates deployment configurations. The changes enable single sign-on through AceDataCloud while adding new deployment automation and a nano-banana plugin for image generation.
- Adds AceDataCloud OAuth provider with auto-provisioning capabilities
- Updates Docker and Kubernetes deployment configurations for production
- Introduces nano-banana plugin for AI image generation via Ace Data Cloud API
Reviewed changes
Copilot reviewed 84 out of 86 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| web/service/acedatacloud-oauth.ts | New OAuth session management utilities |
| web/app/signin/page.tsx | Auto-redirect logic for AceDataCloud OAuth |
| web/app/login/page.tsx | New login page redirecting to signin |
| api/controllers/console/auth/oauth.py | OAuth callback handling with state validation |
| api/libs/oauth.py | AceDataCloud OAuth provider implementation |
| api/services/feature_service.py | Login method enforcement for AceDataCloud |
| docker/docker-compose.yaml | Updated image references and build configs |
| deploy/run.sh | New Kubernetes deployment script |
| plugins/nano-banana/* | Complete plugin for Nano Banana image API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| image: ghcr.io/acedatacloud/dify-api:${BUILD_NUMBER:-latest} | ||
| platform: linux/amd64 | ||
| build: '../api/.' |
There was a problem hiding this comment.
The build path ../api/. and ../web/. references parent directory which won't work when running docker-compose from the docker directory. The build context should be relative to the docker-compose file location or use absolute paths.
| const parseUtcSeconds = (iso?: string) => { | ||
| if (!iso) | ||
| return null | ||
| const normalized = /Z$|[+-]\\d\\d:?\\d\\d$/.test(iso) ? iso : `${iso}Z` |
There was a problem hiding this comment.
The regex pattern has an error - backslashes need to be escaped in the regex. The current pattern /Z$|[+-]\\d\\d:?\\d\\d$/ contains double backslashes which will match literal backslash characters rather than the digit pattern \d. This should be /Z$|[+-]\d\d:?\d\d$/ or use a raw string literal.
| const hasConsoleAccessTokenCookie = (cookieStore: CookieStore) => | ||
| Boolean(cookieStore.get('access_token')?.value || cookieStore.get('__Host-access_token')?.value) |
There was a problem hiding this comment.
Using __Host- prefix in cookie name requires the cookie to be set with Secure=true, Path=/, and no Domain attribute. However, the code checks for this cookie without verifying these constraints are met when setting it elsewhere in the codebase. This could lead to security issues or the cookie not being found as expected.
| namespace: acedatacloud | ||
| type: Opaque | ||
| stringData: | ||
| SECRET_KEY: please-change-me |
There was a problem hiding this comment.
Hardcoded secret key "please-change-me" in production secret configuration is a critical security vulnerability. This file should not contain actual secrets or should have placeholder text that makes it obvious this must be changed before deployment.
| SECRET_KEY: please-change-me | |
| SECRET_KEY: CHANGE_ME_BEFORE_PRODUCTION_DEPLOYMENT |
| if state_payload and state and state_payload.get("nonce") == state: | ||
| invite_token = state_payload.get("invite_token") | ||
| elif state_payload: | ||
| return {"error": "Invalid or expired OAuth state"}, 400 |
There was a problem hiding this comment.
The OAuth state validation logic has a flaw. When state_payload exists but state parameter is missing or mismatched, it returns an error. However, when state_payload is None (line 144), it silently continues without validation. This inconsistent behavior could allow CSRF attacks when the session state is missing.
| if state_payload and state and state_payload.get("nonce") == state: | |
| invite_token = state_payload.get("invite_token") | |
| elif state_payload: | |
| return {"error": "Invalid or expired OAuth state"}, 400 | |
| # Enforce consistent OAuth state validation: if the stored state payload | |
| # is missing, or the returned state parameter is missing or mismatched, | |
| # treat the OAuth flow as invalid. | |
| if not state_payload or not state or state_payload.get("nonce") != state: | |
| return {"error": "Invalid or expired OAuth state"}, 400 | |
| invite_token = state_payload.get("invite_token") |
| # plugin daemon | ||
| plugin_daemon: | ||
| image: langgenius/dify-plugin-daemon:0.5.2-local | ||
| platform: linux/amd64 |
There was a problem hiding this comment.
Using platform: linux/amd64 forces the container to run in emulation mode on ARM systems, which significantly degrades performance. Consider using multi-platform builds or removing this constraint to allow native execution on ARM64 systems listed in the manifest.
| useEffect(() => { | ||
| saveOrUpdate() | ||
| }, []) |
There was a problem hiding this comment.
Missing dependency array for the useEffect hook. This will cause the effect to run on every render, potentially making redundant API calls. Add an empty dependency array [] to run only on mount.
| catch { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Bare except clause catches all exceptions including system exits and keyboard interrupts. This should catch specific exceptions or at minimum use except Exception: to avoid catching BaseException subclasses.
Summary
fix: Update deploy files
Checklist
dev/reformat(backend) andcd web && npx lint-staged(frontend) to appease the lint gods