Skip to content

fix: anonymous authentication #3216

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
shaohuzhang1 merged 1 commit into from
Jun 9, 2025
Merged

fix: anonymous authentication #3216

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_authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def auth(self, request, with_valid=True):
_type = AuthenticationType.CHAT_ANONYMOUS_USER
return ChatUserToken(application_access_token.application_id, None, access_token, _type,
ChatUserType.ANONYMOUS_USER,
chat_user_id, ChatAuthentication(None, False, False)).to_token()
chat_user_id, ChatAuthentication(None)).to_token()
else:
raise NotFound404(404, _("Invalid access_token"))

Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Jun 9, 2025

Choose a reason for hiding this comment

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

There seem to be two minor issues in the provided code:

  1. The use of ChatAuthentication(None, False, False) creates an instance of ChatAuthentication without setting the required attributes. This might cause unexpected behavior when converting the token.

  2. The _type variable is set to AuthenticationType.CHAT_ANONYMOUS_USER, but if with_valid=False, a NotFound404 exception will be raised instead of returning a token immediately. It would be better to handle this case more gracefully by either ensuring that the condition is always met or modifying how exceptions are handled after authentication checks.

Expand Down
8 changes: 3 additions & 5 deletions apps/common/auth/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,19 @@


class ChatAuthentication:
def __init__(self, auth_type: str | None, is_auth: bool, auth_passed: bool):
self.is_auth = is_auth
self.auth_passed = auth_passed
def __init__(self, auth_type: str | None):
self.auth_type = auth_type

def to_dict(self):
return {'is_auth': self.is_auth, 'auth_passed': self.auth_passed, 'auth_type': self.auth_type}
return {'auth_type': self.auth_type}

def to_string(self):
return encrypt(json.dumps(self.to_dict()))

@staticmethod
def new_instance(authentication: str):
auth = json.loads(decrypt(authentication))
return ChatAuthentication(auth.get('auth_type'), auth.get('is_auth'), auth.get('auth_passed'))
return ChatAuthentication(auth.get('auth_type'))


class ChatUserToken:
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Jun 9, 2025

Choose a reason for hiding this comment

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

There are several areas where the code can be improved:

  1. Consistent Initialization: In ChatAuthentication, you have different initializers with varying parameters. Only accept one or two of these in future implementations.

  2. Use Constants for Boolean Values: For is_auth and auth_passed, define constants (e.g., AUTH_IS_ACTIVE) to avoid magic numbers.

  3. Avoid Implicit Decryption in Constructor: Direct decryption within the constructor might expose sensitive data if not handled securely.

  4. Remove Redundant Parameters: Since is_auth becomes redundant with authorization_status, remove it from the constructor and update methods accordingly.

Here’s a proposed improvement on how the class could look like after addressing these points. Note that actual encryption/decryption logic is omitted as it depends on external libraries such as AES-CBC, but this should give an idea of restructuring.

@@ -14,7 +14,8 @@
class ChatAuthentication:
    AUTH_TYPE_NOT_SET = "Not Set"
    NOT_AUTHENTICATED = False
    AUTHENTICATION_SUCCESSFUL = True

    def __init__(self, auth_type: str):
        """Initialize the authentication type."""
        self.auth_type = auth_type or self.AUTH_TYPE_NOT_SET

    def to_dict(self):
        # This method doesn't need to include all fields anymore due to simplification
        return {'auth_type': self.auth_type}

    def to_string(self):
        encrypted_data = encrypt(json.dumps(self.to_dict()))
        # Encrypted result will be stringified before returning
        return encrypted_result

    @staticmethod
    def new_instance(authentication: str):
        decrypted_data = decrypt(authentication)
        auth_obj = {k.lower(): v for k,v in json.loads(decrypted_data).items()}
        return ChatAuthentication(
            auth_type=auth_obj.get('auth_type', ChatAuthentication.AUTH_TYPE_NOT_SET),
            authorization_status=auth_obj.get('authorization_status', ChatAuthentication.NOT_AUTHENTICATED)  # Assuming 'authorization_status' replaces 'is_auth'
        )

Note: Replace encrypt and decrypt functions with proper cryptographic library function calls, ensuring they manage secrets appropriately.

Expand Down
3 changes: 2 additions & 1 deletion apps/common/auth/handle/impl/chat_anonymous_user_token.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ def handle(self, request, token: str, get_token_details):
if application_setting_model is not None:
application_setting = QuerySet(application_setting_model).filter(application_id=application_id).first()
if application_setting.authentication:
raise AppAuthenticationFailed(1002, _('Authentication information is incorrect'))
if 'password' != chat_user_token.authentication.auth_type:
raise AppAuthenticationFailed(1002, _('Authentication information is incorrect'))
return None, ChatAuth(
current_role_list=[RoleConstants.CHAT_ANONYMOUS_USER],
permission_list=[
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Jun 9, 2025

Choose a reason for hiding this comment

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

There's an inconsistency in checking the auth_type. If the authentication type is neither 'password' nor anything else specified, it should be explicitly checked within the AuthenticationFailed exception message to prevent incorrect error messages. Here’s an updated version with proper logic:

@@ -45,7 +45,8 @@ def handle(self, request, token: str, get_token_details):
         if application_setting_model is not None:
             application_setting = QuerySet(application_setting_model).filter(application_id=application_id).first()
             if application_setting.authentication:
-                raise AppAuthenticationFailed(1002, _('Authentication information is incorrect'))
+                auth_types_allowed = ['custom', 'oauth']  # Add the allowed types here
+                if chat_user_token.authentication.auth_type not in auth_types_allowed:
+                    raise AppAuthenticationFailed(1002,
+                                                 _("Unknown or unsupported authentication method"))

This ensures that only known and supported authentication methods trigger the exception, instead of being overly generic (e.g., "Authentication information is incorrect").

Expand Down
Loading