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

check if user has access to the default asset location #13010

Conversation

i-just
Copy link
Contributor

@i-just i-just commented Mar 29, 2023

Description

When the assets field has a default assets location set, check if the user has permission to viewVolume before setting the defaultSource and defaultSourcePath template variables.

Please let me know if you need me to raise a separate PR for Craft 4.

Related issues

#13006

@i-just i-just requested a review from a team as a code owner March 29, 2023 16:01
@brandonkelly
Copy link
Member

This seems kindof at odds with these lines, which explicitly authorizes the user to access the upload folder, regardless of permissions:

cms/src/fields/Assets.php

Lines 638 to 639 in bff6946

$folder = $this->_uploadFolder($element, false);
Craft::$app->getSession()->authorize('saveAssetInVolume:' . $folder->getVolume()->uid);

Maybe that’s just not working as intended currently?

@i-just
Copy link
Contributor Author

i-just commented Mar 31, 2023

I tracked it down to enforcing defaultSourcePath for Element Index.

In 3.7, it worked as expected. The defaultSource was still set to what’s defined in the field’s settings, but it was then “ignored” in favour of showing the volume user has permission to view. If a user didn’t have permission to view any volumes, they could see the field in the entry layout but not modify it.

In 3.8, the defaultSource and defaultSourcePath are enforced even if a user cannot view the volume those two reference. It then caused the criteria for the element query to contain a folderId for an unpermitted volume. PHP side of things works roughly the same, but because of this: https://github.com/craftcms/cms/blob/v3/src/web/assets/cp/src/js/BaseElementIndex.js#L306, the defaults were enforced regardless of permissions.

I reverted the previous change and committed a new fix.

…when-default-location-set

# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
[ci skip]
@brandonkelly brandonkelly self-requested a review as a code owner April 4, 2023 00:24
@brandonkelly brandonkelly merged commit 6b7b8a4 into v3 Apr 4, 2023
@brandonkelly brandonkelly deleted the bugfix/13006-assets-field-respect-permissions-when-default-location-set branch April 4, 2023 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants