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

Change view_ permissions to edit_ permissions for elements in uberbar search #13095

Merged
merged 2 commits into from Jan 27, 2017

Conversation

exside
Copy link
Contributor

@exside exside commented Aug 22, 2016

What does it do?

Actually quite a simple change, all it does is changing the view_ permissions in the search processor to edit_ permissions.

Why is it needed?

If we assume you have some access policies in place where users cannot and should not be confronted with elements nor edit them, so they don't have the permissions to edit, but to view they have, as otherwise they wouldn't be able to fill content into templates, or drag chunks into the content field etc., but for these things, no edit permission is allowed.
So the problem appears when searching with the uberbar and then elements with the search sting in their name pop up as results and the user is tempted to click on them sometimes (at least once...), but what they get then is a nasty, ugly looking "Access denied"-white-error-page-that-looks-broken. And consequentially they assume the system is broken...and I get unnecessary calls over and over (nobody else has that??) =)
The (IMHO) simple solution is to adjust the needed permissions to "edit_" as the search-result-links only function is to EDIT the result right? Hopefully I'm not overlooking something critical here...please enlighten me!

For the more visual types:
modx uberbar permissions

Related issue(s)/PR(s)

none that I know of, but will update if I find one!

@rtripault
Copy link
Contributor

Makes sense!

Looking good from my seat, so 👍

@exside
Copy link
Contributor Author

exside commented Aug 22, 2016

Was thinking if the permission logic should not also apply for resources, currently the code looks like

        if (!empty($this->query)) {
            if (strpos($this->query, ':') === 0) {
                // upcoming "launch actions"
                //$this->searchActions();
            } else {
                // Search elements & resources
                $this->searchResources();
                if ($this->modx->hasPermission('edit_chunk')) {
                    $this->searchChunks();
                }
                if ($this->modx->hasPermission('edit_template')) {
                    $this->searchTemplates();
                }
                if ($this->modx->hasPermission('edit_tv')) {
                    $this->searchTVs();
                }
                if ($this->modx->hasPermission('edit_snippet')) {
                    $this->searchSnippets();
                }
                if ($this->modx->hasPermission('edit_plugin')) {
                    $this->searchPlugins();
                }
                if ($this->modx->hasPermission('edit_user')) {
                    $this->searchUsers();
                }
            }
        }

but why should it not look like

        if (!empty($this->query)) {
            if (strpos($this->query, ':') === 0) {
                // upcoming "launch actions"
                //$this->searchActions();
            } else {
                // Search elements & resources
                if ($this->modx->hasPermission('edit_document')) {
                    $this->searchResources();
                }
                if ($this->modx->hasPermission('edit_chunk')) {
                    $this->searchChunks();
                }
                if ($this->modx->hasPermission('edit_template')) {
                    $this->searchTemplates();
                }
                if ($this->modx->hasPermission('edit_tv')) {
                    $this->searchTVs();
                }
                if ($this->modx->hasPermission('edit_snippet')) {
                    $this->searchSnippets();
                }
                if ($this->modx->hasPermission('edit_plugin')) {
                    $this->searchPlugins();
                }
                if ($this->modx->hasPermission('edit_user')) {
                    $this->searchUsers();
                }
            }
        }

any idea why that permission check was left out there? putting it in didn't have any negative effect if the correct permissions are set, but otherwise nothing is shown, as I'd expect

@rtripault
Copy link
Contributor

Not sure if there was a purpose of not checking permissions for resources (probably a left over from the initial PoC).

All in all, makes sense too (again, from my seat ;p).

@sebastian-marinescu
Copy link
Contributor

+1 – stumbled upon it many times. With this fix the search-bar would make much more sense for editors.

@Jako Jako added type-frontend Issues related to UI/UX issues, mostly about styles and frontend implementations on JavaScript. proposal Proposal about improvement aka RFC. Need to be discussed before start implementation. priority-2-high labels Sep 5, 2016
@Jako Jako added this to the v2.6.0 milestone Sep 5, 2016
@exside
Copy link
Contributor Author

exside commented Sep 9, 2016

Updated PR with the discussed permission for resources

@theboxer theboxer self-assigned this Jan 27, 2017
@theboxer theboxer merged commit ebfe688 into modxcms:2.x Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposal about improvement aka RFC. Need to be discussed before start implementation. type-frontend Issues related to UI/UX issues, mostly about styles and frontend implementations on JavaScript.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants