Allow zeus to see activities in hidden courses even when not subscribed #5986
Conversation
jorg-vr
commented
Dec 9, 2024
jorg-vr
commented
- Tests were added
WalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Course
User->>Course: Request visibility
Course-->>User: Check visibility conditions
alt User is zeus?
Course-->>User: Grant access to hidden course
else Other conditions
Course-->>User: Deny access
end
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 (1)
test/system/courses_test.rb (1)
61-69: Test implementation looks good, but could be more comprehensive.The test verifies the basic functionality, but consider enhancing it with additional assertions:
- Verify specific activity content is visible
- Test edge cases (e.g., multiple series/activities)
- Verify navigation through course structure
Consider expanding the test with additional scenarios:
test 'zeus should see activities in hidden courses' do course = create :course, visibility: :hidden, series_count: 1, activities_per_series: 1 + # Create multiple series to test comprehensive access + series = create :series, course: course + activity = create :exercise, series: series sign_in users(:zeus) visit(course_path(:en, course.id)) - # assert activity table is visible assert_selector '.activity-table' + # Verify specific activity content + assert_selector '.activity-table', text: activity.name + # Verify series structure + assert_selector '.card.series', count: 2 + # Verify navigation works + click_on activity.name + assert_current_path activity_path(:en, activity) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/models/course.rb(1 hunks)test/system/courses_test.rb(1 hunks)
🔇 Additional comments (1)
app/models/course.rb (1)
308-308: Implementation looks correct, verify broader system impact.
The change correctly implements the requirement by adding the zeus check while maintaining existing visibility rules.
Let's verify other parts of the system that might be affected:
✅ Verification successful
Implementation is correct and consistent with system-wide security model
The zeus check addition to the course visibility logic is consistent with the system-wide security model, as evidenced by:
-
The change aligns with existing zeus-based access control patterns throughout the codebase:
- Zeus users have system-wide access in various policies (e.g., JudgePolicy, UserPolicy, RepositoryPolicy)
- Zeus check is consistently used as a top-level override for access control
-
The implementation maintains the existing visibility rules while adding the zeus override:
- Preserves all existing visibility checks (visible_for_all, visible_for_institution)
- Zeus check is added as a final OR condition, consistent with other similar implementations
-
The change is properly integrated with the course policy system:
- CoursePolicy already handles zeus users appropriately in its scope resolution
- Other course-related policies (SeriesPolicy, etc.) rely on this visibility check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other visibility-related code that might need adjustment
echo "Checking for related visibility checks..."
rg -A 3 "visible_for_user|visible_for_all|visible_for_institution" --type ruby
# Check for potential security implications in controllers
echo "Checking controller authorization..."
rg -A 3 "before_action.*:authorize" app/controllers/
Length of output: 12445
Script:
#!/bin/bash
# Let's check for any potential security-related code that might need the zeus check
echo "Checking for zeus-related security checks..."
rg -A 3 "zeus\?" --type ruby
# Let's also check for any course visibility related tests
echo "Checking course visibility tests..."
ast-grep --pattern 'test $_ do
$$$
visible_for_$$$
$$$
end'
# Check for any other course access control methods
echo "Checking course access control methods..."
ast-grep --pattern 'def $_
$$$
visible_for_$$$
$$$
end'
Length of output: 15491
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.
