Implement Authorization for NestBot calendar events and add improvements #2315
Conversation
Summary by CodeRabbit
WalkthroughAdds a leader-only permission helper for NestBot calendar events, changes one-off reminder scheduling to a send-and-delete flow, gates Slack calendar handlers behind permission checks with improved messaging and event caching, and updates/expands tests and a small migration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/nest/schedulers/calendar_events/slack.py (1)
26-28: Delete reminder schedule record as well.We call
reminder_schedule.reminder.delete(), but the schedule itself remains when the FK is nullable. Explicitly delete the schedule to avoid orphans when cascade is disabled or future schema changes.- if reminder_schedule := ReminderSchedule.objects.filter(pk=reminder_schedule_id).first(): - reminder_schedule.reminder.delete() + if reminder_schedule := ReminderSchedule.objects.filter(pk=reminder_schedule_id).first(): + reminder_schedule.delete()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/nest/auth/calendar_events.py(1 hunks)backend/apps/nest/schedulers/calendar_events/base.py(3 hunks)backend/apps/nest/schedulers/calendar_events/slack.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(5 hunks)backend/tests/apps/nest/schedulers/calendar_events/base_test.py(2 hunks)backend/tests/apps/nest/schedulers/calendar_events/slack_test.py(1 hunks)backend/tests/apps/slack/common/handlers/calendar_events_test.py(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/apps/nest/schedulers/calendar_events/base.py
🧬 Code graph analysis (7)
backend/apps/nest/auth/calendar_events.py (2)
backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/slack/common/handlers/calendar_events.py (4)
backend/apps/slack/blocks.py (1)
markdown(21-34)backend/apps/nest/auth/calendar_events.py (1)
has_calendar_events_permission(7-13)backend/apps/nest/models/google_account_authorization.py (2)
GoogleAccountAuthorization(28-225)authorize(55-93)backend/apps/nest/handlers/calendar_events.py (1)
set_reminder(35-88)
backend/tests/apps/nest/schedulers/calendar_events/base_test.py (2)
backend/apps/nest/schedulers/calendar_events/base.py (4)
BaseScheduler(8-64)schedule(16-40)send_and_delete(49-52)send_and_update(55-58)backend/apps/nest/schedulers/calendar_events/slack.py (2)
send_and_delete(21-28)send_and_update(31-34)
backend/apps/nest/schedulers/calendar_events/base.py (1)
backend/apps/nest/schedulers/calendar_events/slack.py (2)
send_and_delete(21-28)send_message(12-18)
backend/apps/nest/schedulers/calendar_events/slack.py (2)
backend/apps/nest/schedulers/calendar_events/base.py (2)
send_and_delete(49-52)send_message(61-64)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-66)
backend/tests/apps/slack/common/handlers/calendar_events_test.py (2)
backend/apps/slack/common/handlers/calendar_events.py (4)
get_cancel_reminder_blocks(14-34)get_events_blocks(37-97)get_reminders_blocks(100-125)get_setting_reminder_blocks(128-169)backend/apps/nest/models/google_account_authorization.py (1)
GoogleAccountAuthorization(28-225)
backend/tests/apps/nest/schedulers/calendar_events/slack_test.py (2)
backend/apps/nest/schedulers/calendar_events/slack.py (2)
SlackScheduler(8-34)send_and_delete(21-28)backend/apps/nest/schedulers/calendar_events/base.py (1)
send_and_delete(49-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (34)
backend/tests/apps/nest/schedulers/calendar_events/slack_test.py (1)
23-33: LGTM! Comprehensive test coverage for send_and_delete method.The test properly validates the new
send_and_deleteflow by mocking both the message sending and deletion aspects. The assertions correctly verify that the message is sent first, the reminder schedule is looked up by ID, and the associated reminder is deleted.backend/tests/apps/nest/schedulers/calendar_events/base_test.py (4)
35-35: Good addition of pk attribute to mock.Adding the
pkattribute tomock_reminder_scheduleis necessary for the updated scheduling flow that passesreminder_schedule_idto the enqueue method.
100-104: LGTM! Proper test for the new abstract method.The test correctly verifies that the base
send_and_deletemethod raisesNotImplementedError, ensuring subclasses must implement this method.
109-109: Good consistency in parameter types.Using primitive value
4instead of aMagicMock()for thereminder_schedule_idparameter is more appropriate and consistent with the actual usage pattern.
43-49: Scheduling behavior change verifiedSlackScheduler.send_and_delete is implemented and correctly scheduled by BaseScheduler.enqueue_at, aligning with the intent to consolidate send-and-delete into a single job.
backend/apps/nest/schedulers/calendar_events/base.py (3)
49-52: LGTM! Proper abstract method definition.The new
send_and_deletemethod is correctly defined as a static method with appropriate parameters and raisesNotImplementedErrorto enforce implementation in subclasses.
60-64: Good preservation of send_message as a separate contract.Keeping
send_messageas a distinct method fromsend_and_deleteprovides flexibility for use cases where deletion isn't required. The method signature and error handling are consistent with the existing pattern.
21-24: All scheduler implementations now support send_and_delete, and no residual enqueue_at → send_message calls remain.backend/apps/slack/common/handlers/calendar_events.py (8)
11-11: LGTM! Clean permission denial constant.The
PERMISSIONS_BLOCKconstant provides a consistent permission denial message across all calendar event handlers using the proper Slack markdown format.
16-21: Excellent authorization implementation.The permission check is properly placed at the beginning of the function and uses a clean early return pattern. The import is done locally to avoid potential circular dependencies.
39-45: Consistent permission gating across handlers.The permission check follows the same pattern as other handlers, ensuring consistent authorization enforcement across all calendar event features.
47-51: Good error handling for authorization flow.The code properly handles both successful authorization (returning
GoogleAccountAuthorizationinstance) and authorization requirement (returning tuple with URL). The user-friendly messaging guides users to complete the OAuth flow.
53-60: Appropriate error handling and user messaging.The addition of
ServerNotFoundErrorhandling with a clear "check your internet connection" message provides better user experience during network issues.
74-79: Smart event numbering and caching strategy.The event numbering scheme using page-based offsets (multiplying by 1000) effectively prevents collisions between pages, and caching the Google Calendar ID with the generated event number is a clean approach for later reminder setup.
102-106: Consistent permission enforcement.The permission check in
get_reminders_blocksfollows the same pattern, ensuring users can only access reminders if they have calendar events permissions.
130-135: Complete permission coverage.All calendar event handlers now properly enforce permissions, ensuring the authorization requirement from issue #2290 is fully implemented.
backend/tests/apps/slack/common/handlers/calendar_events_test.py (18)
15-15: Good test coverage for the newly public function.Adding
get_cancel_reminder_blocksto the imports ensures the function is properly tested. This function was likely made public as part of the authorization changes.
23-23: Appropriate test constant.The
PERMISSION_STRINGconstant matches the actual permission denial message and provides a clean way to verify permission-denied scenarios across tests.
29-55: Comprehensive test coverage for cancel reminder functionality.The test properly validates the successful cancellation flow including permission checks, scheduler interaction, and correct response formatting. The mock structure accurately reflects the domain model relationships.
57-64: LGTM! Proper permission denial testing.The test correctly verifies that unauthorized users receive the permission denial message and that the permission check is called with the correct user ID.
66-76: Good error handling test coverage.The test validates that invalid reminder IDs are handled gracefully with appropriate error messaging, while still performing the permission check.
78-99: Proper ownership validation testing.The test correctly verifies that users can only cancel their own reminders by checking the slack_user_id mismatch scenario.
105-159: Thorough testing of events retrieval with permission checks.The test comprehensively validates the events flow including permission checking, Google authorization, calendar client usage, event parsing, and proper block formatting. The assertion count and content checks are appropriate.
161-170: Clean permission denial test for events.The test properly validates that unauthorized users cannot access calendar events and receive the permission denial message.
174-178: Good authorization flow testing.The test validates the OAuth authorization flow when users haven't connected their Google accounts, while ensuring permission checks still occur for authorized users.
191-207: Appropriate network error handling test.The test validates that service errors are handled gracefully with user-friendly messaging while maintaining permission checks.
212-228: Good edge case coverage for empty events.The test ensures that when no events are found, users receive an appropriate message indicating the empty state.
231-266: Comprehensive reminders testing with proper filtering.The test validates that reminders are properly filtered by slack_user_id, ordered by scheduled_time, and formatted correctly in the response blocks. The permission check is also verified.
268-275: Consistent permission testing for reminders.The permission denial test for reminders follows the same pattern as other handlers, ensuring consistency across the API.
278-290: Good empty state handling for reminders.The test validates that users with no reminders receive appropriate guidance on how to create them.
294-333: Thorough testing of reminder setting flow.The test comprehensively validates the reminder setting process including permission checks, handler calls, scheduler interaction, and message sending. The parameter validation in the
set_remindercall is particularly good.
335-349: Complete permission coverage for reminder setting.The permission denial test ensures that unauthorized users cannot set reminders, completing the authorization coverage for all calendar event operations.
352-384: Good error handling test coverage.The tests for
ValidationErrorandValueErrorscenarios ensure that various error conditions are handled gracefully with appropriate user messaging, while maintaining permission checks.
388-404: Comprehensive service error testing.The service error test ensures network-related issues are handled gracefully with user-friendly messaging.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/apps/nest/auth/calendar_events_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/apps/nest/auth/calendar_events_test.py (3)
backend/apps/nest/auth/calendar_events.py (1)
has_calendar_events_permission(7-13)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)backend/apps/slack/models/member.py (1)
Member(10-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/apps/nest/auth/calendar_events_test.py (1)
26-37: Assert the.exists()call to harden the regression guard.Great to see the negative branch covered. Please also assert that
EntityMember.objects.filter(...).exists()was invoked—e.g.mock_filter.return_value.exists.assert_called_once()—so the test fails if the production code stops calling.exists()or swaps in a different method.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/apps/nest/auth/calendar_events_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/apps/nest/auth/calendar_events_test.py (3)
backend/apps/nest/auth/calendar_events.py (1)
has_calendar_events_permission(7-13)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)backend/apps/slack/models/member.py (1)
Member(10-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
arkid15r
force-pushed
the
feature/nestbot-calendar-events
branch
from
8931916 to
93cc0cb
Compare
ahmedxgouda
force-pushed
the
nestbot-calendar/authorization
branch
from
1d0c2fa to
a837f90
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/apps/slack/common/handlers/calendar_events_test.py (2)
23-24: Avoid duplicating the permission copy in testsHard-coding the permission message here means the tests will break (or worse, silently assert the wrong thing) the next time the product copy changes. Please derive the expected text from the source constant so the test suite stays aligned with the handler. For example:
-from apps.slack.common.handlers.calendar_events import ( - get_cancel_reminder_blocks, - get_events_blocks, - get_reminders_blocks, - get_setting_reminder_blocks, -) -PERMISSION_STRING = "*You do not have the permission to access calendar events.*" +from apps.slack.common.handlers.calendar_events import ( + PERMISSIONS_BLOCK, + get_cancel_reminder_blocks, + get_events_blocks, + get_reminders_blocks, + get_setting_reminder_blocks, +) +PERMISSION_STRING = PERMISSIONS_BLOCK[0]["text"]["text"]
172-205: Assert the permission gate is invoked in every pathThis new test exercises the “no auth” flow but no longer asserts that
has_calendar_events_permissionwas called. That means we won’t detect if someone accidentally removes the permission gate from this code path—a regression the PR is meant to prevent. Please addmock_has_permission.assert_called_once_with("test_slack_user_id")here (and mirror it in the other happy/error-path tests below: service error, no events, no reminders, validation/value/server errors for setting reminders) to keep the coverage consistent with the success-path cases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/apps/nest/auth/calendar_events.py(1 hunks)backend/apps/nest/schedulers/calendar_events/base.py(3 hunks)backend/apps/nest/schedulers/calendar_events/slack.py(1 hunks)backend/apps/slack/common/handlers/calendar_events.py(5 hunks)backend/tests/apps/nest/auth/calendar_events_test.py(1 hunks)backend/tests/apps/nest/schedulers/calendar_events/base_test.py(2 hunks)backend/tests/apps/nest/schedulers/calendar_events/slack_test.py(1 hunks)backend/tests/apps/slack/common/handlers/calendar_events_test.py(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/slack/common/handlers/calendar_events.py
- backend/tests/apps/nest/schedulers/calendar_events/slack_test.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T15:32:12.688Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2211
File: backend/apps/nest/controllers/calendar_events.py:0-0
Timestamp: 2025-09-10T15:32:12.688Z
Learning: In the backend/apps/nest/controllers/calendar_events.py file, the scheduled_time parameter in the schedule_reminder function is guaranteed to be timezone-aware, so explicit timezone awareness validation is not needed.
Applied to files:
backend/apps/nest/schedulers/calendar_events/base.py
🧬 Code graph analysis (6)
backend/tests/apps/nest/auth/calendar_events_test.py (3)
backend/apps/nest/auth/calendar_events.py (1)
has_calendar_events_permission(7-13)backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)backend/apps/slack/models/member.py (1)
Member(10-78)
backend/apps/nest/schedulers/calendar_events/base.py (1)
backend/apps/nest/schedulers/calendar_events/slack.py (2)
send_and_delete(21-28)send_message(12-18)
backend/tests/apps/slack/common/handlers/calendar_events_test.py (6)
backend/apps/slack/common/handlers/calendar_events.py (4)
get_cancel_reminder_blocks(14-34)get_events_blocks(37-97)get_reminders_blocks(100-125)get_setting_reminder_blocks(128-169)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-66)backend/apps/nest/models/reminder.py (1)
Reminder(6-33)backend/apps/owasp/models/event.py (1)
Event(25-377)backend/apps/nest/schedulers/calendar_events/base.py (1)
cancel(42-46)backend/apps/nest/models/google_account_authorization.py (1)
GoogleAccountAuthorization(28-225)
backend/apps/nest/schedulers/calendar_events/slack.py (2)
backend/apps/nest/schedulers/calendar_events/base.py (1)
send_and_delete(49-52)backend/apps/nest/models/reminder_schedule.py (1)
ReminderSchedule(6-66)
backend/tests/apps/nest/schedulers/calendar_events/base_test.py (2)
backend/apps/nest/schedulers/calendar_events/base.py (4)
BaseScheduler(8-64)schedule(16-40)send_and_delete(49-52)send_and_update(55-58)backend/apps/nest/schedulers/calendar_events/slack.py (2)
send_and_delete(21-28)send_and_update(31-34)
backend/apps/nest/auth/calendar_events.py (2)
backend/apps/owasp/models/entity_member.py (2)
EntityMember(10-112)Role(13-16)backend/apps/slack/models/member.py (1)
Member(10-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/nest/auth/calendar_events.py (1)
3-13: Fix authorization leak for unlinked Slack members.If a Slack user has no linked GitHub account then
member.userisNone, so this query degenerates toEntityMember.objects.filter(member=None, role=LEADER). That matches any leader entry whosememberFK is null, which means every Slack member without a linkage inherits calendar access as soon as one leader record lacks a GitHub association—directly violating the restriction we’re introducing. Please branch onmember.user_id, short‑circuit when it’s set, and only fall back to deterministic identifiers (email/login/Slack names) for the null-FK case so we never run themember=Nonefilter. Also add coverage for the unlinked-leader path so this doesn’t regress again. Proposed fix:-from apps.owasp.models.entity_member import EntityMember -from apps.slack.models.member import Member +from django.db.models import Q +from apps.owasp.models.entity_member import EntityMember +from apps.slack.models.member import Member @@ - return EntityMember.objects.filter(member=member.user, role=EntityMember.Role.LEADER).exists() + leader_memberships = EntityMember.objects.filter(role=EntityMember.Role.LEADER) + + if member.user_id and leader_memberships.filter(member=member.user).exists(): + return True + + fallback_filters = Q() + has_predicate = False + + if member.email: + fallback_filters |= Q(member_email__iexact=member.email) + has_predicate = True + + user_login = getattr(member.user, "login", None) + for candidate in (user_login, member.real_name, member.username): + if candidate: + fallback_filters |= Q(member__isnull=True, member_name__iexact=candidate) + has_predicate = True + + if not has_predicate: + return False + + return leader_memberships.filter(fallback_filters).exists()
arkid15r
force-pushed
the
feature/nestbot-calendar-events
branch
from
93cc0cb to
9710778
Compare
…b only for the once recurring reminder
ahmedxgouda
force-pushed
the
nestbot-calendar/authorization
branch
from
a837f90 to
db7d1b4
Compare
|
1 participant
Proposed change
Resolves #2290
Checklist
make check-testlocally; all checks and tests passed.