Skip to content

fix: chat user login #7803

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

Closed
shaohuzhang1 wants to merge 1 commit into from
Closed

fix: chat user login #7803

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
2 changes: 1 addition & 1 deletion apps/chat/serializers/chat_user_serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def _get_login_policy(cls, access_token):
auth_setting.get("lock_time", 10),
)
portal = Portal.objects.first()
if portal is None or not portal.enable_auth:
if portal is None:
return ("portal", 1, 5, 10)
auth_config = portal.auth_config or {}
return (

@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 the review of the code diff provided:

1. Logical/Security Irregularity (Critical)

Issue: Removing or not portal.enable_auth from the validation check.

  • Before: If the Portal exists but authentication is disabled (enable_auth = False), the system would fall back to default values or a different logic path (depending on what was elsewhere, but here it returned defaults).
  • After: The function proceeds to use portal.auth_config even if portal.enable_auth is False.
  • Risk: This likely enables brute-force protection or login throttling settings for a portal where authentication is explicitly disabled. This is semantically incorrect and potentially a security regression. Login policies should generally only apply when authentication is active.

2. Code Quality & Readability

Issue: Unnecessary space before colon.

  • Line: if portal is None :
  • Suggestion: Standard Python style (PEP 8) requires no space before the colon in an if statement.
    • Fix: if portal is None:

3. Ambiguity / Logic Flow

Issue: Incomplete visibility of the fallback logic.

  • The return statement after this block is cut off (return (). It’s unclear what happens if auth_config lacks keys or if enable_auth is true/false. Ensure that when enable_auth is false, the function does not proceed to read auth_config unless intended.

Recommendation

Revert the semantic change unless you have a specific reason to allow lockout policies on non-authenticating portals. If enable_auth=False, you should likely still return defaults or skip policy application.

If you must keep this change, fix the style:

-        if portal is None :
+        if portal is None:

But seriously, restore the enable_auth check unless it's a deliberate and well-documented behavior change.

@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 the review of the code snippet provided:

1. Potential Logic Issue (High Priority)

Regularity: The logic change might be intentional, but it introduces a potential bypass security risk.

  • Original Code: if portal is None or not portal.enable_auth:
    • Explicitly checks if authentication is disabled (enable_auth is False). If disabled, it returns a default/insecure policy tuple.
  • New Code: if portal is None :
    • Removes the check for portal.enable_auth.
    • Implication: If portal exists but enable_auth is False (i.e., public access or unsecured mode intended), this code will now proceed into the authentication logic instead of short-circuiting. This could lead to authentication failures, null reference errors later on, or unintended enforcement of auth policies when they should be bypassed.

2. Style & Formatting

  • Whitespace: There is extra whitespace before the colon in the condition: if portal is None :
    • Fix: Change to if portal is None: for PEP8 compliance.

3. Optimization / Readability Suggestions

  • Explicit Boolean Check: While not strictly required, consider explicit boolean handling if enable_auth is optional or nullable elsewhere in the model. 
    # If you intended to keep the original logic but fix formatting:
if not portal or not portal.enable_auth:
    return ("portal", 1, 5, 10)
    Note: if not portal is generally preferred over if portal is None for Django models unless there's a specific case where an empty object is valid but falsy checks cause issues.

Recommendations

Option A: Fix Formatting Only (If business logic requires ignoring enable_auth)

If this change was deliberate to ignore enable_auth, only fix the style:

-        if portal is None :
+        if portal is None:

Option B: Restore Original Safety (Recommended)

If the intention is to ensure that when enable_auth is off, the default policy is returned (as per the original code):

-        if portal is None :
+        if not portal or not portal.enable_auth:
             return ("portal", 1, 5, 10)

Summary

  • Bug Risk: High. Removing not portal.enable_auth likely breaks expected behavior for unauthenticated/public contexts.
  • Style: Minor whitespace error.
  • Action: Confirm with stakeholders if removing the enable_auth check was part of a feature requirement (e.g., "always enforce auth policy even if flag says disabled"). If not, revert the logical change and fix the whitespace.

@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.

The diff is syntactically valid, but it introduces a behavioral/security change that should be reviewed carefully.

1. Main issue: enable_auth is now ignored

Original:

if portal is None or not portal.enable_auth:
    return ("portal", 1, 5, 10)

New:

if portal is None:
    return ("portal", 1, 5, 10)

This means that when a portal exists but portal.enable_auth == False, the function no longer returns the restrictive/default policy ("portal", 1, 5, 10). Instead, it continues and uses portal.auth_config.

Potential risks:

  • If enable_auth=False is intended to disable authentication or apply a safe fallback policy, this change may weaken login protection or cause lockout behavior changes.
  • If auth_config is empty or partially configured, the function may return weaker/incorrect policy values.
  • If enable_auth is deprecated, the field and its semantics should be removed/documented, and migration behavior must be verified.
  • If enable_auth is a feature flag, removing it from this check may cause authentication policy to be applied when it should not be.

2. Missing test coverage

Add tests for at least these cases:

  • portal is None
  • portal exists, enable_auth=False, auth_config={}
  • portal exists, enable_auth=False, auth_config with custom values
  • portal exists, enable_auth=True, auth_config={}
  • portal exists, enable_auth=True, auth_config with custom values

The key regression test is the enable_auth=False case.

3. Pre-existing concerns

These are not caused by the diff, but are worth checking:

  • Portal.objects.first() is non-deterministic if multiple portals exist. Consider filtering by the relevant portal/domain or ordering explicitly.
  • portal.auth_config or {} protects against falsy values, but not against truthy non-dict values. If auth_config could be a list/string, later .get() calls may fail.
  • The tuple values 1, 5, 10 are magic numbers; consider named constants.
  • If portal configuration can change concurrently, consider whether a cached or transactional read is needed.

Recommendation

If the removal of not portal.enable_auth is intentional, document why. Otherwise, restore it:

if portal is None or not portal.enable_auth:
    return ("portal", 1, 5, 10)

Also verify that auth_config always contains safe defaults when enable_auth=False.

Expand Down
6 changes: 3 additions & 3 deletions ui/src/conversation-panel/left-sidebar/account-menu/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ const displayName = computed(() => account.value?.nickName || account.value?.use
// 配额尚未接入时保留空值展示，不将缺失数据表示为零用量。
const tokenPercentage = computed(() => {
const quota = account.value?.token_quota
if (!quota) return 0
return Math.min(100, Math.max(0, (quota.used_tokens / quota.total_tokens) * 100))
if (!quota || quota.quota_type === QUOTA_TYPE.UNLIMITED || !quota.token_limit) return 0
return Math.min(100, Math.max(0, (quota.used_tokens / quota.token_limit) * 100))
})

const userPwdDialogRef = useTemplateRef<InstanceType<typeof UserPwdDialog>>('userPwdDialogRef')
Expand Down Expand Up @@ -71,7 +71,7 @@ const goLogin = () => {
<span class="mr-3">Tokens</span>
<span class="text-primary">{{ formatTokenNumber(account?.token_quota?.used_tokens) }}</span>
<span class="text-N500">
/{{ account?.token_quota?.quota_type === QUOTA_TYPE.UNLIMITED ? '不限' : formatTokenNumber(account?.token_quota?.total_tokens) }}
/{{ account?.token_quota?.quota_type === QUOTA_TYPE.UNLIMITED ? '不限' : formatTokenNumber(account?.token_quota?.token_limit) }}
</span>
</div>

@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 an analysis of the code changes, including potential issues and optimization suggestions:

1. Logical Consistency and Data Integrity

  • Field Consistency: You changed the denominator from total_tokens to token_limit. Ensure that token_limit is indeed the correct field name in your account.value?.token_quota object structure. If total_tokens was previously used for display but token_limit is for calculation, verify if this inconsistency exists elsewhere in the UI (though you also updated the template to use token_limit, which shows good consistency).
  • Undefined Check: The condition !quota.token_limit is crucial here. If token_limit can legitimately be 0, !quota.token_limit will evaluate to true, returning 0%. While a quota of 0 usually implies no usage allowed, ensure this aligns with business logic. If token_limit could be undefined or null, the check works correctly. If it's specifically 0, verify if users should see "0%" or if 0 is treated as "no data/uninitialized".

2. Code Clarity and Robustness

  • Explicit Comparison: The condition !quota.token_limit mixes falsy checks. It's often clearer to check specifically for validity if 0 has a distinct meaning different from undefined. However, given the context of quotas, if token_limit is missing or zero, showing 0% usage is generally safe and prevents NaN errors in the division.
  • Readability: The line: 
    if (!quota || quota.quota_type === QUOTA_TYPE.UNLIMITED || !quota.token_limit) return 0
    is concise but slightly busy. Consider breaking it down if the team prefers readability over conciseness: 
    const tokenPercentage = computed(() => {
  const quota = account.value?.token_quota;
  
  // Return 0 if no quota info, unlimited type, or invalid limit
  if (!quota || quota.quota_type === QUOTA_TYPE.UNLIMITED || !quota.token_limit) {
    return 0;
  }
  
  const percent = (quota.used_tokens / quota.token_limit) * 100;
  return Math.min(100, Math.max(0, percent));
});

3. Edge Case Handling

  • Division by Zero/Infinity: The new check !quota.token_limit effectively prevents division by zero because:
    • If token_limit is 0, the condition is true, returning 0.
    • If token_limit is undefined/null, the condition is true, returning 0.
    • This is a robust improvement over the previous version which relied on quota itself being present but didn't strictly guard against total_tokens being 0/undefined.
  • Negative Values: If used_tokens or token_limit can somehow become negative due to backend errors, Math.max(0, ...) handles the lower bound, and Math.min(100, ...) handles the upper bound, so the displayed percentage stays within [0, 100]. This is good.

4. Template Change Analysis

In the template:

/{{ account?.token_quota?.quota_type === QUOTA_TYPE.UNLIMITED ? '不限' : formatTokenNumber(account?.token_quota?.token_limit) }}
  • Consistency: You correctly updated the denominator display to match the calculation source (token_limit).
  • Null Safety: Optional chaining (?.) is used correctly. If token_limit is undefined, formatTokenNumber(undefined) might render incorrectly depending on how that function is implemented. Ensure formatTokenNumber gracefully handles null/undefined/0 inputs (e.g., returning '-' or '0' instead of crashing or rendering NaN).

Summary of Recommendations

  1. Verify Field Name: Confirm token_limit is the standard API field name replacing total_tokens.
  2. Refactor for Readability (Optional): Split the conditional check in the computed property for better maintainability.
  3. Check formatTokenNumber: Ensure it handles undefined or 0 gracefully when called in the template, as token_limit might be absent in some states.
  4. No Critical Bugs Found: The change improves safety by preventing division by zero/undefined errors, assuming token_limit is the correct data field.

@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 an analysis of the provided code diff, highlighting potential issues and optimization suggestions.

1. Critical Issue: Inconsistent Data Consistency & "Unlimited" Representation in Percentage

Problem:
The tokenPercentage computed property returns 0 when the quota is UNLIMITED or if token_limit is missing.

  • Logical Flaw: If the quota is unlimited, showing "0%" usage might be misleading (could imply no usage vs. infinite capacity). More importantly, if token_limit is missing/falsy but quota_type is NOT unlimited (e.g., a data error), it also masks the value as 0.
  • Recommendation: Ensure that the business logic correctly interprets 0% for unlimited quotas. If the UI displays a progress bar based on this percentage, users with unlimited quotas will see an empty bar, which is acceptable, but ensure this is intended.

Optimization/Suggestion:
Consider explicitly handling the UNLIMITED case outside the calculation to avoid unnecessary Math.min/max calls if performance is critical (though negligible here). More importantly, verify if return null or a specific flag is better handled by the template than returning 0.

However, looking at the template change, it seems the template also handles UNLIMITED textually. The percentage might not be used for text display but for a progress bar.

2. Naming Convention & Data Structure Mismatch (total_tokens vs token_limit)

Observation:

  • Before: Used quota.total_tokens for calculation and display.
  • After: Uses quota.token_limit for calculation and display.

Potential Issue:

  • Ensure the backend/API has actually migrated from total_tokens to token_limit. If total_tokens still exists in some contexts but token_limit is the new field, make sure account.value?.token_quota is consistently providing token_limit.
  • If token_limit can be 0 for legitimate limits, the check !quota.token_limit will treat 0 as "falsy" and return 0, which is correct mathematically (division by zero avoided), but ensure the UI communicates this correctly (e.g., "Limit reached" vs "Not configured").

3. Template Logic Complexity & Readability

Current Code:

/{{ account?.token_quota?.quota_type === QUOTA_TYPE.UNLIMITED ? '不限' : formatTokenNumber(account?.token_quota?.token_limit) }}

Suggestions:

  1. Extract Computed Property: The ternary operator inside the template is getting long. Consider extracting this into a computed property for readability and reusability.

    const tokenLimitDisplay = computed(() => {
  if (account.value?.token_quota?.quota_type === QUOTA_TYPE.UNLIMITED) {
    return '不限'
  }
  return formatTokenNumber(account.value?.token_quota?.token_limit)
})

    Then in template:

    /{{ tokenLimitDisplay }}

  2. Null Safety: The use of optional chaining (?.) is good. However, repeatedly accessing account.value?.token_quota?.token_limit in both computed and template is slightly redundant. Extracting the quota object itself can help:

    const quota = computed(() => account.value?.token_quota || {})

4. Edge Case: Division by Zero/Invalid token_limit

Code:

return Math.min(100, Math.max(0, (quota.used_tokens / quota.token_limit) * 100))

Analysis:

  • You correctly check !quota.token_limit before the division. This prevents division by zero if token_limit is 0 or undefined.
  • Clamp Logic: Math.min(100, Math.max(0, ...)) ensures the result is between 0-100. This is robust.

Suggestion:
If token_limit is negative (bad data), max(0, ...) will clamp it to 0, hiding the error. Consider logging if a negative limit is encountered during development.

5. Minor: Comment Clarity

Comment:
// 配额尚未接入时保留空值展示，不将缺失数据表示为零用量。
(Translation: Keep empty value display when quota is not yet accessed; do not represent missing data as zero usage.)

Inconsistency:
The comment says "keep empty value display" (implying null/undefined might be better), but the code returns 0. Returning 0 explicitly represents "zero usage". If the intent is to hide the UI element when data is missing, returning null from the computed property and using v-if="tokenPercentage !== null" might be more aligned with the comment's intent.

Fix:
If you want to follow the comment strictly:

const tokenPercentage = computed(() => {
  const quota = account.value?.token_quota
  // Return null if no valid quota data exists to allow hiding UI elements
  if (!quota) return null 
  if (quota.quota_type === QUOTA_TYPE.UNLIMITED) return null // Or 0, depending on desired UX
  if (!quota.token_limit) return null

  return Math.min(100, Math.max(0, (quota.used_tokens / quota.token_limit) * 100))
})

Summary of Recommendations

  1. Confirm Field Names: Verify total_tokens is fully deprecated in favor of token_limit across all environments.
  2. Improve Readability: Extract the complex ternary in the template into a computed property.
  3. Clarify "Missing" vs "Zero": Re-evaluate if returning 0 aligns with the comment about "not representing missing data as zero usage." Consider returning null for missing data and handling it in the template.
  4. Robustness: The current logic safely handles division by zero and clamps values, which is good.

@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.

Overall, the change from total_tokens to token_limit is reasonable, but there are a few semantic and defensive-programming issues.

Potential issues

  1. Returning 0 conflates multiple states

    • No quota
    • Unlimited quota
    • Missing/invalid token_limit
    • Actual 0% usage
      all return 0. This contradicts the comment saying missing data should not be represented as zero usage. If the UI uses this value for a progress bar, 0 may be misleading. Consider returning null/undefined for “not applicable” states, or use a separate isQuotaFinite flag.

  2. !quota.token_limit is not a full numeric validation

    • It rejects 0, null, undefined, NaN, but accepts negative numbers, Infinity, and numeric strings like "0".
    • A negative value would pass and produce a misleading clamped result.
    • Prefer: 
      if (!Number.isFinite(limit) || limit <= 0) return 0

  3. used_tokens is not guarded

    • If quota.used_tokens is undefined, the expression becomes NaN, and: 
      Math.min(100, Math.max(0, NaN)) // NaN
    • Add a finite-number check or default only when that is semantically correct.

  4. Template fallback is inconsistent with the computed guard

    • The template only handles UNLIMITED; otherwise it always calls: 
      formatTokenNumber(account?.token_quota?.token_limit)
    • If token_limit is missing, the formatter may render 0, undefined, or NaN depending on its implementation. This can violate the “do not show missing as zero” rule.
    • Align the template condition with the computed logic, e.g. show '--' or hide the value when token_limit is not a valid number.

  5. Verify all total_tokens references were updated

    • Renaming to token_limit should be checked across:
      • Type definitions
      • API response models
      • Other components/composables
      • Tests and mocks
        Otherwise you may get stale references or runtime mismatches.

  6. Unlimited quota may still show a 0% progress bar

    • The text shows 不限, but tokenPercentage returns 0.
    • If this drives a progress indicator, consider hiding the progress bar for unlimited quotas instead of showing 0%.

Suggested refactor

const tokenPercentage = computed<number | null>(() => {
  const quota = account.value?.token_quota
  if (!quota) return null
  if (quota.quota_type === QUOTA_TYPE.UNLIMITED) return null

  const { used_tokens, token_limit } = quota
  if (!Number.isFinite(token_limit) || token_limit <= 0) return null
  if (!Number.isFinite(used_tokens)) return null

  return Math.min(100, Math.max(0, (used_tokens / token_limit) * 100))
})

If the progress component requires a number instead of null, keep returning 0 for rendering but use a separate boolean such as hasFiniteQuota to hide or disable the progress display.

Template consistency suggestion:

<span class="text-N500">
  /
  {{
    account?.token_quota?.quota_type === QUOTA_TYPE.UNLIMITED
      ? '不限'
      : account?.token_quota?.token_limit != null
        ? formatTokenNumber(account.token_quota.token_limit)
        : '--'
  }}
</span>

This avoids showing misleading 0 values for missing quota data.

Expand Down
Loading