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

Fix: Non admin can not crop avatar #155

Merged
merged 2 commits into from
Nov 28, 2022
Merged

Fix: Non admin can not crop avatar #155

merged 2 commits into from
Nov 28, 2022

Conversation

jayedul
Copy link
Contributor

@jayedul jayedul commented Sep 22, 2022

Description of the Change

In this pull request we've just unregistered all the setup_theme action hook at application runtime during ajax call only when it is a crop request. As I debugged backtrace, I found this hook execution (https://github.com/WordPress/WordPress/blob/master/wp-settings.php#L549) exits somewhere in case of non admin users.

I definitely agree it's a very important hook, but as long as we are dealing with only image crop request, so I believe here's nothing to do with theme related stuffs. That's why cleared the hook for the crop request only.

The issue description meant to integrate another crop tool other than the core one. But that would add more dev stacks. So
before proceeding new stacks, this pull request just proposes a simpler fix for the original issue #141
If it doesn't seem an appropriate solution, then no issue to adopt a new crop tool.

Closes #150 and #141

How to test the Change

  1. Go to user edit screen as like as you do from admin account
  2. Go to avatar section and click the button Choose From Media Library
  3. Select Avatar and Crop
  4. Crop a portion as your wish
  5. Click Crop button
  6. Previously this is where you faced the error, but now it should save as expected.

Changelog Entry

Fix: Non admin can not crop avatar

@jeffpaul jeffpaul added this to the 2.7.0 milestone Sep 22, 2022
@jeffpaul jeffpaul requested review from faisal-alvi and removed request for jeffpaul September 22, 2022 16:16
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@jayedul this is awesome! Working fine!

@dkotter Code LGTM but would like to know your opinion too before merging.

@dkotter
Copy link
Collaborator

dkotter commented Sep 26, 2022

So if I'm following correctly, the crop tool we're currently using is also used within the customizer and it's limited there to only work for admins, correct?

If so, my concern here is that it seems with the changes here, we would also allow non-admin users the ability to crop within the customizer (though I haven't tested that). If that is the case, is there some way to fix that? For instance, could we add some unique data to our crop request that we can look for and only remove the setup_theme hook in that case? That way we can be sure this will only impact crop requests coming from this plugin.

@faisal-alvi
Copy link
Member

@dkotter Only users with the "customizer" cap can access the customizer area. And as per the Roles and Capabilities doc, the "customize" cap is only with the Super Admins and Administrators user roles. I assume this resolves your concern.

@faisal-alvi
Copy link
Member

@dkotter any thoughts on my previous comment?

@faisal-alvi faisal-alvi added the needs:feedback This requires feedback to determine next steps. label Nov 17, 2022
@dkotter
Copy link
Collaborator

dkotter commented Nov 18, 2022

@dkotter any thoughts on my previous comment?

@faisal-alvi Okay, I think we're probably good to go here then. Still makes me a little wary but as long as we've tested with different account levels and haven't seen any issues with users having access they shouldn't have, should be good to go

@faisal-alvi
Copy link
Member

I've tested again and the "Appearance > Customize" is only accessible for the users with the "customizer" cap. And only admins and super admins have that cap.

@faisal-alvi faisal-alvi marked this pull request as ready for review November 28, 2022 06:31
@faisal-alvi faisal-alvi merged commit d1000c4 into develop Nov 28, 2022
@faisal-alvi faisal-alvi deleted the fix/150 branch November 28, 2022 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:feedback This requires feedback to determine next steps.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alternate Cropping Solution
4 participants