Joomla! Issue Tracker | Joomla! CMS #10793 - [ACL] Improve panel, solve ajax problems and ACL bugs

? ?

Fixed in Code Base
19 Jun 2016
Medium
Build: staging
# 10793
Diff
andrepereiradasilva:improve-ajax-js-errors

Success

Success continuous-integration/travis-ci/pr The Travis CI build passed Details
Success JTracker/HumanTestResults Human Test Results: 7 Successful 0 Failed. Details

User tests: Successful: Unsuccessful:

andrepereiradasilva
11 Jun 2016

Pull Request for solving several ACL issues.

Summary of Changes

This PR makes several changes in the permissions panel and also replicate js improvements in sendtest mail in order to solves bugs and improve php/js code.

Bugs/Improvements

1: Ajax js errors (json parse, no content, http status error, etc) are not captched in permission.js
After this PR uses the same checks as sendtestmail.js that where now moved to core.js

2: Session token is not being checked in permission.js
After this PR the session token is checked.

3: Permision js ajax call is returned in html mimetype
After this PR the ajax call is returned in json mimetype.

4: Some messages returned by permission and sentestmail js ajax calls are returned in HTML format.
After this PR all messages are returned in Json.

5: The js messages in sendtestmail and permissions.js are not cleaned on change.
After this PR they are cleaned everytime you change.

6: Permission js uses a custom html message system
After this PR all message use the Joomla API message system.

7: A super user cannot remove super user permission from other groups that his not into.
After this PR this is possible.

8: When a super user permission is changed or a permission of a group with child groups is changed, the user is not informed that we needs to refresh the page to recalculate permissions.
After this PR the user is notified.

9: A user that is not Super User can change a Super User group permissions.
After this PR this is not possible.

10: A user that is not Super User can change change its own's groups ot it's parent permissions.
After this PR this is not possible.

11: We don't have calculate permissions column in com_config "Public" (root) group.
After this PR we have the calculated permission here too.

12: Changing a permission with "Not Allowed (Locked)" to "Allowed" or "Denied" changes it's calculate permission (it should stay at "Not Allowed (Locked)").
After this PR the calculated permissions always stays "Not Allowed (Locked)" in this cases.

13: If you change a "Inherited" permission with a calculated permission of "Not Allowed" to "Allowed" and then back to "Inherited", the calculated status is changed to "Allowed" (it should be "Not Allowed (Inherited)").
This PR solves that.

14: There are some "Conflict" messages when you have a parent group with "Denied" and some child group with "Allowed". This should be only "Not Allowed (Locked)"
This PR solves that.

There can be more bugs/improvements that i don't remember...

Scenarios

Calculated Permissions Scenarios

If only "Inherited" across the group tree or group in global config:

"Inherited" -> "Not Allowed (Inherited)" (red).
"Allowed" -> "Allowed" (green).
"Denied" -> "Not Allowed" (red).

If any "Denied" across the group tree or group in global config:

"Inherited" -> "Not Allowed (Locked)" (red with lock).
"Allowed" -> "Not Allowed (Locked)" (red with lock).
"Denied" -> "Not Allowed (Locked)" (red with lock).

If no "Denied" and a "Allowed" across the group tree or group in global config:

"Inherited" -> "Allowed (Inherited)" (green).
"Allowed" -> "Allowed" (green).
"Denied" -> "Not Allowed" (red).

Special case: If group has super User permissions all values in that group and child groups should be "Allowed (Super User)" (green with lock).

Error/Notices Scenarios

Can't change permissions scenarios:

Session token is not valid.
Database query/update errors.
Any error while checking or storing asset.
Current user does not have permissions to change ACL for the component.
Current user is Super User and is trying to remove his own Super User permissions.
Current user is not Super User and is trying to change a Super User group permissions.
Current user is not Super User and is trying to change its own's groups permissions.
Current user is not Super User and is trying to change its own's groups parent permissions.

Ask user to reload the page scenarios:

User changed permissions of a group with child groups.
User changed group permissions to Super User.

Testing Instructions

Use latest staging.
Check all "Bugs/Improvements".
Apply patch
Go to global configuration
Clean browser cache.
Check all "Bugs/Improvements" are solved.
Check all scenarios described in the "Scenarios".
Do the tests two times. With your super user and other user with less permissions (easier if you open a session for that user in a private browser window).

ac2d33c 9 Jun 2016

ajax js error messages

7ae9758 9 Jun 2016

add the js error messages to views

177d2cf 9 Jun 2016

send test mail should send always json

c61bd27 9 Jun 2016

simplify sendmailjs

9a9819e 9 Jun 2016

centralzie ajax errors message sin core

45cad98 9 Jun 2016

message, not sucess

fd87424 9 Jun 2016

don't render messages if null

0d5f209 9 Jun 2016

better json/javascript handling

4dd345d 9 Jun 2016

remove extra debug

3eb91c9 9 Jun 2016

corrrect admin check by component

1510488 9 Jun 2016

app not this->app

9f13208 9 Jun 2016

ups

9893e10 9 Jun 2016

ups ... again

9246cd1 9 Jun 2016

remove messages if they exist

74832a5 9 Jun 2016

remov messages in sendmail too

b3f16d2 9 Jun 2016

to do and superuser doesn't change

a89c953 9 Jun 2016

a super user can demote other groups from being a super user

67bbad9 9 Jun 2016

check if current group has super user permissions

92a4340 9 Jun 2016

language var doesn't exist

81940d0 9 Jun 2016

isUserSuperUser

0a00a9f 9 Jun 2016

we can have child groups ...

37d5be2 9 Jun 2016

remvoe extra spaces

e4ef8fb 9 Jun 2016

if user is not is superuser group, cannot change a superuser group

407f64d 9 Jun 2016

a super user can change other groups

42f3559 9 Jun 2016

improve comments

5d35504 9 Jun 2016

improve messages

996949a 9 Jun 2016

not Locked, should be Inherited

7ad4776 9 Jun 2016

better permissions

b234ecb 10 Jun 2016

alert recalculate

126f797 10 Jun 2016

solves conflicts 1

andrepereiradasilva - open - 11 Jun 2016

andrepereiradasilva - change - 11 Jun 2016

Status

New

⇒

Pending

joomla-cms-bot - change - 11 Jun 2016

Labels

Added: ? ?

andrepereiradasilva - change - 11 Jun 2016

The description was changed

andrepereiradasilva - change - 11 Jun 2016

The description was changed

andrepereiradasilva - change - 11 Jun 2016

The description was changed

andrepereiradasilva - change - 11 Jun 2016

The description was changed

andrepereiradasilva - comment - 11 Jun 2016

pinging @infograf768 @roland-d

andrepereiradasilva - change - 11 Jun 2016

The description was changed

infograf768 - comment - 12 Jun 2016

Please add permissions.min.js to delete in script.php

infograf768 - test_item - 12 Jun 2016 - Tested successfully

infograf768 - comment - 12 Jun 2016

I have tested this item ✅ successfully on 126f797

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

infograf768 - change - 12 Jun 2016

Testing

Super user cannot change it's own permission ✅
Parent group has option set to Denied, Child is set to Denied the message is Not allowed (Inherited) but on page refresh it says Denied ????
If a component has no entry in the asset table, storing a permission creates an invalid asset entry ????
When I change a permission in a parent group I get this message: Notice Permissions changed in a group with child groups. Save or reload to recalculate the child groups permissions. The message is OK, but every other change I make is causing the screen to jump up and down. Would be nicer if the screen wouldn't jump up and down. ????
The correct calculated value is returned for setting Allowed/Denied/Inherited ✅
User cannot change the permission of it's parent or own group ✅
User cannot change a super user permission ✅

@infograf768

Because Andre replaced it by an uncompressed.js one in this patch

I see, usually we have a minified version and a regular version as these are loaded as needed by Joomla depending on debug status. No worries, was just wondering.

infograf768 - comment - 12 Jun 2016

Parent group has option set to Denied, Child is set to Denied the message is Not allowed (Inherited) but on page refresh it says Denied ????

I can't reproduce this (Tested on a single article): It remains on Not Allowed (inherited) here.✅

When I change a permission in a parent group I get this message: Notice Permissions changed in a group with child groups. Save or reload to recalculate the child groups permissions. The message is OK, but every other change I make is causing the screen to jump up and down. Would be nicer if the screen wouldn't jump up and down.

Agree, but I can live with it. ????

If a component has no entry in the asset table, storing a permission creates an invalid asset entry ???? invalid_asset_entry

To test this, I deleted all Permissions in com_banners, just left {}. I had no other asset for com_banners.
Then I edited the permissions of the banners component, forbidding create, delete, edit and edit state to Manager (group id 6) and gave some Allowed permissions for Configure ACL and Configure Options to Adminitrator (group id 7).
Then I saved, got the notice, and Not Allowed (Inherited) showing OK in children groups.

Looking at db, I get

A com_banners category which had no assets is now present in the assets table as
com_banners.category.3 with the correct parent_id

Parents and level look OK to me:

infograf768 - comment - 12 Jun 2016

@roland-d just saw one of your com_banners has an 's' too much

roland-d - comment - 12 Jun 2016

@infograf768 It doesn't have an s too much, I have done that on purpose. You misunderstood the test I did. The test is that if you install a component which has no entry (nothing) in the assets table, the entry will be created but incorrect. You tested an empty param column instead.

Agree, but I can live with it. ?

I can't. I find it very annoying.

I can't reproduce this (Tested on a single article)

I will test this again another time, let's see what others find.

joomla-cms-bot - comment - 12 Jun 2016

This PR has received new commits.

CC: @infograf768

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

andrepereiradasilva - comment - 12 Jun 2016

@roland-d thanks for testing.

4: When I change a permission in a parent group I get this message: Notice Permissions changed in a group with child groups. Save or reload to recalculate the child groups permissions. The message is OK, but every other change I make is causing the screen to jump up and down. Would be nicer if the screen wouldn't jump up and down. ?

Can be easily changed.
But please understand i made that to make sure the user always see the message (mobile for instance). Also remember you're testing and using a LOT of times. IMHO a regular user will not change it so many times.

I will check your other comments.

brianteeman - comment - 12 Jun 2016

It's good I see the messages. Usually I don't see them at all in my screen
size

joomla-cms-bot - comment - 12 Jun 2016

This PR has received new commits.

CC: @infograf768

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

andrepereiradasilva - comment - 12 Jun 2016

Parent group has option set to Denied, Child is set to Denied the message is Not allowed (Inherited) but on page refresh it says Denied ?

Solved.

Also solved another bug. When parent group is set to denied and a child group is Suer user before last commit the calculated permission was "Not Allowed (Inhreited)" (it should be Allowed "(Super User)").

joomla-cms-bot - comment - 12 Jun 2016

This PR has received new commits.

CC: @infograf768

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

andrepereiradasilva - comment - 12 Jun 2016

@roland-d

3: If a component has no entry in the asset table, storing a permission creates an invalid asset entry ? invalid_asset_entry

This already happens before the patch right?

joomla-cms-bot - comment - 12 Jun 2016

This PR has received new commits.

CC: @infograf768

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

infograf768 - comment - 13 Jun 2016

@roland-d
Before last changes from @andrepereiradasilva (which I will test now), I have no issue when installing new component.
Exemple with patchtester

roland-d - comment - 13 Jun 2016

@andrepereiradasilva Yes, this already happens before this patch.
@infograf768 I didn't say I have the issue but was thinking in case you have an issue with your asset table.

infograf768 - comment - 13 Jun 2016

Parent group has option set to Denied, Child is set to Denied the message is Not allowed (Inherited) but on page refresh it says Denied ?

Solved.

That WAS working before. I now systematically get Not Allowed (inherited)

infograf768 - comment - 13 Jun 2016

Also solved another bug. When parent group is set to denied and a child group is Suer user before last commit the calculated permission was "Not Allowed (Inhreited)" (it should be Allowed "(Super User)").

OK here.

andrepereiradasilva - comment - 13 Jun 2016

That WAS working before. I now systematically get Not Allowed (inherited)

And that is how it's supposed to be right?

If any "Denied" across the group tree:

"Inherited" -> "Not Allowed (Inherited)" (red with lock).
"Allowed" -> "Not Allowed (Inherited)" (red with lock).
"Denied" -> "Not Allowed (Inherited)" (red with lock).

(see PR introduction)

andrepereiradasilva - comment - 13 Jun 2016

@andrepereiradasilva Yes, this already happens before this patch.

@roland-d i have no solution for that. I propose to not put a solution for taht in this PR and solve it in another PR.

infograf768 - test_item - 14 Jun 2016 - Tested successfully

infograf768 - comment - 14 Jun 2016

I have tested this item ✅ successfully on 126f797

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

roland-d - comment - 14 Jun 2016

@andrepereiradasilva No problem to put it in another PR. I won't be a common issue but yet still a bug.

roland-d - comment - 16 Jun 2016

@andrepereiradasilva Can you please fix the merge conflicts?

roland-d - change - 16 Jun 2016

Milestone

Added:

roland-d - comment - 16 Jun 2016

@andrepereiradasilva

Also remember you're testing and using a LOT of times. IMHO a regular user will not change it so many times.

Yes, I understand that but even doing 2 or 3 changes it jumps. Would be nice if we could just replace the content as it is.

roland-d - test_item - 16 Jun 2016 - Tested successfully

roland-d - comment - 16 Jun 2016

I have tested this item ✅ successfully on 126f797

I have checked all the permissions before and after. After applying the patch all the ACL changes are as expected.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

roland-d - change - 16 Jun 2016

Status	Pending	⇒	Ready to Commit
Labels

joomla-cms-bot - change - 16 Jun 2016

Milestone

Removed:

roland-d - comment - 16 Jun 2016

Setting to RTC as we have 2 succesful tests

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - change - 16 Jun 2016

Labels

Added: ?

zero-24 - comment - 16 Jun 2016

@andrepereiradasilva there are merge conflicts.

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

rishiv3 - test_item - 16 Jun 2016 - Tested successfully

rishiv3 - comment - 16 Jun 2016

I have tested this item ✅ successfully on 126f797

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

andrepereiradasilva - comment - 16 Jun 2016

@rishiv3 you tested in a not fully completed version.

Please test again

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

andrepereiradasilva - comment - 16 Jun 2016

@roland-d @infograf768 i added a temporary (or not) debug system.

Please enable debug. And check if everything is ok.

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

roland-d - comment - 16 Jun 2016

I did the final testing once more and all looks good, all issues I found before are solved.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

rishiv3 - test_item - 16 Jun 2016 - Tested successfully

rishiv3 - comment - 16 Jun 2016

I have tested this item ✅ successfully on 126f797

tested again and everything looks good.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

zero-24 - comment - 16 Jun 2016

@roland-d can we RTC than?

gunjanpatel - comment - 17 Jun 2016

@zero-24 it is already in RTC.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

roland-d - comment - 17 Jun 2016

If you guys @zero-24 and @gunjanpatel could test this as well it would be appreciated. It will be good to have some extra tests.

infograf768 - comment - 17 Jun 2016

Final testing done here. All looks real fine! Thanks @andrepereiradasilva and @roland-d

joomla-cms-bot - comment - 17 Jun 2016

This PR has received new commits.

CC: @infograf768, @rishiv3, @roland-d

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10793.}

andrepereiradasilva - comment - 17 Jun 2016

@infograf768 done as requested.

It's a small change, just changed the message string and the comment as request from JM.

See 5d72352

brianteeman - change - 17 Jun 2016

Add a Comment

Older
Newer

Joomla! Issue Tracker - CMS

[#10793] - [ACL] Improve panel, solve ajax problems and ACL bugs

Summary of Changes

Bugs/Improvements

Scenarios

Calculated Permissions Scenarios

Error/Notices Scenarios

Testing Instructions

Testing

Add a Comment

Status	Ready to Commit	⇒	Fixed in Code Base
Closed_Date	0000-00-00 00:00:00	⇒	2016-06-19 19:55:21
Closed_By		⇒	wilsonge