Skip to content

fix: chat user login #7791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wxg0103 merged 1 commit into from
Nov 10, 2026
Merged

fix: chat user login #7791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions apps/chat/serializers/chat_authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
"""

import uuid_utils.compat as uuid
from django.core import signing
from django.core.cache import cache
from django.db.models import QuerySet
from django.utils.translation import gettext_lazy as _
Expand Down Expand Up @@ -116,6 +115,7 @@ def profile():
"max_attempts": 1,
"enable_auth": False,
"login_value": ["LOCAL"],
"default_value": "LOCAL",
"rsa_key": get_key_pair_by_sql().get("key"),
"meta": {},
"logo": "./favicon.ico",
Expand All @@ -128,8 +128,9 @@ def profile():
return {
**{
"max_attempts": auth_config.get("max_attempts", 1),
"enable_auth": portal.enable_auth,
"enable_auth": False,
"login_value": auth_config.get("login_value", []),
"default_value": auth_config.get("type", "LOCAL"),
"rsa_key": get_key_pair_by_sql().get("key"),
"meta": {},
"logo": portal.logo or "./favicon.ico",
Expand Down
1 change: 0 additions & 1 deletion apps/portal/api/portal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ def get_request():
"tab_logo": {"type": "string", "format": "binary", "description": "浏览器Tab Logo"},
"enable_public_access": {"type": "boolean", "description": "是否开启公开访问"},
"enable_api": {"type": "boolean", "description": "是否开启API服务"},
"enable_auth": {"type": "boolean", "description": "是否开启身份认证"},
"auth_config": {"type": "object", "description": "身份认证配置"},
"enable_cors": {"type": "boolean", "description": "是否开启跨域设置"},
"cross_domain_list": {
Expand Down
19 changes: 10 additions & 9 deletions apps/portal/serializers/portal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class PortalSerializer(serializers.Serializer):
enable_knowledge_base_api = serializers.BooleanField(
required=False, label=_("enable knowledge base api"), help_text=_("enable knowledge base api")
)
enable_auth = serializers.BooleanField(required=False, label=_("enable auth"), help_text=_("enable auth"))
auth_config = serializers.JSONField(required=False, label=_("auth config"), help_text=_("auth config"))
enable_cors = serializers.BooleanField(required=False, label=_("enable cors"), help_text=_("enable cors"))
cross_domain_list = serializers.ListSerializer(
Expand All @@ -62,7 +61,7 @@ class PortalSerializer(serializers.Serializer):
class Model(serializers.ModelSerializer):
class Meta:
model = Portal
fields = "__all__"
exclude = ("enable_auth",)

@staticmethod
def _is_license_valid():
Expand Down Expand Up @@ -111,11 +110,12 @@ def one(self):
if portal is None:
raise AppApiException(500, _("Portal configuration does not exist"))
data = PortalSerializer.Model(portal).data
# 授权无效时不展示 auth_config，避免暴露认证配置
# 授权无效时不展示 auth_config 与 logo，避免暴露认证配置与品牌信息
if self._is_license_valid():
data["auth_config"] = self._enrich_auth_config(data["auth_config"])
else:
data["auth_config"] = {}
data["logo"] = ""
return data

def _upload_file(self, file_obj):
Expand Down Expand Up @@ -152,8 +152,9 @@ def edit(self, instance, with_valid=True):
if field == "auth_config" and isinstance(value, dict):
value = {**(portal.auth_config or {}), **value}
setattr(portal, field, value)
# Logo 为授权功能，授权无效时忽略 Logo 上传/修改
for field_name in ["logo"]:
if field_name in instance:
if field_name in instance and self._is_license_valid():
self._handle_file_field(portal, field_name, instance.get(field_name))
portal.save()
return PortalSerializer.Model(portal).data
Expand All @@ -166,13 +167,13 @@ def get_login_profile():
if portal is None:
raise AppApiException(500, _("Portal configuration does not exist"))
auth_config = portal.auth_config or {}
license_valid = PortalSerializer._is_license_valid()
return {
"name": portal.name,
"description": portal.description or "",
"logo": portal.logo or "",
"enable_auth": portal.enable_auth,
"authentication_type": auth_config.get("type", "LOCAL") if portal.enable_auth else "",
"login_value": auth_config.get("login_value", []) if portal.enable_auth else [],
"max_attempts": auth_config.get("max_attempts", 1) if portal.enable_auth else 1,
"logo": (portal.logo or "") if license_valid else "",
"authentication_type": auth_config.get("type", "LOCAL"),
"login_value": auth_config.get("login_value", []),
"max_attempts": auth_config.get("max_attempts", 1),
"rsa_key": get_key_pair_by_sql().get("key", ""),
}

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a review of the code changes, highlighting potential issues, logic flaws, and optimization suggestions.

1. Critical Logic Issue: Inconsistency between get_login_profile and Internal State

In get_login_profile, you removed the check for portal.enable_auth when deriving authentication_type, login_value, and max_attempts. You also removed enable_auth from the response entirely.

  • Problem: The frontend likely used enable_auth to decide whether to show authentication controls. By removing it but still returning authentication_type (e.g., "LOCAL") even when auth might be effectively disabled or unconfigured, you create ambiguity.
  • Risk: If auth_config is empty or invalid, auth_config.get("type", "LOCAL") returns "LOCAL". The frontend will see authentication_type: "LOCAL" and might assume authentication is enabled and configured, leading to login failures if the backend doesn't enforce it correctly elsewhere.
  • Suggestion: Either keep enable_auth in the response (but derive it carefully) or ensure authentication_type returns None/"DISABLED" explicitly if auth is not active/authorized.

2. Security/Logic Flaw: License Check Coupling

You coupled logo handling and auth_config exposure strictly to _is_license_valid(). However, you removed enable_auth from the serializer/model entirely (exclude = ("enable_auth,",)).

  • Problem: If a portal has enable_auth=False but an invalid license, the previous code would hide auth_config. The new code hides auth_config and logo based on license validity, regardless of the actual enable_auth state (since the field is gone from the model view).
  • Question: Is enable_auth still a column in the database?
    • If YES: Removing it from fields via exclude hides it from API responses, but it remains in the DB. This is fine if intentional.
    • If NO: You should remove the column from the DB schema as well.
    • Bigger Issue: The business rule now seems to be: "No license = No Logo & No Auth Config exposure." But what if a user wants to disable auth (enable_auth=False) with a valid license? The code still returns authentication_type: "LOCAL" because it reads directly from auth_config. The logic if portal.enable_auth else "" was removed.
    • Recommendation: Restore the conditional logic for authentication_type in get_login_profile to reflect whether auth is actually enabled, OR ensure that setting enable_auth=False clears/neutralizes auth_config in _enrich_auth_config or edit method.

3. Redundant/Unsafe File Handling

for field_name in ["logo"]:
    if field_name in instance and self._is_license_valid():
        self._handle_file_field(portal, field_name, instance.get(field_name))
  • Issue: Calling self._is_license_valid() twice (once here, once potentially in one() or other paths) is inefficient if it involves DB queries or complex checks.
  • Better Practice: Fetch license status once at the beginning of the method or cache it per request.
  • Logic: If the license is invalid, logo updates are silently ignored. Ensure this is the desired UX. Usually, users should get an error message ("Upgrade your license to upload logos") rather than a silent failure.

4. Serializer Model Configuration

class Meta:
    model = Portal
    exclude = ("enable_auth",)
  • Verification: Ensure enable_auth is indeed a legacy field that can be safely excluded. If this field is used in business logic elsewhere (e.g., middleware checking Portal.objects.first().enable_auth), removing it from the serializer won't break internal calls, but it breaks API consumers. Confirm API clients don't depend on enable_auth.

5. get_login_profile Optimization & Clarity

license_valid = PortalSerializer._is_license_valid() # Called here
# ...
"logo": (portal.logo or "") if license_valid else "",
  • Consistency: In the one() method, you clear data["logo"] = "" if license is invalid. In get_login_profile, you do the same. This is consistent.
  • Potential Bug: In get_login_profile, you access portal.auth_config directly from the DB object (portal.auth_config). Ensure this matches the enriched version returned by one(). Currently, one() enriches auth_config only if license is valid, otherwise sets it to {}. get_login_profile reads raw DB data. This could lead to inconsistency between /one and /login_profile endpoints regarding what auth_config content looks like.

6. General Suggestions

a. Extract License Check to Context/Decorator

Since _is_license_valid() is used in one, edit, and get_login_profile, consider using a decorator or middleware to inject license_valid into the view/request context to avoid repeated calls and improve performance.

b. Explicit Error Handling for Disabled Features

Instead of silently ignoring logo uploads when license is invalid, consider raising a specific exception:

if field_name in instance and field_name == "logo":
    if not self._is_license_valid():
        raise AppApiException(403, _("Logo feature requires a valid license"))
    self._handle_file_field(...)

c. Clean Up Removed Imports

Ensure that if enable_auth was imported from models or utilities specifically for this serializer/view, those unused imports are removed.

Summary of Recommended Fixes

  1. Restore Logic for Auth Type in get_login_profile:

    # Determine if auth is effectively active
auth_enabled = license_valid and portal.auth_config and portal.auth_config.get("type") != "DISABLED" # Or use legacy enable_auth if still exists in DB

return {
    # ...
    "authentication_type": auth_config.get("type", "") if auth_enabled else "",
    "login_value": auth_config.get("login_value", []) if auth_enabled else [],
    # ...
}

    Without this, you're returning config details even if the system shouldn't use them.

  2. Cache License Validity:
    Pass license_valid as a parameter or store it in the service class attribute during initialization if per-request.

  3. User Feedback:
    Add errors when users try to modify licensed features (logo, auth_config) without a valid license, instead of silent ignores.

  4. Verify Frontend Contract:
    Confirm the frontend no longer expects enable_auth or max_attempts when authentication_type is empty/disabled.

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code diff refactors the handling of enable_auth and logo fields in a Django REST Framework serializer/view, primarily to enforce licensing checks. Below is an analysis of potential issues, irregularities, and optimization suggestions.

1. Critical Logic Issue: Removal of enable_auth but Retention of Auth Data

  • Issue: The enable_auth field is removed from the Model Meta (via exclude) and deleted from the Serializer. However, in get_login_profile, the logic previously gated authentication_type, login_value, and max_attempts on portal.enable_auth. Now, these values are returned unconditionally (as long as license is valid, implied by context) or based only on the config itself.
  • Risk: If enable_auth was serving as the primary "Auth Enabled" switch in the database/logic now, removing it from the API response while still serving authentication_type might confuse the frontend. Does the frontend no longer check if auth is "on" before using these values? Ensure that auth_config.get("type", "LOCAL") defaults correctly if auth is implicitly expected to be active only when license is valid.

2. Security/Behavior Change in get_login_profile

  • Observation: Previously, if portal.enable_auth was False, authentication details were hidden or defaulted ("", [], 1). Now, they are returned directly from auth_config.
  • Potential Bug: If auth_config contains sensitive types (e.g., LDAP, OAuth) but enable_auth was effectively disabled via the old flag, exposing these details might be unintended. Since the flag is gone, ensure that auth_config["type"] reliably reflects the enabled state. If auth_config can exist even when auth is "disabled" by other means, this leak information.

3. Inconsistent License Check in edit vs. Other Methods

  • Code: 
    # In edit()
if field_name in instance and self._is_license_valid():
    self._handle_file_field(portal, field_name, instance.get(field_name))
    # In get_login_profile()
license_valid = PortalSerializer._is_license_valid()
# ...
"logo": (portal.logo or "") if license_valid else "",
  • Issue: In edit(), if the license is invalid, changes to logo are silently ignored. The user gets no feedback (no warning/error). This "silent failure" is poor UX. The frontend might think the update succeeded because the API returns status 200 and serialized data, but the persisted portal.logo remains unchanged.
  • Suggestion: Return a validation error or a specific message if the user attempts to upload/modify a logo without a valid license. At minimum, document this silent behavior clearly for frontend developers.

4. Repeated License Validation Calls

  • Optimization: _is_license_valid() appears to be called multiple times within single request cycles (e.g., potentially in edit flow if not optimized, and definitely in get_login_profile). If this method queries the database or performs expensive computations, cache the result per request/context.
  • Current State: In get_login_profile, it's calculated once (license_valid = PortalSerializer._is_license_valid()), which is good. Ensure consistency across all methods.

5. JSONField Merge Risk in edit

  • Code: 
    value = {**(portal.auth_config or {}), **value}
setattr(portal, field, value)
  • Issue: Shallow merge is generally fine for dictionaries, but ensure that nested dictionaries within auth_config are handled correctly if needed. Also, if value from the request contains unexpected keys due to lack of strict serialization (since you're iterating over instance dynamically), consider validating allowed keys for auth_config.

6. Logo Handling Inconsistency

  • Observation:
    • In one (GET): data["logo"] = "" if license invalid.
    • In edit: Changes to logo are ignored if license invalid.
    • In get_login_profile: "logo": ... if license_valid else "".
  • Suggestion: This is consistent. However, verify what happens in the Model(portal).data serialization in edit. PortalSerializer.Model(portal) includes the logo field because exclude only removes enable_auth. If portal has a logo saved (from a previous valid license period) and then the license expires, edit won't change it, but one() will return "" for logo because of the explicit override: 
    data = PortalSerializer.Model(portal).data # Contains original logo from DB
# ...
if self._is_license_valid():
    pass # keeps logo in data
else:
    data["auth_config"] = {}
    data["logo"] = "" # Explicitly clears it in return object only
    • Check: Is the intent to never expose the logo if license is invalid, even if it exists in DB? Yes, likely. But note that the one method modifies the returned dictionary, not the model instance. This is correct behavior for security, but ensure portal.save() elsewhere isn't accidentally clearing the DB logo when license becomes invalid (currently edit avoids saving logo changes if invalid, which is good).

7. Naming Convention & Clarity

  • self._handle_file_field(portal, field_name, instance.get(field_name)) assumes _handle_file_field updates the portal instance in memory. Ensure this method actually sets portal.logo (or equivalent) so that portal.save() persists it only when conditions are met. The current code relies on _is_license_valid() blocking the call entirely, which leaves portal.logo untouched. This is safe.

Summary of Recommendations

  1. Frontend Coordination: Confirm frontend no longer expects enable_auth field and that authentication_type presence is sufficient to determine auth UI behavior.
  2. Explicit Error on Invalid License Upload: Modify edit to raise a ValidationError or similar exception if field_name == "logo" and _is_license_valid() is False, providing clearer feedback than silent ignoring.
  3. Validate auth_config Keys: Consider restricting mergeable keys in auth_config to avoid malicious injection or configuration drift.
  4. Consistent Default in get_login_profile: Ensure auth_config.get("type", "LOCAL") is correct. If auth_config is empty {}, it returns "LOCAL". Is this desired? Previously it was "" if enable_auth was false. Now it might always say "LOCAL" unless overridden, which might imply "always local auth available". Verify this is the intended business logic change.

Optimized Code Suggestion for edit Warning (Optional Enhancement)

If you want to provide feedback:

        for field_name in ["logo"]:
            if field_name in instance:
                if not self._is_license_valid():
                    # Optionally raise exception here depending on product requirements
                    # raise ValidationError(_("Logo modification requires a valid license."))
                    pass # Current behavior: silently ignore
                else:
                    self._handle_file_field(portal, field_name, instance.get(field_name))

1 change: 0 additions & 1 deletion apps/portal/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,4 @@

urlpatterns = [
path("portal", views.PortalView.as_view()),
path("portal/info", views.PortalInfoView.as_view()),
]
13 changes: 0 additions & 13 deletions apps/portal/views/portal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
from portal.api.portal import PortalAPI
from portal.serializers.portal import (
PortalSerializer,
PortalLoginSerializer,
)


Expand Down Expand Up @@ -55,15 +54,3 @@ def get(self, request: Request):
@has_permissions(PermissionConstants.PORTAL_EDIT, RoleConstants.ADMIN)
def put(self, request: Request):
return result.success(PortalSerializer(data=request.data).edit(request.data))


class PortalInfoView(APIView):
@extend_schema(
methods=["GET"],
description=_("Get portal login info"),
summary=_("Get portal login info"),
operation_id=_("Get portal login info"), # type: ignore
tags=[_("Portal")], # type: ignore
)
def get(self, request: Request):
return result.success(PortalLoginSerializer.get_login_profile())

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a review of the provided code diff:

1. Irregularities & Potential Issues

  • Unused Import Removal: The PortalLoginSerializer import has been removed. This is correct and necessary because the only view using it (PortalInfoView) has also been deleted. No issue here.
  • Dead Code Cleanup: The removal of PortalInfoView cleanly removes an endpoint. If this endpoint was still referenced in frontend routing, documentation, or tests elsewhere, those references must be updated/removed simultaneously, or they will cause broken links/test failures.
  • No Logic Changes to Remaining Code: The get and put methods for the remaining class are untouched. This suggests the patch is purely a deletion/refactoring cleanup.

2. Optimization Suggestions

  • Verify Test Coverage: Ensure that unit/integration tests referencing PortalInfoView or PortalLoginSerializer.get_login_profile() are removed or updated. Leaving orphaned tests will lead to CI fails.
  • API Documentation Sync: If drf-spectacular or similar tools are used, ensure that OpenAPI/Swagger specs reflect this new endpoint removal automatically (usually handled by rebuilds, but worth confirming).
  • Frontend Coordination: Confirm with frontend teams that no client-side code calls the now-deleted /portal/info/ (or equivalent) endpoint.

3. Summary

✅ Clean Deletion Patch: The diff is syntactically correct and logically consistent.
⚠️ External Dependencies Risk: Ensure all consumers (tests, frontend, docs) are aware of this endpoint’s removal.

No code-level bugs detected in the snippet itself.

2 changes: 0 additions & 2 deletions apps/users/serializers/login.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,6 @@ def chat_generate(username: str, type: str = "chat", access_token: str = ""):
@staticmethod
def portal_generate(username: str, type: str = "portal"):
portal = Portal.objects.first()
if portal is None or not portal.enable_auth:
return {"captcha": ""}
auth_config = portal.auth_config or {}
max_attempts = auth_config.get("max_attempts", 1)
key = f"{type}_{username}"
Expand Down
2 changes: 1 addition & 1 deletion ui/src/api/chat/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const getDingTalkOauthCallback = (code: string, accessToken?: string) => {
* 获取应用登录验证码；无需验证码时返回空字符串。
* 后端以 `accessToken` 查询参数接收应用令牌，且仅支持应用场景。
*/
const getCaptcha = (username: string, accessToken: string) => {
const getCaptcha = (username: string, accessToken?: string) => {
return get<ChatCaptchaResponse>('/v3/captcha', { username, accessToken })
}

Expand Down
1 change: 1 addition & 0 deletions ui/src/api/types/chat-auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export interface AuthProfile<Meta = Record<string, never>> {
max_attempts: number
enable_auth: boolean
login_value: LoginMethod[]
default_value?: LoginMethod
rsa_key: string
logo: string
custom_theme?: { theme_color?: string; header_font_color?: string } | null
Expand Down
19 changes: 11 additions & 8 deletions ui/src/views/chat/login/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { computed, onBeforeMount, ref } from 'vue'
import { ArrowLeft } from '@element-plus/icons-vue'
import { getDingTalkClientAuthCode } from '@/views/login/dingtalk-client-login'
import { useRoute, useRouter, type RouteLocationRaw } from 'vue-router'
import { type RouteLocationRaw, useRoute, useRouter } from 'vue-router'
import { v7 as uuidv7 } from 'uuid'
import ChatAuthApi from '@/api/chat/auth'
import { CHAT_API_BASE_PATH } from '@/api/constants'
Expand All @@ -25,11 +25,15 @@ const loginTitle = computed(() => {
return (auth.authProfile?.meta as ApplicationAuthProfileMeta | undefined)?.application_name || '登录'
})

// 认证前置配置只有可用登录方式，默认登录方式依次取本地、LDAP、第一个可用方式
// 认证前置配置只有可用登录方式，默认登录方式优先取后端配置，再依次回退本地、LDAP、第一个可用方式
const loginConfig = computed<LoginConfig>(() => {
const loginMethods = auth.authProfile?.login_value?.length ? auth.authProfile.login_value : [LOGIN_METHOD.LOCAL]
const configuredDefault = auth.authProfile?.default_value
const defaultLoginMethod =
[LOGIN_METHOD.LOCAL, LOGIN_METHOD.LDAP].find((method) => loginMethods.includes(method)) ?? loginMethods[0] ?? LOGIN_METHOD.LOCAL
(configuredDefault && loginMethods.includes(configuredDefault) ? configuredDefault : undefined) ??
[LOGIN_METHOD.LOCAL, LOGIN_METHOD.LDAP].find((method) => loginMethods.includes(method)) ??
loginMethods[0] ??
LOGIN_METHOD.LOCAL
return { default_value: defaultLoginMethod, login_methods: loginMethods, max_attempts: auth.authProfile?.max_attempts ?? 1 }
})

Expand Down Expand Up @@ -75,7 +79,7 @@ const getExternalLoginUrl = (authType: LoginMethod): Promise<string> => {
if (!config.authEndpoint || !config.clientId) return ''
const redirectUrl = `${config.redirectUrl}${queryString ? `?${queryString}` : ''}`
const state = accessToken ? `${uuidv7()}_${accessToken}` : uuidv7()
let url = `${config.authEndpoint}?client_id=${config.clientId}&response_type=code&redirect_uri=${redirectUrl}&state=${state}`
let url = `${config.authEndpoint}?client_id=${config.clientId}&response_type=code&redirect_uri=${config.redirectUrl}&state=${state}`
if (config.scope) url += `&scope=${config.scope}`
return url
})
Expand All @@ -102,11 +106,10 @@ const loginService: LoginService = {
},
login: (loginRequest) => auth.login(loginRequest),
ldapLogin: (loginForm) => auth.ldapLogin(loginForm),
// 验证码：后端按失败次数决定是否需要，无需时返回空字符串；目前仅应用场景支持
// 验证码：后端按失败次数决定是否需要，无需时返回空字符串
getCaptcha: (username) => {
const { accessToken } = auth
if (!accessToken || !username.trim()) return Promise.resolve('')
return ChatAuthApi.getCaptcha(username, accessToken).then(({ captcha }) => captcha)
if (!username.trim()) return Promise.resolve('')
return ChatAuthApi.getCaptcha(username, auth.accessToken).then(({ captcha }) => captcha)
},
getExternalLoginUrl,
getQrCodeSources: () => ChatAuthApi.getQrCodeSources(),

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a code review focusing on the changes in the provided diff.

1. Critical Security Bug: Missing URL Encoding

Location: getExternalLoginUrl function (Line 79 context)

-    let url = `${config.authEndpoint}?client_id=${config.clientId}&response_type=code&redirect_uri=${redirectUrl}&state=${state}`
+    let url = `${config.authEndpoint}?client_id=${config.clientId}&response_type=code&redirect_uri=${config.redirectUrl}&state=${state}`

Issue:
The change appears to accidentally discard the logic that appends queryString to redirectUrl.

  • The variable redirectUrl was constructed as: ${config.redirectUrl}${queryString ? ?${queryString} : ''}.
  • The new code uses config.redirectUrl directly, ignoring queryString.
  • More importantly, redirect_uri, state, and client_id are not encoded. If these values contain special characters (e.g., &, #, ?), this will break the OAuth2 request or introduce security vulnerabilities (parameter injection).

Recommendation:

  1. Restore the use of redirectUrl if queryString is intended to be part of the redirect target.
  2. Mandatory: Use encodeURIComponent for all query parameters.
const redirectUrl = config.redirectUrl + (queryString ? `?${queryString}` : '');
let url = `${config.authEndpoint}?client_id=${encodeURIComponent(config.clientId)}&response_type=code&redirect_uri=${encodeURIComponent(redirectUrl)}&state=${encodeURIComponent(state)}`;
if (config.scope) url += `&scope=${encodeURIComponent(config.scope)}`;

2. Logic Regression in default_value Handling

Location: loginConfig computed property

const configuredDefault = auth.authProfile?.default_value
const defaultLoginMethod =
    (configuredDefault && loginMethods.includes(configuredDefault) ? configuredDefault : undefined) ??
    [LOGIN_METHOD.LOCAL, LOGIN_METHOD.LDAP].find(...) ??
    ...

Analysis:
The logic a ? b : undefined ?? c is valid but can be misleading due to operator precedence. However, the real issue is type safety and robustness.

  • If configuredDefault is an empty string "", it is falsy, so it falls back to hardcoded defaults. This might be intentional, but ensure backend never returns "" when a value should be prioritized over fallbacks.
  • The original comment said "default takes local, LDAP, first available". The new comment says "backend config priority".
  • Potential Issue: If auth.authProfile.default_value contains a method that is no longer in login_value (e.g., stale config), the check loginMethods.includes(configuredDefault) correctly rejects it. This is good.

Optimization Suggestion:
The chain is slightly verbose. You could extract this into a helper function for readability, but it is acceptable as-is. No logical bug detected, just ensure configuredDefault is strictly typed against LOGIN_METHOD.

3. Reactivity Issue in getCaptcha

Location: getCaptcha function within loginService

// Before
const { accessToken } = auth
if (!accessToken || !username.trim()) return Promise.resolve('')
return ChatAuthApi.getCaptcha(username, accessToken)...

// After
if (!username.trim()) return Promise.resolve('')
return ChatAuthApi.getCaptcha(username, auth.accessToken)...

Issue:

  • Before: The destructuring const { accessToken } = auth likely caused Vue to track accessToken as a dependency during the function execution phase (if auth is reactive). However, since getCaptcha is called explicitly (not inside a computed/watch), this reactivity tracking was irrelevant unless used inside a reactive context.
  • After: Accessing auth.accessToken directly ensures the latest value is used at call time.
  • Risk Removed: The previous code captured a stale accessToken if auth.accessToken changed between the destructure and the promise creation (micro-tasks). The new version is safer because it reads the current value at invocation time.
  • Remaining Risk: There is no guard for auth.accessToken being null/undefined here. The API call ChatAuthApi.getCaptcha might fail if accessToken is undefined.

Recommendation:
Consider adding a guard for accessToken if it is required by the backend, returning an empty string or error if missing, to avoid unexpected API errors.

getCaptcha: (username) => {
  if (!username.trim()) return Promise.resolve('');
  // If accessToken is required, add this guard:
  // if (!auth.accessToken) return Promise.reject(new Error('No access token')); 
  return ChatAuthApi.getCaptcha(username, auth.accessToken).then(({ captcha }) => captcha);
},

4. Import Order Minor Style

Change:

-import { useRoute, useRouter, type RouteLocationRaw } from 'vue-router'
+import { type RouteLocationRaw, useRoute, useRouter } from 'vue-router'

Analysis:
This is purely stylistic. Some linters enforce order (e.g., types first, or alphabetical). Ensure your ESLint/prettier configuration is consistent with placing type imports first if that’s the convention chosen.

Summary of Actions Required

  1. [CRITICAL] Fix getExternalLoginUrl: Revert to using redirectUrl (which includes queryString) and add encodeURIComponent to all parameters. The current diff breaks functionality and security.
  2. [REVIEW] getCaptcha: Verify if auth.accessToken can be null when this is called. If yes, handle it gracefully.
  3. [OK] loginConfig: Logic seems correct for prioritizing backend config. Ensure default_value type is strictly checked against LOGIN_METHOD.

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a code review focusing on irregularities, potential issues, and optimization suggestions.

1. Critical Issue: redirect_uri Mismatch (Potential Bug)

Location: getExternalLoginUrl function.
Change:

// Old
let url = `${config.authEndpoint}?...&redirect_uri=${redirectUrl}&state=${state}`
// New
let url = `${config.authEndpoint}?...&redirect_uri=${config.redirectUrl}&state=${state}`

Analysis:

  • Strict OAuth2 Compliance: Most OAuth2 providers require the redirect_uri in the authorization request to exactly match the registered URI or the one used in the subsequent token exchange.
  • The Risk: The variable redirectUrl included query parameters (?${queryString}). By changing it to config.redirectUrl (which likely does not include these dynamic parameters), you risk:
    1. OAuth Failure: The provider might reject the request if config.redirectUrl doesn't exactly match what was stored or expected during registration.
    2. Loss of State/Context: If queryString contained important navigation state (e.g., ?target=/dashboard), removing it from redirect_uri means the user will lose context after login unless that state is entirely handled via the state parameter or cookies.
  • Recommendation: Verify if config.redirectUrl is sufficient for your OAuth provider's validation rules. If not, revert to using redirectUrl but ensure it is URL-encoded if necessary (though usually, the base redirect URI must match strictly). If the change was intentional to fix a mismatch error, ensure the token exchange endpoint also uses this same config.redirectUrl.

2. Security Concern: Captcha Logic Reduction

Location: getCaptcha method.
Change:

// Old
if (!accessToken || !username.trim()) return Promise.resolve('')
return ChatAuthApi.getCaptcha(username, accessToken).then(({ captcha }) => captcha)

// New
if (!username.trim()) return Promise.resolve('')
return ChatAuthApi.getCaptcha(username, auth.accessToken).then(({ captcha }) => captcha)

Analysis:

  • Loss of Early Exit: The original code skipped the API call if accessToken was missing. This is a logical short-circuit because CAPTCHAs are typically required for unauthenticated users to prevent brute force. However, by always calling ChatAuthApi.getCaptcha even when auth.accessToken is undefined, you make an unnecessary network request.
  • API Contract: If ChatAuthApi.getCaptcha throws an error or returns unexpected data when passed an undefined access token, this could cause UI bugs.
  • Recommendation: Keep the early exit for performance and stability. 
    getCaptcha: (username) => {
  if (!username.trim()) return Promise.resolve('');
  // Optional: Still check if accessToken is required by your specific API implementation
  // If the API handles no-token gracefully, this is fine, but checking locally avoids a roundtrip.
  return ChatAuthApi.getCaptcha(username, auth.accessToken).then(({ captcha }) => captcha);
}
    Note: If the backend explicitly supports generating a captcha before any auth context exists, then the new logic is correct, but consider adding a type guard or default value for auth.accessToken to avoid passing undefined if it breaks the API serialization.

3. Code Style & Import Order

Location: Top import block.
Change: import { useRoute, useRouter, type RouteLocationRaw } from 'vue-router' → import { type RouteLocationRaw, useRoute, useRouter } from 'vue-router'
Analysis:

  • Irregularity: While functional, standard ESLint rules (like eslint-plugin-import) often enforce alphabetical order or separate "type" imports. Moving type ... to the front is acceptable but ensure your team's linter configuration accepts it. Some projects prefer putting types last to distinguish runtime values from compile-time types.
  • Suggestion: No functional issue, just ensure consistency with the rest of the codebase.

4. Logic Clarity: Default Login Method

Location: loginConfig computed property.
Change: Added priority for auth.authProfile?.default_value.
Analysis:

  • Improvement: This is a good enhancement. It respects server-side configuration first, which is more flexible.
  • Complexity: The chain of nullish coalescing (??) is becoming long and slightly hard to read: 
    const defaultLoginMethod =
  (configuredDefault && loginMethods.includes(configuredDefault) ? configuredDefault : undefined) ??
  [LOGIN_METHOD.LOCAL, LOGIN_METHOD.LDAP].find((method) => loginMethods.includes(method)) ??
  loginMethods[0] ??
  LOGIN_METHOD.LOCAL
  • Optimization/Readability Suggestion: Breaking this into a helper function or clearer if/else blocks might improve maintainability, especially if more fallbacks are added later. However, for now, it is concise and functional. Ensure configuredDefault type matches LOGIN_METHOD enum/string exactly to avoid subtle inclusion bugs.

Summary of Recommendations

  1. Double-check OAuth redirect_uri: Confirm that dropping the queryString from redirect_uri does not break authentication flows or cause provider mismatches.
  2. Protect Captcha API Call: Consider reverting or safeguarding the getCaptcha call when accessToken is missing, unless the API is explicitly designed to handle it robustly.
  3. Test Edge Cases: Specifically test the loginConfig logic when:
    • Backend sends a default_value that is NOT in login_methods.
    • Backend sends NO default_value.
    • login_methods is empty.

Expand Down
12 changes: 9 additions & 3 deletions ui/src/views/system/chat-management/portal-setting/components/ButtonEditPortal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,13 @@ import type { FormInstance, FormRules } from 'element-plus'
import type { PortalSetting } from '@/api/types'
import MkEditAvatar from '@/components/mk-edit-avatar/index.vue'

const props = defineProps<{ disabled: boolean; setting: PortalSetting; saving: boolean; save: (payload: FormData) => Promise<void> }>()
const props = defineProps<{
disabled: boolean
logoDisabled?: boolean
setting: PortalSetting
saving: boolean
save: (payload: FormData) => Promise<void>
}>()

/* 门户名称与 Logo */
const editVisible = ref(false)
Expand Down Expand Up @@ -35,7 +41,7 @@ function handleSavePortalInfo() {
if (!valid || props.disabled || props.saving) return
const payload = new FormData()
payload.append('name', portalForm.name.trim())
if (logoChanged.value) payload.append('logo', logoFile.value || '')
if (logoChanged.value && !props.logoDisabled) payload.append('logo', logoFile.value || '')
return props.save(payload).then(() => {
editVisible.value = false
})
Expand Down Expand Up @@ -68,7 +74,7 @@ function handleClosePortalEdit() {
>
<el-form-item label="名称" prop="name">
<div class="flex-align-center w-full gap-3">
<MkEditAvatar v-model="portalForm.logo" @change="handleLogoChange" :size="32">
<MkEditAvatar v-model="portalForm.logo" @change="handleLogoChange" :size="32" :disabled="logoDisabled">
<template #default="{ icon }">
<PortalIcon :icon="icon" />
</template>
Expand Down
13 changes: 10 additions & 3 deletions ui/src/views/system/chat-management/portal-setting/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ onMounted(() => loadPortalSetting())

// 权限 控制
const canEditPortal = computed(() => perm.system.portal.edit())
// Logo 为授权功能，授权无效（含社区版）时仅可查看，禁止上传或更换 Logo
const canEditLogo = computed(() => perm.system.portal.edit() && (auth.isPE || auth.isEE))
</script>

<template>
Expand All @@ -118,7 +120,13 @@ const canEditPortal = computed(() => perm.system.portal.edit())
<h4 class="truncate" :title="previewName">{{ previewName }}</h4>
</div>
<!-- 编辑门户名称与 Logo -->
<ButtonEditPortal :setting="portalSetting" :saving="saving" :disabled="!canEditPortal" :save="savePortalSetting" />
<ButtonEditPortal
:setting="portalSetting"
:saving="saving"
:disabled="!canEditPortal"
:logo-disabled="!canEditLogo"
:save="savePortalSetting"
/>
</div>
</el-card>
<el-card shadow="never">
Expand Down Expand Up @@ -197,12 +205,11 @@ const canEditPortal = computed(() => perm.system.portal.edit())
/>
</div>

<div class="flex-between gap-2">
<div class="flex-between gap-2" v-if="auth.isPE || auth.isEE">
<span>登录设置</span>
<div class="flex-align-center gap-2">
<!-- 配置登录设置 -->
<ButtonPortalAuthSetting
v-if="auth.isPE || auth.isEE"
ref="authSettingButtonRef"
:setting="portalSetting"
:saving="saving"

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a review of the provided code snippet, focusing on logic, consistency, and potential issues.

1. Logic & Consistency Issues

A. Inconsistent Permission Logic for "Login Settings"

  • Issue: You modified canEditLogo to include an enterprise/community check (auth.isPE || auth.isEE). However, you did not update the visibility or editability of the "Login Settings" (登录设置) section based on this same logic in a clear way relative to editing rights.
    • Previously: The button was hidden entirely if not PE/EE (v-if="auth.isPE || auth.isEE").
    • Now: The <div> container is hidden if not PE/EE. This means users without an enterprise license cannot even see the login settings UI, let alone view them.
  • Comparison with Logo: For the Logo, you allowed viewing (ButtonEditPortal is rendered but logo-disabled is true). For Login Settings, you completely hide the UI. This is an inconsistent UX pattern.
    • Recommendation: If the policy is "View-only for Community," you should show the Login Settings UI but disable the button (similar to how you handle logo-disabled). If the policy is "Feature Hidden," then your current change is fine, but ensure it aligns with other feature-gating strategies in your app.

B. Redundant auth.isPE || auth.isEE Check

  • You introduced const canEditLogo = computed(() => perm.system.portal.edit() && (auth.isPE || auth.isEE)).
  • Later, you manually check v-if="auth.isPE || auth.isEE" for the Login settings div.
  • Suggestion: Consider creating a single computed property for "Is Enterprise Feature Enabled" if used frequently, e.g., const isEnterprise = computed(() => auth.isPE || auth.isEE). This improves readability and maintainability.

2. Code Quality & Optimization

A. Prop Naming Clarity

  • :logo-disabled="!canEditLogo" is clear, but ensure the ButtonEditPortal component internally handles the case where disabled is false but logo-disabled is true correctly (i.e., allowing text edits but blocking logo changes). This looks good from the outside, but verify the child component implementation.

B. Template Structure

  • Moving v-if from the child component (ButtonPortalAuthSetting) to the parent container (<div>) is acceptable. It prevents rendering the container entirely. However, as noted above, consider if you want to show a disabled state instead.

3. Potential Bugs

  • Null Safety: Ensure auth and perm are properly injected/defined before computed executes. In Vue Composition API, if auth is reactive and initialized asynchronously, accessing .isPE directly might throw if auth starts as undefined or null. Assuming standard Pinia/Vuex patterns where these are initialized synchronously or safely accessed, this is likely fine. If not, use optional chaining: (auth?.isPE || auth?.isEE).

Summary of Recommendations

  1. Harmonize Feature Gating: Decide whether "Community Edition" users should not see Premium features (hide UI) or see but not edit (disable UI). Apply one pattern consistently. Currently:
    • Logo: Visible but disabled.
    • Login Settings: Hidden.
  2. Extract Repeated Condition: Use a shared computed property for auth.isPE || auth.isEE.
  3. Verify Child Component: Confirm ButtonEditPortal correctly interprets logo-disabled independently of the main disabled prop.

Improved Code Suggestion (for consistency)

// In <script>
const isEnterprise = computed(() => auth.isPE || auth.isEE)
const canEditPortal = computed(() => perm.system.portal.edit())
const canEditLogo = computed(() => canEditPortal.value && isEnterprise.value)
<!-- In <template> -->
<!-- Option A: Hide Login Settings for Non-Enterprise (Current Behavior) -->
<div class="flex-between gap-2" v-if="isEnterprise"> ... </div>

<!-- OR Option B: Show but Disable (Consistent with Logo) -->
<div class="flex-between gap-2">
  <span>登录设置</span>
  <ButtonPortalAuthSetting
    :disabled="!isEnterprise || !canEditPortal" 
    ... 
  />
</div>

Expand Down
Loading