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

Added additional gate for selectlists #10662

Merged
merged 7 commits into from Feb 11, 2022

Conversation

snipe
Copy link
Owner

@snipe snipe commented Feb 11, 2022

This one is a little fraught, since if something was missed here, it could break all kinds of things - BUT it should at least be pretty straightforward.

Right now, we have a Gate that determine whether you should see the gear icon in the right side. This logic makes sense, because if you have the ability to edit ANY of those things, you need to be able to see the gear icon in order to get the dropdown options in the nav item.

Gate::define('backend.interact', function ($user) {
return $user->can('view', Statuslabel::class)
|| $user->can('view', AssetModel::class)
|| $user->can('view', Category::class)
|| $user->can('view', Manufacturer::class)
|| $user->can('view', Supplier::class)
|| $user->can('view', Department::class)
|| $user->can('view', Location::class)
|| $user->can('view', Company::class)
|| $user->can('view', Manufacturer::class)
|| $user->can('view', CustomField::class)
|| $user->can('view', CustomFieldset::class)
|| $user->can('view', Depreciation::class);
});

This PR introduces a new permission that determines whether you should be able to see the contents of a select list like locations, etc.

The reason for this change is that it comes up occasionally on huntr.dev reports, and while it would be difficult to execute AND you'd have to be a valid user with login privileges on the system, it seems fair enough I suppose to lock that down a little further.

If you can edit assets, for example, even if you're not allowed to view details of a location, you still need to be able to see the select list for it, otherwise you cannot complete the task of creating the asset.

I've tested this locally, but would love additional eyes on it.

Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

The implementation here is pretty clear, and makes sense.

Just to make sure I understand it, let me restate what we're doing here:

Right now, displaying various selectlists are ungated - we will just show them to you, even if you don't have the appropriate 'view' authorization on the related thing that's being listed.

This change makes it so that, if you have view permissions on any class of object in Snipe-IT (asset, license, component, consumable, accessory, or user) then you can get any selectlist you want.

I think that's probably a pretty fair compromise, but should we restrict it further? E.g. each selectlist authorization should be related to whether you can 'view' that thing? Like, If you want to display the selectlist for locations, you need view access on locations. If you want to display the selectlist for assetmodels, you need 'view' on assetmodels? (etc, presuming there are even such permissions that even exist?)

Or should we restrict it to whether or not you have 'view' permissions on all Snipe-IT objects - e.g. to show selectlists, you need 'view' permissions on all of asset, license, component, consumable, accessory, and user?

Once we've decided that this is the best compromise, I'll be happy to sign off on it.

@snipe
Copy link
Owner Author

snipe commented Feb 11, 2022

I think that's probably a pretty fair compromise, but should we restrict it further? E.g. each selectlist authorization should be related to whether you can 'view' that thing? Like, If you want to display the selectlist for locations, you need view access on locations.

Nope - if that would work, we wouldn't need the extra gate, we could just use the location, etc view gates.

If a user can edit assets but wasn't granted the ability to view asset models details via the asset model permissions, they would never be able to save the new or edited asset, since the asset models dropdown would never be populated, therefore would be null, and would then be a missing required field.

Make sense?

As I said, it's a little confusing, but you need to load those select lists in order to be able to complete the form to create other (main) models like assets, components, etc.

@snipe
Copy link
Owner Author

snipe commented Feb 11, 2022

Or should we restrict it to whether or not you have 'view' permissions on all Snipe-IT objects - e.g. to show selectlists, you need 'view' permissions on all of asset, license, component, consumable, accessory, and user?

This would break the dropdowns for users who only have access to edit/add assets, not not licenses, etc.

@snipe
Copy link
Owner Author

snipe commented Feb 11, 2022

What we're really dealing with here is that you need permissions to at least list on what I might call a "second class" model (Locations, Asset Models, Categories, etc) in order to create or edit "first class" models (like Assets, Licenses, etc).

We don't want to add those on as separate "list" permissions on those second-class models, because they're not really optional. You MUST be able to load those dropdowns if you want to create/edit first-class models.

Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

OK, I think I get it.

What you're saying is that if you grant 'create' permissions for, say, assets - then it's only fair for us to say "well there's no way to create an asset in the web GUI unless you can actually list categories, etc"

If so, should the final Gate for view.selectlists instead be asking if we can create (or update) any of those things? If you granted only view permissions for everything to someone (but no create or edit permissions) it'd be fair to say you have no need for selectlists, right?

@snipe
Copy link
Owner Author

snipe commented Feb 11, 2022

What you're saying is that if you grant 'create' permissions for, say, assets - then it's only fair for us to say "well there's no way to create an asset in the web GUI unless you can actually list categories, etc"

bingo-teachers-series

Exactly right.

If so, should the final Gate for view.selectlists instead be asking if we can create (or update) any of those things? If you granted only view permissions for everything to someone (but no create or edit permissions) it'd be fair to say you have no need for selectlists, right?

Good point. Unfortunately, that doubles the size of that gate, since we'd need create or edit perms.

I'll update and re-push.

Signed-off-by: snipe <snipe@snipe.net>
@snipe snipe requested a review from uberbrady February 11, 2022 20:31
Signed-off-by: snipe <snipe@snipe.net>
uberbrady
uberbrady previously approved these changes Feb 11, 2022
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

This looks good - as we discussed it'd be nice if we could say:

$user->can(['create','update'], Asset::class);

But that's just a nice-to-have, not a must-have. Approved.

Signed-off-by: snipe <snipe@snipe.net>
@snipe snipe merged commit 10c26f3 into master Feb 11, 2022
@snipe snipe deleted the fixes/tighter_security_on_select_lists branch February 11, 2022 20:48
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.

None yet

2 participants