Fix full company scoping in user selects #12467

marcusmoore · 2023-02-06T21:00:28Z

Description

This PR fixes a bug in the user select menu that currently returns users from other companies when a search string is provided.

Example:
With Full Companies Support enabled, Marcus is an admin for Company A. Marcus attempts to sign out an asset that belongs to Company A. When selecting the user to check the asset out to, Marcus is presented with a list of users for Company A:

So far so good 👍🏾

But, when Marcus types a query into the user search, he sees users from other companies 👎🏾

With this PR, the users returned are scoped to the user's of the company that the requester belongs to:

Notes

For super admins, the existing behavior remains and all users are returned.
For clarity, if an asset is assigned to a company the company name is displayed at the top of the checkout form (as seen in screenshot above).
I added a change to the SecurityHeaders middleware that skips it when running in testing environments.

On Testing

A few tests have been included in this PR but running the tests requires some changes that are outside of the scope of this PR and will be followed up on in the future:

The DB_* variables in .env.testing-ci should point to a valid MySQL database
DB_CONNECTION in phpunit.xml should be mysql
You'll also need to temporarily bypass the SecurityHeaders middleware

Type of change

Bug fix (non-breaking change which fixes an issue)

what-the-diff · 2023-02-06T21:00:40Z

Added a new test for the users select list API endpoint.
Fixed an issue where assets with no model would cause errors in asset checkout view and added company name to that page as well.
Removed unwanted headers from responses when running tests on CI/CD servers (GitLab, Travis).
Updated factories so they can be used by other classes without having to pass them around everywhere or use global state which is bad practice anyway! Also updated some of the existing factory definitions while I was at it... :)
Changed language file key 'company' => 'Company'. This will make things easier if we ever want to change this label later on down the line since all references are now using Laravel's translation helper function trans(). It also makes more sense than just hardcoding "Company" into our views like before because then you have two places where you need to update your code instead of one place only - not DRY :( . The same goes for any other labels throughout Snipe-IT too btw ;) !

marcusmoore · 2023-02-06T23:59:30Z

app/Http/Middleware/SecurityHeaders.php

+        if (App::environment(['testing', 'testing-ci'])) {
+            return $next($request);
+        }
+


Reviewers: double-check that this check makes sense please. We don't want accidentally disable this middleware 😅

Why would we want to bypass this in testing? (I'm sure there's a good reason for it, I'm just not coming up with it :) )

Great question that made me dig in a little more.

Calling header_remove in this middleware causes the tests to throw exceptions with the following error:

ErrorException : Cannot modify header information - headers already sent by (output started at /path/to/project/vendor/phpunit/phpunit/src/Util/Printer.php:104) /path/to/project/app/Http/Middleware/SecurityHeaders.php:112 /path/to/project/app/Http/Middleware/SecurityHeaders.php:30 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 /path/to/project/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/TransformsRequest.php:21 /path/to/project/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/ConvertEmptyStringsToNull.php:31 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 /path/to/project/app/Http/Middleware/CheckForDebug.php:25 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 /path/to/project/app/Http/Middleware/CheckForSetup.php:25 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 /path/to/project/vendor/fideloper/proxy/src/TrustProxies.php:57 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 /path/to/project/vendor/laravel/framework/src/Illuminate/View/Middleware/ShareErrorsFromSession.php:49 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 /path/to/project/vendor/laravel/framework/src/Illuminate/Session/Middleware/StartSession.php:121 /path/to/project/vendor/laravel/framework/src/Illuminate/Session/Middleware/StartSession.php:64 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 /path/to/project/vendor/laravel/framework/src/Illuminate/Foundation/Http/Middleware/PreventRequestsDuringMaintenance.php:86 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 /path/to/project/app/Http/Middleware/NoSessionStore.php:28 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:167 /path/to/project/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php:103 /path/to/project/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php:142 /path/to/project/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php:111 /path/to/project/vendor/laravel/framework/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php:510 /path/to/project/vendor/laravel/framework/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php:476 /path/to/project/vendor/laravel/framework/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php:306 /path/to/project/tests/Feature/Api/Users/UsersForSelectListTest.php:25

That if check in such an important middleware is sketchy though. I'm pulling it out of this and adding a comment about it in the PR description.

snipe

This looks great, Marcus - thank you! A few small nits (and a question), but it's great work thank you!

snipe · 2023-02-15T19:58:56Z

resources/lang/en/admin/companies/general.php

@@ -1,6 +1,7 @@
 <?php

 return [
+    'company' => 'Company',


I think we can just use general.company here, no need to duplicate the string.

snipe · 2023-02-15T20:00:54Z

app/Http/Middleware/SecurityHeaders.php

+        if (App::environment(['testing', 'testing-ci'])) {
+            return $next($request);
+        }
+


Why would we want to bypass this in testing? (I'm sure there's a good reason for it, I'm just not coming up with it :) )

snipe · 2023-02-15T20:01:18Z

resources/views/hardware/checkout.blade.php

@@ -26,14 +26,23 @@
                    </div>
                    <div class="box-body">
                    {{csrf_field()}}
+                        @if ($asset->company && $asset->company->name)
+                            <div class="form-group">
+                                {{ Form::label('model', trans('admin/companies/general.company'), array('class' => 'col-md-3 control-label')) }}


Same thing as above general.company should work find here.

marcusmoore added 11 commits January 24, 2023 11:54

Logically group query when searching users to scope to company

b92d1d3

Scaffold tests for user select list api and begin to implemenet

a2e47d1

Continue implementing tests

89eff23

Implement test for searching users

93a62c8

Improve readability

d8e92d2

Clear some extra lines

9a0c66b

Skip security headers middleware when testing

0cefc7d

Add asset's company name to checkout page

78343dd

Eager load company

38cdcdf

Move company name to better location

45e8e9b

Add translation for Company

0e57e48

probot-autolabeler bot added api backend frontend labels Feb 6, 2023

marcusmoore commented Feb 6, 2023

View reviewed changes

Be more explicit in test case

c030799

marcusmoore marked this pull request as ready for review February 7, 2023 00:07

marcusmoore requested a review from snipe as a code owner February 7, 2023 00:07

snipe requested changes Feb 15, 2023

View reviewed changes

marcusmoore added 4 commits February 22, 2023 12:02

Use existing language translation string

aae2106

Remove middleware bypass

931ca98

Remove extra line

7667b0a

Remove unused import

f63710f

marcusmoore requested a review from snipe February 22, 2023 20:36

snipe merged commit eb63576 into snipe:develop Feb 23, 2023

marcusmoore deleted the fix/scope-people-with-full-multiple-companies branch February 23, 2023 18:37

snipe mentioned this pull request Apr 16, 2023

Dropdown Menu's non functional #12829

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix full company scoping in user selects #12467

Fix full company scoping in user selects #12467

marcusmoore commented Feb 6, 2023 •

edited

what-the-diff bot commented Feb 6, 2023

marcusmoore Feb 6, 2023

snipe Feb 15, 2023

marcusmoore Feb 22, 2023

snipe left a comment

snipe Feb 15, 2023

snipe Feb 15, 2023

snipe Feb 15, 2023

Fix full company scoping in user selects #12467

Fix full company scoping in user selects #12467

Conversation

marcusmoore commented Feb 6, 2023 • edited

Description

Notes

On Testing

Type of change

what-the-diff bot commented Feb 6, 2023

marcusmoore Feb 6, 2023

Choose a reason for hiding this comment

snipe Feb 15, 2023

Choose a reason for hiding this comment

marcusmoore Feb 22, 2023

Choose a reason for hiding this comment

snipe left a comment

Choose a reason for hiding this comment

snipe Feb 15, 2023

Choose a reason for hiding this comment

snipe Feb 15, 2023

Choose a reason for hiding this comment

snipe Feb 15, 2023

Choose a reason for hiding this comment

marcusmoore commented Feb 6, 2023 •

edited