GCTC-NTGC / gc-digital-talent

GC Digital Talent is the new recruitment platform for digital and tech jobs in the Government of Canada. // Talents numériques du GC est la nouvelle plateforme de recrutement pour les emplois numériques et technologiques au gouvernement du Canada.
https://talent.canada.ca
GNU Affero General Public License v3.0
21 stars 8 forks source link

🐛 ProtectedRequestUserChecker has unexpected behaviour checking array of permissions. #11149

Open tristan-orourke opened 2 months ago

tristan-orourke commented 2 months ago

🐛 Bug

Our custom ProtectedRequestUserChecker class adds an extra layer to the Laratrust isAbleTo check. If a permission would require any role besides Applicant or Base roles, it fails unless the request came from the Protected endpoint.

However, in the case of an array of permissions being passed in, it seems to always fail. The default behaviour of this method is to pass as long as the user has one of the permissions (unless a second parameter of true is passed in).

🦋 Expected Behaviour

If an array of permissions is passed in, without a second parameter of true, then ProtectedRequestUserChecker should evaluate each permission separately. As long as any one permission belongs to the user and is unprivileged, then it should succeed even outside of a privileged request.

🕵️ Details

This is related to #11144 and #11143.

📋 Steps to Reproduce

  1. Open a laravel tinker session php artisan tinker
  2. Find applicant user $applicant = User::where('email', 'applicant@test.com')->first()
  3. Compare the results of $applicant->isAbleTo('view-own-application') and `$applicant->isAbleTo(['view-own-application'])

🙋‍♀️ Proposed Solution

Split the array internally (privileged and unprivileged) and check them separately.

✅ Acceptance Criteria

A set of assumptions which, when tested, verify that the bug was addressed.

tristan-orourke commented 1 month ago

api/tests/UsesProtectedRequestContext.php - this trait will be useful for tests