fixed user name issue in the audit log for leave bot api #1567
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
kairon/shared/account/processor.py (2)
601-606: LGTM! Consider adding a comment for clarity.The changes to the
remove_bot_accessmethod look good. The logic for handling both single user removal and all users removal is correct.Consider adding a brief comment to explain the difference between the two cases:
if kwargs: # Single user removal active_bot_access.update(set__status=ACTIVITY_STATUS.DELETED.value, user=kwargs.get('accessor_email')) else: # All users removal active_bot_access.update(set__status=ACTIVITY_STATUS.DELETED.value)
Line range hint
1012-1037: LGTM! New method handles bot leaving process well.The new
process_leave_botmethod is a great addition. It correctly handles the process of a user leaving a bot with appropriate checks and actions. The method prevents the bot owner from leaving and ensures that all integration tokens are deleted before a user can leave, which is a good security practice.Consider adding more specific error messages for different scenarios:
if owner_info["accessor_email"] == current_user.email: raise AppException("Bot owner cannot leave the bot. Transfer ownership first if needed.") if tokens_data: raise AppException("Please delete all your integration tokens for this bot before leaving.")This will provide clearer guidance to users on why they can't leave the bot in specific situations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- kairon/shared/account/processor.py (1 hunks)
- kairon/shared/data/audit/data_objects.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
kairon/shared/data/audit/data_objects.py (1)
75-77: Approved: User logging enhancement implemented correctly.The addition of
user=kwargs.get('user')in theauditlog.sendcall effectively addresses the PR objective of fixing the user name issue in the audit log. This change is consistent with other methods in the class and doesn't break the existing method signature.However, consider the following suggestions for further improvement:
- Add input validation for the 'user' parameter to ensure data integrity in the audit log.
- Consider using a default value for the user if it's not provided in kwargs, e.g.,
kwargs.get('user', 'system').Here's a suggested improvement:
def update(self, event_url=None, **kwargs): obj = super().update(**kwargs) - auditlog.send(self.__class__, document=self, action=AuditlogActions.UPDATE.value, event_url=event_url, user=kwargs.get('user')) + user = kwargs.get('user', 'system') + if not isinstance(user, str) or len(user) > 100: # Adjust max length as needed + raise ValueError("Invalid user value for audit log") + auditlog.send(self.__class__, document=self, action=AuditlogActions.UPDATE.value, event_url=event_url, user=user) return objTo ensure this change fully addresses the user name issue, please confirm:
- Are there any other locations in the codebase where user names are logged that might need similar updates?
- Is there a need to update existing audit log entries, or is this change sufficient for future logs?
✅ Verification successful
Verified: All relevant audit logging calls are updated.
No other instances of
auditlog.sendwere found in the codebase that require similar updates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other audit logging calls that might need similar updates rg --type python 'auditlog\.send' -g '!kairon/shared/data/audit/data_objects.py'Length of output: 113
Script:
#!/bin/bash # Search for other audit logging calls that might need similar updates rg -t py 'auditlog\.send' -g '!kairon/shared/data/audit/data_objects.py'Length of output: 74
kairon/shared/account/processor.py (1)
Line range hint
608-614: LGTM! New method adds important checks.The new
remove_membermethod is a good addition. It correctly prevents users from removing themselves and ensures that the bot owner cannot be removed. The method then appropriately callsremove_bot_accessfor the actual member removal.
Summary by CodeRabbit
New Features
Improvements
Refactor