Access service v2

[Elfangor93]

What is this service for

This is a redo of PR #104 since the old PR is somehow messed up and not working anymore...

The Access service will be used to extend the default ACL logic of Joomla core in order to allow checks against inown and own access rules. Inown and own access rules are used in JoomGallery to manage ACL exceptions. For example, a user may perform a certain task within an asset of which he is the creator (owner), even if this task is generally forbidden for him.

How to use this service

You can load the service anywhere in Joomla by doing

$component = Factory::getApplication()->bootComponent('com_joomgallery');  // boot JoomGallery component
$component->createAccess();   // instantiate the service
$acl = $component->getAccess();   // load the service into your scope
$acl->setUser(<userID>); // perform checks against a specified user. If no user is set the current user will be used.
$res = $acl->checkACL(<action>, <asset>, <ID>); // check if the user is permitted to perform <action> on <asset> with the ID <ID>

The following actions are available: add, admin, delete, edit, editstate, manage

The following assets are available: image, category, tag, config

How to test the service

Make sure "Debug System" is "Yes" in Joomla Global configuration. Open the file /administrator/com_joomgallery/src/View/Faulties/HtmlView.php and insert the following code on line 62 after throw new \Exception(implode("\n", $errors)); }

Different test instructions see comments below...

[Elfangor93]

Test Methodology

\: User ID (integer) - Choose a user for which to perform the ACL checks \: The action you want to perform the ACL check (string) \: The asset for which you want to perform the ACL check (optional, string, default: "") \: The id of the asset (optional, integer, default: 0)

$this->component->createAccess();
$acl = $this->component->getAccess();
$acl->setUser(<USERID>); // optional
$res = $acl->checkACL(<ACTION>,<ASSET>, <ID>);
dump($res);

Test Instructions

1. Apply the the code from above as described in the first comment.

2. Prepare component and settings

   • Create a user (additional to the Super user)
   • Create a category (additional to default category)
   • Create at least one asset each (image, category, tag, config-set)
   • Apply permission options accordingly
   • • Global component permission options
   • • Category permission options
   • • Asset permission options

3. Perform the test by reloading the maintenance view and check if the output is correct. false: permission denied true: permission accepted

[Elfangor93]

@szepty-ziemi Please test the access service in this PR...

[szepty-ziemi]

@Elfangor93 I have follow instructions. here is my code:

I have user with ID 698 and Image with ID 1

Checking maintenance I have error:

Is it my mistake?

[Elfangor93]

The error happens in line 88 of the HtmlView.php file. It seems to me as if there was old testing code below your new entered one...

[szepty-ziemi]

Before installed this PR I removed previous versions of JG by uninstalling it. This is a fresh file just modified with code from instruction:

[Elfangor93]

You can delete line 74 - 88. Seems like old debug code of mine. Sorry for that...

[szepty-ziemi]

Okay, now it works perfectly. I have questions regarding actions: admin and manage. It seem that whatever I choose they are always false. Even if user is SU. I checked for image and config but it seems that this status apply for every asset.

Should they be related somehow with these options?

[szepty-ziemi]

As for "delete" we have two options "delete" and "delete own". Should both return true if enabled for user?

Currently $res = $acl->checkACL('delete','image','1');

• Returns "true" when Delete is Allowed
• Returns "false" when Delete Own is Allowed but Delete is Not Allowed.

This apply to all "Own" settings.

[szepty-ziemi]

Okay, I tested and in general - except my questions above - it looks good.

Another question is: do category permissions influence image permissions that reside in that category?

[Elfangor93]

Currently $res = $acl->checkACL('delete','image','1'); Returns "true" when Delete is Allowed Returns "false" when Delete Own is Allowed but Delete is Not Allowed.

How does JG3 behaves in this example? I would expect the own permissions having higher priority and overriding the general one. So your example should always return true as soon as Delete Own is set and the current user is the owner, right? @MrMusic

[Elfangor93]

I have questions regarding actions: admin and manage. It seem that whatever I choose they are always false. Even if user is SU.

@szepty-ziemi The thing is that the actions admin and manage do only exist on the component global level. You can not permit or deny backend access (manage) or ACL access (admin) for specific assets individualy. I improved the code now in such a way that an exception is trown telling you that you are trying to do something impossible instead of just returning always false.

The correct way of checking access for the actions admin and manage is by doing $res = $acl->checkACL('manage', 'com_joomgallery'); or the short way $res = $acl->checkACL('manage');

[szepty-ziemi]

How does JG3 behaves in this example? I would expect the own permissions having higher priority and overriding the general one. So your example should always return true as soon as Delete Own is set and the current user is the owner, right?

I confirm, it works that way. User just needs to be an owner of the asset.

[szepty-ziemi]

@szepty-ziemi The thing is that the actions admin and manage do only exist on the component global level. You can not permit or deny backend access (manage) or ACL access (admin) for specific assets individualy. I improved the code now in such a way that an exception is trown telling you that you are trying to do something impossible instead of just returning always false.

Okay, now we have an error and that's ok.

The correct way of checking access for the actions admin and manage is by doing $res = $acl->checkACL('manage', 'com_joomgallery'); or the short way $res = $acl->checkACL('manage');

Yes, this works perfectly!

[MrMusic]

So your example should always return true as soon as Delete Own is set and the current user is the owner, right? @MrMusic

Correct, once the current user is the owner, the 'own' permissions should take precedence over the general permissions.

[Elfangor93]

I have found out that the permissions were not inherited correctly since the asset table was not created correctly. I had to rewrite a big part of the code such that the permissions gets inherited correctly based on the asset table.

In order for all the changes to apply, you have to do a fresh JoomGallery installation since the asset table needs to be recreated from scratch.

Example

Categories

Uncategorised (Owner: admin) -- MyCategory (Owner: user)

Images

FirstImage (Owner: user, Category: MyCategory)

The usergroup of user has the permission to delete, defined in the permissions tab of the category MyCategory. Now we check the access right for the user with userneme user to the action delete on FirstImage.

Behavior before the changes

The access for this action was denied, since core.delete is denied through inheritance. FirstImage is no child of MyCategory in the asset table.

Behavior after the changes

The access for this action is accepted, since core.delete gets accepted through inheritance from MyCategory. FirstImage is a child of MyCategory in the asset table.

[Elfangor93]

Currently $res = $acl->checkACL('delete','image','1'); Returns "true" when Delete is Allowed Returns "false" when Delete Own is Allowed but Delete is Not Allowed.

@szepty-ziemi Could you check the service once more? I have changed quite a lot of things. This with delete and delete own should now also work correctly.

[Elfangor93]

@szepty-ziemi Do you think you are through with testing this until Monday? I would like to merge it by then and move forward if you do not find something serious.

[szepty-ziemi]

@Elfangor93 yes, I'll do the testing over Sunday. Will let you know.

[szepty-ziemi]

I downloaded latest version. Removed previously installed and installed the newest one. Created few assets and started with testing code. Got an error on start:

My code:

[Elfangor93]

@szepty-ziemi Sorry for that. Should be fixed now.

[szepty-ziemi]

Round 2 of round 3 :) This time call to undefined method in access.php

[Elfangor93]

@szepty-ziemi Sorry for that. Should be fixed now.

[szepty-ziemi]

New one ;)

[Elfangor93]

Well done @szepty-ziemi thats a serious bug. Give me some time to fix it. In all my tests I never faced that one...

[Elfangor93]

But beside the fact that you found a general error in the code, to be honest the command you used does not really make sense.

$res = $acl->checkACL('add','image', 1);

This command translates to: Check if I am allowed to add an image with the ID 1. But when you want to add a new image you probably do not know yet its ID since the ID gets created after adding the image. The ID we need to pass to the Access service in this case should be the category ID into which we wanna add the image. I have to think about that.

[szepty-ziemi]

Actually I agree :) Previously this setup worked without error. Also in image permissions we have "Create":

In regard whet you've said, this permission could be removed for image.

[Elfangor93]

True. The action create inside the permissions tab of one image doesn't really make sense. Is there an action create for articles in the permissions tab of articles?

[szepty-ziemi]

Nope:

[Elfangor93]

Its now required to add a fourth argument to the checkACL() method if you are checking for the action "add" on images. So the correct syntax for that is now:

$res = $acl->checkACL('add','image', <CATID>, true);

If the fourth argument is not provided in this example or is set to false, then an error will be thrown explaining you the issue with your command.

[szepty-ziemi]

Testing results for configuration set:

Global settings

• Create +
• Create in own (always false)
• Edit +
• Edit own (always false)
• Edit state +
• Delete +
• Delete own (always false)

Configuration settings

• Delete +
• Edit +
• Edit state +
• Edit own (always false)

I think that "Edit own" in configuration set is not needed, as there is no way to assign it to the user (add owner). Also that's why other "own" return false as the don't apply to configuration set.

For admin and manage we get error " Action not available for this asset. Access can not be checked. Please provide reasonable inputs." but that seems to be ok.

RESULT: OK

[szepty-ziemi]

Testing results for tag:

Global settings

• Create +
• Create in own (always false) <- seems to be ok as there is no own to be
• Edit +
• Edit own (always false)
• Edit state +
• Delete +
• Delete own (always false)

Tag settings

• Delete +
• Edit +
• Edit state +

"Edit own" and "Delete own" should be the case, as user can be assigned as tag owner (Created by). Yet it returns "false". Or am I wrong here? Tag permission do not have "Edit own".

For admin and manage we get error " Action not available for this asset. Access can not be checked. Please provide reasonable inputs." That seems to be ok.

RESULT: Pending (as for "own" permissions)

[szepty-ziemi]

Testing result for category:

Global settings

• Create +
• Create in own (always false)
• Edit +
• Edit own x
• Edit state +
• Delete +
• Delete own (always false)

"Edit own" and "Delete own" should be the case, as user can be assigned as category owner (Created by). Yet it returns "false" in component settings. Just like in tags.

I imagine scenario where admin sets certain user as category owner and allows that user to be able to create categories within that parent category (Create in own).

Category settings:

• Create (always false!)
• Create in own (always false)
• Delete (must be owner or within owned category, asset id specified) ?
• Delete own (must be owner or within owned category, asset id specified) ?
• Edit (must be owner or within owned category, asset id specified) ?
• Edit own (must be owner or within owned category, asset id specified) ?
• Edit state (must be owner or within owned category, asset id specified) ?

For "Create" it return false, even if user is category owner, when set in category permissions. I tested that with and without asset id parameter and all return false.

All those marked with question mark: it returns true when user is category owner, but also returns true when user is NOT category owner but category resides IN category owned by user. Is it correct?

What about:

• Upload
• Upload in own

I presume that add = Create... but what is equivalent of Upload? Shouldn't Upload and Create be the same as each upload should finish with image creation in the gallery.

For admin and manage we get error " Action not available for this asset. Access can not be checked. Please provide reasonable inputs." That seems to be ok.

RESULT: Pending (many unclear acceptance criteria)

[Elfangor93]

I think that "Edit own" in configuration set is not needed, as there is no way to assign it to the user (add owner).

Actually, there is. Its just not available in the GUI. But in principle the configuration sets are owned by a specific user. I think we should keep the possibility of the own permissions to be future proof.

[Elfangor93]

"Edit own" and "Delete own" should be the case, as user can be assigned as category owner (Created by). Yet it returns "false" in component settings. Just like in tags.

The "own" access checks should be fine now.

[Elfangor93]

I presume that add = Create... but what is equivalent of Upload? Shouldn't Upload and Create be the same as each upload should finish with image creation in the gallery.

Create means: Can I add child categories? Upload means: Can I add images within this category?

[Elfangor93]

For admin and manage we get error " Action not available for this asset. Access can not be checked. Please provide reasonable inputs." but that seems to be ok.

This is done by purpose. It is not possible to check for admin and manage on a specific asset. Admin and manage only exist on component level. That's why you need to check for that by doing

$res = $acl->checkACL('manage');

or

$res = $acl->checkACL('admin');

[Elfangor93]

@szepty-ziemi Would you mind checking again? There was a serious problem in checking for the ownership which influenced most of the checks in a bad way. This should be fixed now...

[szepty-ziemi]

Testing results for configuration set:

Global settings

• Create +
• Create in own (always false) + it make sense
• Edit +
• Edit own (if owner = true, else = false) +
• Edit state +
• Delete +
• Delete own (if owner = true, else = false) +

Configuration settings

• Delete +
• Edit +
• Edit state +
• Edit own (if owner = true, else = false) +

RESULT: OK

[szepty-ziemi]

Testing results for tag:

Global settings

• Create +
• Create in own (always false) + it make sense
• Edit +
• Edit own (if owner = true, else = false) +
• Edit state +
• Delete +
• Delete own (if owner = true, else = false) +

Tag settings

• Delete +
• Edit +
• Edit state +

RESULT: OK

[szepty-ziemi]

Testing result for category:

Global settings

• Create +
• Create in own (always false) -
• Edit +
• Edit own (always false) -
• Edit state +
• Delete +
• Delete own (always false) -

Category settings:

• Create +
• Create in own (if owner = true, else = false) +
• Delete +
• Delete own (if owner = true, else = false) +
• Edit (always false) -
• Edit own (always false) -
• Edit state +

RESULT: FAIL

Setting:

So for global setting "Create in own" (and others alike) I'm specifying id for category (3) that is owned by user Tester (698) but result is false.

And if asset settings is very similar but in addition "Edit" also returns false no matter if is specify category id, if category is owned by user or not.

[szepty-ziemi]

For admin and manage we get error " Action not available for this asset. Access can not be checked. Please provide reasonable inputs." but that seems to be ok.

This is done by purpose. It is not possible to check for admin and manage on a specific asset. Admin and manage only exist on component level. That's why you need to check for that by doing

$res = $acl->checkACL('manage');

or

$res = $acl->checkACL('admin');

This works perfectly.

RESULT: OK

[szepty-ziemi]

@Elfangor93 would you provide a fix for reported issues?

I didn't tested image permissions yet, as I'm not sure if changes you'll implement, will cause retest of all assets. Please, let me know.

[Elfangor93]

@szepty-ziemi The issues with the category checks should be solved now.

And you were right not to perform the tests for images permissions. They suffered from the same issue. Shouls all be solved now.

[Elfangor93]

Its now required to add a fourth argument to the checkACL() method if you are checking for the action "add" on categories or images. So the correct syntax for that is now:

$res = $acl->checkACL('add','image', <CATID>, true);

or

$res = $acl->checkACL('add','category', <PARENT-CATID>, true);

[szepty-ziemi]

Okay, I'm leaving tomorrow and will be available at Monday. I will continue testing when I'm back.

[Elfangor93]

Oke, tested this PR by myself in a coordinated and structured manner. Found some more issues and fixed them. This PR looks to me now ready to merge. Lets await @szepty-ziemi last tests and merge it afterwards.

Important! I had to change again the database structure to fix the issues. Therefore its needed to to a fresh install before testing again!

[Elfangor93]

Could anyone pleas give this PR a last check before I merge it?

[szepty-ziemi]

I will check it tomorrow. Just returned from holidays and couldn't manage today. Sorry for that.

[szepty-ziemi]

There is a warning:

It seems it shows when root category is called.

[szepty-ziemi]

Global settings

• Create +
• Create in own (if owner = true, else = false) +
• Edit +
• Edit own (if owner = true, else = false) +
• Edit state +
• Delete +
• Delete own (if owner = true, else = false) +

Category settings:

• Create +
• Create in own (if owner = true, else = false) +
• Delete +
• Delete own (if owner = true, else = false) +
• Edit (if owner = true, else = false) +
• Edit own (if owner = true, else = false) +
• Edit state +

RESULT: OK