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.

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

PIMS-2082 Backend Authorization Updates #2679

Merged
merged 17 commits into from
Sep 18, 2024
Merged

PIMS-2082 Backend Authorization Updates #2679

merged 17 commits into from
Sep 18, 2024

Conversation

dbarkowsky
Copy link
Collaborator

@dbarkowsky dbarkowsky commented Sep 13, 2024

🎯 Summary

PIMS-2082

Goal

  • Ensure there is a consistent way to check the database for user role-based permissions
  • Remove role checks that used the Keycloak roles (found in the req.user object)

Changes

  • Expanded upon the old activeUserCheck middleware. Renamed it to userAuthCheck. It now performs role checking functions as well. It also contains a function called hasOneOfRoles that you can use for role checking in other API locations. It then adds the user from the database plus this function to the request. It can be accessed liked req.pimsUser.
  • This userAuthCheck is applied in the paths to the routes. It must be proceeded by the protectedRoute middleware or it will not have the user.preferred_username field to look up the database user. In cases where all the routes in an area (like agencies) were Admin protected, I added it to the express.ts file, but if there were different permissions for each route in an area, I added it to the route files. Open to preference on this if we wanted it to be a little more repetitive in some cases but keep all these checks in the route files.
  • Replaced any locations where we were using the Keycloak roles for authorization checking.
  • Replaced unneeded request user lookups. Now that the user's info is passed down with the request, there were a lot of getUser calls that could be omitted.
  • Updated tests to match this new system

Use Example

router
  .route('/myRoute')
  .get(userAuthCheck(), catchErrors(myController))  // Active user required, any role
  .post(userAuthCheck({ requiredRoles: [Roles.ADMIN] }), catchErrors(myOtherController); // Same, but Admin role required.

Testing

With your user already in the database:

  • Try sending requests to a few different endpoints with different roles. You have to change the roles in the database now. Changing them in Keycloak will not have an effect on the backend checks.
  • Can use the react-app to view pages and see if you are locked out from certain requests. This can be a little odd, as the react-app is still using checks for Keycloak roles.

🔰 Checklist

  • I have read and agree with the following checklist and am following the guidelines in our Code of Conduct document.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation where required.
  • I have tested my changes to the best of my ability.
  • My changes generate no new warnings.

Copy link

codeclimate bot commented Sep 17, 2024

Code Climate has analyzed commit 99f3346 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 95.4% (50% is the threshold).

This pull request will bring the total coverage in the repository to 93.2%.

View more on Code Climate.

Copy link
Collaborator

@LawrenceLau2020 LawrenceLau2020 left a comment

Choose a reason for hiding this comment

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

Seems to work, after changing my role to general user, I'm only able to see properties under my agency and below, tested updating my user and I get a 403 error, and tested viewing another building and get a 403 and redirected to the map page.
Tested as an auditor as well, and the api is preventing me from making changes as expected.

@dbarkowsky dbarkowsky merged commit 3afc60a into main Sep 18, 2024
9 checks passed
@dbarkowsky dbarkowsky deleted the PIMS-2082 branch September 18, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants