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

Do not redirect in a JSON call but return a JSON string #10763

Merged
merged 1 commit into from
Jun 9, 2016

Conversation

roland-d
Copy link
Contributor

@roland-d roland-d commented Jun 9, 2016

Summary of Changes

The storing of ACL permissions is always done through AJAX. When a user doesn't have permissions a redirect to the index.php happens but this has no effect in an AJAX call causing the call to die with no notice.

This change fixes that by returning a JSON call with the message to show to the user.

This is not the fix for allowing this user to make the change, just deal with the incorrect redirect.

Testing Instructions

  1. Login with a user that is part of the Manager/Administrator/or subgroup of this
  2. Go to Content -> Articles
  3. Click on the Options tab
  4. Try to change a permission, there will be no answer from the server. The AJAX call has died with a redirect which can't be parsed.
  5. Apply the patch
  6. Try to change the permission again and now you will get the notice
    no_access
  7. Problem solved

Pretty please @andrepereiradasilva and @infograf768

@roland-d roland-d added this to the Joomla 3.6.0 milestone Jun 9, 2016
@infograf768
Copy link
Member

I have tested this item ✅ successfully on 2b88f07

This works. No spin anymore.
Obviously we now need to solve the fact that it happens when an administrator wants to change the permissions of a subgroup or a site-only group.
I know you are on it.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10763.

@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ successfully on 2b88f07

Works as descrived.
Also confirmed JM results.

The error message itself we could do better (permission to change, not view), but that is another PR.

Found another bug.
Changed the group permisison to be superuser and now i can't remove it anymore.
I get the message "You can't remove your own Super User permissions."


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10763.

@brianteeman
Copy link
Contributor

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10763.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 9, 2016
@andrepereiradasilva
Copy link
Contributor

@roland-d we should have a nicer way to solve this and all permission js ajax issues.

Already done that for sendtestmail js.

IMHO what we really need is to catch the errors in the js (see https://github.com/joomla/joomla-cms/blob/staging/media/system/js/sendtestmail-uncompressed.js#L33-L66).

So if we follow that path this change should not be done as it would be capched by the js as a HTTP 403 error. You can see that happening in sendtestmail
See https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_config/controller/application/sendtestmail.php#L27-L37

I don't have time right now, but i can look into this when i have time.

So if you want to go ahead and merge this, ok, it can be easily reverted in my future PR.

@roland-d
Copy link
Contributor Author

roland-d commented Jun 9, 2016

@andrepereiradasilva You won't solve all the AJAX issues this way because they are completely unrelated except this one change.

Further I disagree actually by putting back the old code and handle it via JS. The actual AJAX call is successful, the result of the operation isn't. When we are doing an AJAX request, Joomla shouldn't return an HTML page. The JSON response object has the option to return a message, so that is what is being used now.

Finally, I do agree with adding the all the extra checks to the JS code in case the AJAX call is not successful.

@infograf768
Copy link
Member

@andrepereiradasilva
Next PR from @roland-d is here:
#10764

still to be completed.

@andrepereiradasilva
Copy link
Contributor

andrepereiradasilva commented Jun 9, 2016

You won't solve all the AJAX issues this way because they are completely unrelated except this one change.

I know. Only ajax js issues. what i mean is when there is no error message because of ajax json issues.

Further I disagree actually by putting back the old code and handle it via JS. The actual AJAX call is successful, the result of the operation isn't. When we are doing an AJAX request, Joomla shouldn't return an HTML page. The JSON response object has the option to return a message, so that is what is being used now.

IMHO Joomla should return 403 HTTP status page if it was no permissions.
Of course, i agree with you, from a page content rendering perspective it should return a 403 in correct json if the mimetype is json and a 403 in in html if the mimetype is html and so on.
But also think, that should be done at rendering engine level.

Finally, I do agree with adding the all the extra checks to the JS code in case the AJAX call is not successful.

Also the mimetype of the retuned AJAX call should be application/json and so on. I will make all those changes when i have time to make the PR.

@wilsonge wilsonge merged commit 1a58f3d into joomla:staging Jun 9, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 9, 2016
@roland-d roland-d deleted the fix-acl-store-redirect branch January 29, 2017 19:47
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.

6 participants