Skip to content
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

Jump to bottom

Allow all student council members to view users #652

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Allow all student council members to view users #652

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
6 changes: 3 additions & 3 deletions app/Policies/UserPolicy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function viewAll(User $user): bool
Role::SECRETARY,
Role::DIRECTOR,
Role::STUDENT_COUNCIL_SECRETARY,
Role::STUDENT_COUNCIL => Role::STUDENT_COUNCIL_LEADERS,
Role::STUDENT_COUNCIL => array_merge(Role::STUDENT_COUNCIL_LEADERS, Role::COMMITTEE_LEADERS),
]);
}

Expand Down Expand Up @@ -68,7 +68,7 @@ public function viewAny(User $user): bool
Role::WORKSHOP_ADMINISTRATOR,
Role::WORKSHOP_LEADER,
Role::STUDENT_COUNCIL_SECRETARY,
Role::STUDENT_COUNCIL => Role::STUDENT_COUNCIL_LEADERS,
Role::STUDENT_COUNCIL => array_merge(Role::STUDENT_COUNCIL_LEADERS, Role::COMMITTEE_LEADERS),
]);
}

Expand Down Expand Up @@ -102,7 +102,7 @@ public function view(User $user, User $target): bool
return $user->hasRole([
Role::SECRETARY,
Role::DIRECTOR,
Role::STUDENT_COUNCIL => Role::STUDENT_COUNCIL_LEADERS,
Role::STUDENT_COUNCIL => array_merge(Role::STUDENT_COUNCIL_LEADERS, Role::COMMITTEE_LEADERS),
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Recommendation to Enhance Caching Strategy in UserPolicy.php

The Cache::remember implementation within UserPolicy.php effectively handles role checks. However, to ensure that permissions remain up-to-date when user roles change, it is advisable to implement a cache invalidation mechanism or reduce the cache duration.

  • Implement Cache Invalidation: Ensure that the cache is cleared or updated whenever a user's roles are modified.
  • Adjust Cache Duration: Consider reducing the cache time to balance performance with the need for up-to-date permissions.
🔗 Analysis chain

A word of caution regarding the caching strategy, if I may.

The modification to the view method is indeed consistent with the changes made to viewAll and viewAny. However, I must bring to your attention a potential issue with the caching strategy employed here. While caching is undoubtedly beneficial for performance, it may lead to outdated permissions if roles change frequently.

Might I suggest considering the following strategies to mitigate this risk:

  1. Reduce the cache duration from 60 seconds to a shorter period, balancing performance with up-to-date permissions.
  2. Implement a cache invalidation mechanism that clears this cache whenever a user's roles are modified.
  3. Include the user's role update timestamp in the cache key to ensure fresh data is fetched when roles change.

To assist in assessing the impact of caching, please consider running the following script:

Further Investigation Needed on Role-Based Caching in UserPolicy.php

To better assess the caching strategy's impact on permissions, please run the following script to identify all instances of Cache::remember in UserPolicy.php:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Identify all instances of role-based caching in the UserPolicy

echo "Instances of role-based caching in UserPolicy:"
rg "Cache::remember\(" app/Policies/UserPolicy.php -A 5 -B 2

Length of output: 449

Script:

#!/bin/bash
# Description: Identify all instances of role-based caching in UserPolicy

echo "Instances of role-based caching in UserPolicy:"
rg "Cache::remember\(" app/Policies/UserPolicy.php -A 5

Length of output: 554

Role::STUDENT_COUNCIL_SECRETARY,
]);
})) || $target->workshops
Expand Down