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

First fix for user FMCS scoping #14591

Merged
merged 21 commits into from
Apr 17, 2024
Merged

Conversation

snipe
Copy link
Owner

@snipe snipe commented Apr 11, 2024

This corrects some of the scoping we were doing around users for exporting lists, editing, etc and adds a lot more info around FMCS in the code (because it's always miserable to try to detangle that hot mess.)

We cannot use the standard CompanyableTrait that we use everywhere else on the user model because it's a global scope, and that causes an infinite loop by Laravel as it tries to add that scope onto the user model of the currently logged in user, in addition to the users listings/view/etc. This took me way too long to remember, but it explains why we handle the scoping on the User model slightly differently.

I also moved the scoping methods in the Company model to the bottom of the file, to keep it consistent with the way we handle query scopes elsewhere.

I'd like to continue working on a better way to handle this so that we get the same benefits of the CompanyableTrait without all the complex manual query scoping we have to do for the User model now, but this solves the immediate issue.

How to test:

Create a company, "Brand Spankin New", and three users:

  • spankinadmin1 (admin with "Brand Spankin New" as their company)
  • nospankadmin2 (admin without any company)
  • spankuser1 (regular user with "Brand Spankin New" as their company)
  • spankin asset (asset belonging to "Brand Spankin New")

Easiest way to test this is using three browsers so you have three different sessions. Make sure you have Full Multiple Company Support enabled.

spankinadmin1:

  • spankinadmin1 - can only see all of the assets and users within "Brand Spankin New"
  • If you fuzz the url to change the /users/{:id} to a user you know doesn't belong to "Brand Spankin New", you should get kicked back with a not found error.
  • You should be able to view/edit/clone/delete users and assets within "Brand Spankin New"
  • Exporting the user list (via the Export button above the table) should ONLY show users that belong to "Brand Spankin New"

nospankadmin2:

  • spankinadmin1 - can only see all of the assets and users without a company assigned
  • If you fuzz the url to change the /users/{:id} to a user you know does have a company association, you should get kicked back with a not found error.
  • You should be able to view/edit/clone/delete users and assets that have no companies associated
  • Exporting the user list (via the Export button above the table) should ONLY show users belonging to no company

Regular superadmins should of course have access to everything they would normally have access to.

Fixes #14539, Fixes [sc-25183], [sc-25258]

Signed-off-by: snipe <[email protected]>
Copy link

what-the-diff bot commented Apr 11, 2024

PR Summary

  • Enhancement in UserController
    The delete user functionality in our system has been improved. Now, it validates whether a user is linked to any other model before it attempts to delete the user.

  • Company Model Adjustments
    Changes have been made in the logic that our system uses to handle company-related data. We've simplified it by merging two functions into one, and also made some improvements to the way children companies are managed within our system.

  • Permission Clarity in SnipePermissionsPolicy
    We've added comments in the code which give a clearer understanding of how permissions are checked within our system.

  • Improved Error Messages
    We've updated some of our error messages to be more descriptive. This includes when a user doesn't exist, and also if a superadmin is trying to update user information in a demo environment. These improved messages will provide a better user experience by making error conditions clearer.

@snipe snipe changed the title Bug/sc 25258/naive fix for user scoping First fix for user FMCS scoping Apr 11, 2024
@snipe
Copy link
Owner Author

snipe commented Apr 11, 2024

This has broken a few of our automated tests - trying to determine why now.

@bzeus
Copy link
Collaborator

bzeus commented Apr 11, 2024

Test results are spot on so far.

snipe added 2 commits April 17, 2024 09:26
Signed-off-by: snipe <[email protected]>
@snipe
Copy link
Owner Author

snipe commented Apr 17, 2024

@bzeus - are you also testing the API or just the GUI?

@snipe
Copy link
Owner Author

snipe commented Apr 17, 2024

@bzeus can you test again - I just pushed through some changes

@marcusmoore - can you take a look at the tests I added and make sure I'm not doing anything too crazy?

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

Tests look good 👍🏾

One minor suggestion on clarity.

Side note: it's best practice to only run one request per test case but I'm guilty of doing exactly what you did here without issue so :shipit:

tests/Feature/Api/Users/UpdateUserApiTest.php Outdated Show resolved Hide resolved
@snipe
Copy link
Owner Author

snipe commented Apr 17, 2024

Side note: it's best practice to only run one request per test case but I'm guilty of doing exactly what you did here without issue so :shipit:

I know, I get it. :( But the build up and tear down would be identical if I made them separate tests, so... damned if you do, damned if you duplicate a bunch of code. :(

@snipe snipe requested a review from marcusmoore April 17, 2024 19:31
@bzeus
Copy link
Collaborator

bzeus commented Apr 17, 2024

Tests are on point, works as it should.

@snipe snipe merged commit 6f195cb into develop Apr 17, 2024
8 checks passed
@snipe snipe deleted the bug/sc-25258/naive_fix_for_user_scoping branch April 17, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants