Do not allow new users to run /retest or /build-and-push-container#769
Do not allow new users to run /retest or /build-and-push-container#769
Conversation
New users (unknown) is not allowed to run /retest and /build-and-push-container commands without permission from one of the approvers.
|
Warning Rate limit exceeded@myakove has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughThe changes introduce user permission checks for executing specific commands via issue comments on pull requests. Only contributors, collaborators, maintainers, or explicitly allowed users can trigger retest, cherry-pick, or build-and-push-container commands. The Python version requirement in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
PR will be approved when the following conditions are met:
Approvers and Reviewers
Supported /retest check runs
Supported labels
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pyproject.toml (1)
75-75: Remove stray leading space in author e-mail
There is a leading space beforemyakove@gmail.com; packaging metadata parsers will keep it verbatim.-email = " myakove@gmail.com" +email = "myakove@gmail.com"webhook_server/libs/github_api.py (1)
2297-2318: Improve maintainers-notice formatting & make lookup constant-time
- Message renders the first maintainer without the leading dash.
- Each call walks every comment – consider caching the “allowed” decision per PR.
- {"\n - @".join(self.all_approvers)} + {"".join([f"\n - @{u}" for u in self.all_approvers])}Low-priority, but it will read better for users of the bot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyproject.toml(4 hunks)webhook_server/libs/github_api.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
webhook_server/libs/github_api.py (1)
webhook_server/tests/test_add_reviewer_action.py (2)
get_contributors(13-14)create_issue_comment(21-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: python-module-install
- GitHub Check: tox
- GitHub Check: build-container
- GitHub Check: pre-commit
🔇 Additional comments (3)
pyproject.toml (1)
42-42: Verify CI/runtime availability for Python 3.12
The required Python version jumped from>=3.10to>=3.12.
• Make sure your CI runners (e.g. GitHub Actions) and prod images already provide 3.12.
• Double-check that all pinned/tested dependencies are 3.12-compatible (some tools – especially binary wheels – often lag a minor version).
If either of the above is not true, pipeline breaks are likely.webhook_server/libs/github_api.py (2)
1358-1360: Double validation path consideration
_run_build_containernow performs its own permission check only whenreviewed_useris passed. During/retest build-container, the function is invoked without that argument (it was already validated earlier). This is fine, but keep in mind that future internal calls must either supply the user or rely on the outer validation to avoid bypasses.
1091-1099: Good addition – propagates user identity for permission checks
Passingreviewed_userdown to the concrete command handlers enables granular gating. Looks correct after the refactor.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
webhook_server/libs/github_api.py (2)
1753-1789:⚠️ Potential issueMissing user validation in
process_cherry_pick_command.Unlike
process_retest_commandand_run_build_container, this method doesn't validate if the user is allowed to run cherry-pick commands, creating an inconsistency in the permission model.Apply this fix to add the validation check:
def process_cherry_pick_command(self, command_args: str, reviewed_user: str) -> None: + if not self._is_user_valid_to_run_commands(reviewed_user=reviewed_user): + return + _target_branches: list[str] = command_args.split() _exits_target_branches: set[str] = set() _non_exits_target_branches_msg: str = ""
2297-2319:⚠️ Potential issuePotential exceptions in
_is_user_valid_to_run_commandsand missing error handling.The method assumes
self.pull_requestis always available and lacks error handling for GitHub API operations.Add error handling and safer access to pull_request:
def _is_user_valid_to_run_commands(self, reviewed_user: str) -> bool: allow_user_comment = f"/add-allowed-user @{reviewed_user}" comment_msg = f""" {reviewed_user} is not allowed to run retest commands. maintainers can allow it by comment `{allow_user_comment}` Maintainers: {"\n - @".join(self.all_approvers)} """ if reviewed_user not in self.valid_users_to_run_commands: + if not hasattr(self, "pull_request") or not self.pull_request: + self.logger.error(f"{self.log_prefix} Cannot check user permissions: pull_request not available") + return False + + try: comments_from_approvers = [ comment.body for comment in self.pull_request.get_comments() if comment.user.login in self.all_approvers ] for comment in comments_from_approvers: if allow_user_comment in comment: return True self.logger.debug(f"{self.log_prefix} {reviewed_user} is not in {self.valid_users_to_run_commands}") self.pull_request.create_issue_comment(comment_msg) + except Exception as e: + self.logger.error(f"{self.log_prefix} Error checking user permissions: {e}") return False return True
♻️ Duplicate comments (1)
webhook_server/libs/github_api.py (1)
154-154:⚠️ Potential issueThe
valid_users_to_run_commandsinitialization is too early.The initialization happens in
__init__beforeself.pull_requestis available (it's set later in theprocessmethod at line 163). This will cause_get_valid_users_to_run_commandsto always return an empty set, meaning every user will be treated as new/unknown and unable to run commands.Apply this fix:
- self.valid_users_to_run_commands = self._get_valid_users_to_run_commands() + # Will be populated after pull_request is available + self.valid_users_to_run_commands: set[str] = set()Then update the
processmethod to populate this after the pull request is obtained:try: self.pull_request = self._get_pull_request() self.log_prefix = self.prepare_log_prefix(pull_request=self.pull_request) self.logger.debug(f"{self.log_prefix} {event_log}") self.last_commit = self._get_last_commit() self.parent_committer = self.pull_request.user.login self.last_committer = getattr(self.last_commit.committer, "login", self.parent_committer) self.changed_files = self.list_changed_files() self.pull_request_branch = self.pull_request.base.ref self.all_approvers_and_reviewers = self.get_all_approvers_and_reviewers() self.all_approvers = self.get_all_approvers() self.all_reviewers = self.get_all_reviewers() + self.valid_users_to_run_commands = self._get_valid_users_to_run_commands()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
webhook_server/libs/github_api.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
webhook_server/libs/github_api.py (1)
webhook_server/tests/test_add_reviewer_action.py (2)
get_contributors(13-14)create_issue_comment(21-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tox
- GitHub Check: pre-commit
- GitHub Check: build-container
- GitHub Check: python-module-install
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
webhook_server/libs/github_api.py (1)
2294-2316: 🛠️ Refactor suggestionUser validation logic needs to handle approvers consistently
The user validation logic checks if a user is in
valid_users_to_run_commandsor has been explicitly allowed by an approver. However, it doesn't automatically allow approvers themselves, which seems inconsistent.Improve the method to automatically allow approvers:
def _is_user_valid_to_run_commands(self, reviewed_user: str) -> bool: + # Approvers are always allowed to run commands + if reviewed_user in self.all_approvers: + return True + allow_user_comment = f"/add-allowed-user @{reviewed_user}" comment_msg = f""" {reviewed_user} is not allowed to run retest commands. maintainers can allow it by comment `{allow_user_comment}` Maintainers: {"\n - @".join(self.all_approvers)} """ if reviewed_user not in self.valid_users_to_run_commands: comments_from_approvers = [ comment.body for comment in self.pull_request.get_comments() if comment.user.login in self.all_approvers ] for comment in comments_from_approvers: if allow_user_comment in comment: return True self.logger.debug(f"{self.log_prefix} {reviewed_user} is not in {self.valid_users_to_run_commands}") self.pull_request.create_issue_comment(comment_msg) return False return True
♻️ Duplicate comments (1)
webhook_server/libs/github_api.py (1)
173-173:⚠️ Potential issueCritical timing issue:
valid_users_to_run_commandsinitialized beforepull_requestis availableThe initialization of
valid_users_to_run_commandsoccurs in__init__at line 173, butself.pull_requestis only set later during theprocess()method at line 162. This means that_get_valid_users_to_run_commands()will be called beforeself.pull_requestis available, making the permission checks ineffective.Move this initialization after
self.pull_requestis set in theprocess()method:- self.valid_users_to_run_commands = self._get_valid_users_to_run_commands()Then add this line after line 172 in the
process()method:self.all_reviewers = self.get_all_reviewers() + self.valid_users_to_run_commands = self._get_valid_users_to_run_commands()
🧹 Nitpick comments (1)
webhook_server/libs/github_api.py (1)
1080-1104:user_commandsmethod needs updating to maintain consistencyThe
user_commandsmethod needs to be updated to maintain consistency with the changes made for user validation.Consider adding a check at the beginning of the
user_commandsmethod to validate the user early and avoid duplicate checks in individual command handlers:def user_commands(self, command: str, reviewed_user: str, issue_comment_id: int) -> None: + # Check if user is allowed to run any commands + if command in [COMMAND_RETEST_STR, BUILD_AND_PUSH_CONTAINER_STR, COMMAND_CHERRY_PICK_STR]: + if not self._is_user_valid_to_run_commands(reviewed_user=reviewed_user): + return + available_commands: list[str] = [ COMMAND_RETEST_STR, COMMAND_CHERRY_PICK_STR, COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, BUILD_AND_PUSH_CONTAINER_STR, COMMAND_ASSIGN_REVIEWER_STR, ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webhook_server/libs/github_api.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
webhook_server/libs/github_api.py (1)
webhook_server/tests/test_add_reviewer_action.py (2)
get_contributors(13-14)create_issue_comment(21-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-container
- GitHub Check: python-module-install
- GitHub Check: tox
- GitHub Check: pre-commit
🔇 Additional comments (2)
webhook_server/libs/github_api.py (2)
1091-1092: Improved signature changes for command methodsThe signature changes to use
reviewed_userinstead ofissue_comment_idinprocess_cherry_pick_command,process_retest_command, and the call to_run_build_containerare appropriate for the new permission model.Also applies to: 1094-1095, 1098-1098
1790-1793: Well implemented permission check inprocess_retest_commandThe early return when user validation fails is implemented correctly here.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
webhook_server/libs/github_api.py(21 hunks)webhook_server/tests/test_pull_request_owners.py(19 hunks)
✅ Files skipped from review due to trivial changes (1)
- webhook_server/tests/test_pull_request_owners.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
webhook_server/libs/github_api.py (1)
webhook_server/tests/test_add_reviewer_action.py (2)
get_contributors(13-14)create_issue_comment(21-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tox
- GitHub Check: build-container
- GitHub Check: python-module-install
- GitHub Check: pre-commit
🔇 Additional comments (1)
webhook_server/libs/github_api.py (1)
171-175: ✔️ Property is now evaluated after PR context is readyCalling
_get_valid_users_to_run_commandsafterself.pull_requestis available fixes the “initialisation-too-early” problem raised in the previous review – good catch.
|
/lgtm |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webhook_server/libs/github_api.py (1)
2347-2348: Consider improving variable name for clarityThe variable
allowed_user_to_approveappears to represent a list of users who can approve other users to run commands. Consider renaming tousers_allowed_to_grant_permissionorpermission_granting_usersfor clarity.- allowed_user_to_approve = self.get_all_repository_maintainers() + self.all_repository_approvers + users_allowed_to_grant_permission = self.get_all_repository_maintainers() + self.all_repository_approvers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webhook_server/libs/github_api.py(22 hunks)
🧰 Additional context used
🧠 Learnings (1)
webhook_server/libs/github_api.py (1)
Learnt from: myakove
PR: myk-org/github-webhook-server#612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
🧬 Code Graph Analysis (1)
webhook_server/libs/github_api.py (2)
webhook_server/tests/test_pull_request_owners.py (1)
all_repository_approvers_and_reviewers(56-73)webhook_server/tests/test_add_reviewer_action.py (2)
get_contributors(13-14)create_issue_comment(21-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pre-commit
- GitHub Check: python-module-install
- GitHub Check: build-container
- GitHub Check: tox
🔇 Additional comments (9)
webhook_server/libs/github_api.py (9)
1949-1956: Good use of cached_property for expensive GitHub API callsUsing
@functools.cached_propertyfor GitHub API calls is an excellent performance improvement that prevents repeated API calls which could hit rate limits.
173-178: Improved clarity by separating repository-wide vs PR-specific usersThe renaming of variables to clearly distinguish between repository-wide approvers/reviewers and PR-specific ones improves code readability and makes the security model clearer.
1357-1364: Security improvement: User validation for build container commandAdding permission validation for the build container command is an important security enhancement that prevents unauthorized users from triggering container builds.
1794-1797: Security improvement: User validation for retest commandSimilar to the build container command, adding permission validation for the retest command is another important security enhancement that prevents unauthorized users from triggering tests.
2346-2371: Well-implemented permission check with approver overrideThe implementation of
_is_user_valid_to_run_commandsis robust - it checks if a user is authorized and allows approvers to explicitly grant permission to specific users via comments.
1957-1973: Good helper methods to determine user permissionsThese helper methods provide a clean abstraction for retrieving repository contributors, collaborators and maintainers, making the permission logic more readable.
537-538: Updated reviewer assignment to use PR-specific reviewersThe code now correctly uses PR-specific reviewers instead of repository-wide reviewers, making reviewer assignment more contextual to the changes in the PR.
2350-2355: Good maintainer list formatting in comment messageThe maintainer list in the rejection message is now correctly formatted with each entry properly prefixed with a dash.
2360-2361: Correctly using issue comments instead of review commentsUsing
get_issue_comments()is the right approach for checking for permission commands, as these will appear in the PR conversation and not as review comments.
|
/retest all |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
webhook_server/libs/github_api.py (3)
1362-1363: Check for None explicitly for safer validationThe validation condition could handle the
Nonecase more explicitly for consistency and safety.- if reviewed_user and not self._is_user_valid_to_run_commands(reviewed_user=reviewed_user): + if reviewed_user is not None and not self._is_user_valid_to_run_commands(reviewed_user=reviewed_user):
2352-2356: Fix formatting issue in maintainers list messageThe template string for maintainers list has a formatting inconsistency where the first maintainer doesn't have the "@" prefix.
-Maintainers: - - {"\n - @".join(allowed_user_to_approve)} +Maintainers: + - @{"\n - @".join(allowed_user_to_approve)}
2358-2371: Add error handling for GitHub API callsThe method doesn't handle potential exceptions when fetching comments from GitHub API, which could lead to unexpected failures.
if reviewed_user not in self.valid_users_to_run_commands: - comments_from_approvers = [ - comment.body - for comment in self.pull_request.get_issue_comments() - if comment.user.login in allowed_user_to_approve - ] + try: + comments_from_approvers = [ + comment.body + for comment in self.pull_request.get_issue_comments() + if comment.user.login in allowed_user_to_approve + ] + except Exception as e: + self.logger.error(f"{self.log_prefix} Error fetching issue comments: {e}") + comments_from_approvers = []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webhook_server/libs/github_api.py(22 hunks)
🧰 Additional context used
🧠 Learnings (1)
webhook_server/libs/github_api.py (1)
Learnt from: myakove
PR: myk-org/github-webhook-server#612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
🧬 Code Graph Analysis (1)
webhook_server/libs/github_api.py (2)
webhook_server/tests/test_pull_request_owners.py (1)
all_repository_approvers_and_reviewers(56-73)webhook_server/tests/test_add_reviewer_action.py (2)
get_contributors(13-14)create_issue_comment(21-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-container
- GitHub Check: pre-commit
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (3)
webhook_server/libs/github_api.py (3)
1757-1757: LGTM: Cherry pick command with user validationThe method correctly accepts a
reviewed_userparameter that can be used for permission validation.
1949-1956: Good use of cached properties for API callsUsing
@functools.cached_propertyfor GitHub API calls is an excellent optimization to prevent redundant network requests.
2338-2346: LGTM: Comprehensive authorized users calculationThe cached property efficiently aggregates users from various sources who should have command execution permissions.
|
/retest tox |
2 similar comments
|
/retest tox |
|
/retest tox |
|
/retest all |
|
/approve |
|
/lgtm |
|
Failed to delete tag: ghcr.io/myk-org/github-webhook-server:pr-769. Please delete it manually. |
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
3 participants
New users (unknown) is not allowed to run /retest and /build-and-push-container commands without permission from one of the approvers.
Summary by CodeRabbit
New Features
Style