JoomGalleryfriends / JG4-dev

Development repository for JoomGallery v4.x
GNU General Public License v3.0
10 stars 7 forks source link

Apply access service to backend views #215

Closed Elfangor93 closed 3 months ago

Elfangor93 commented 5 months ago

This PR applies the access service provided with PR #113 to the JoomGallery MVC backend classes. This means that in the backend the advanced own and inown permissions are working now.

Additionally I improved the code style and coding notes of the backend PHP files. And all easy to replace deprecations are replaced like Factory::getDbo; with Factory::getContainer()->get(DatabaseInterface::class);.

How to test this PR

Set error reporting mode to maximum in the Joomla global configuration.

All views and actions should work as before and should not throw any errors. Additionally check that users without super user rights can access and perform tasks according to the ACL settings set in the global component setting and in the corresponding form of the content type (image form if you want to create an image ...).

eumel1602 commented 5 months ago

J 5.0.3 / PHP 8.2 folgende Meldung taucht in verschiedenen Bereichen der JG in großer Anzahl (immer diese selbe) auf: Deprecated: Creation of dynamic property Joomgallery\Component\Joomgallery\Administrator\Service\Config\DefaultConfig::$jg_msg_zipdownload is deprecated in /var/www/vhosts/rrrrrrrrrrrrrrrrrrr.alfahosting-server.de/www.example.org/administrator/components/com_joomgallery/src/Extension/ServiceTrait.php on line 115

eumel1602 commented 5 months ago

Thumbnails im FE und BE und Original bilder werden bei mir nicht angezeigt:

thumbnal

thumbnal2

Elfangor93 commented 5 months ago

folgende Meldung taucht in verschiedenen Bereichen der JG in großer Anzahl (immer diese selbe) auf: Deprecated: Creation of dynamic property

Die Meldung wird mit PR #211 behoben sein.

Thumbnails im FE und BE und Original bilder werden bei mir nicht angezeigt

Bekommst du eine Fehlermeldung in der Browserkonsole im Tab unter "Netzwerkanalyse"?

eumel1602 commented 5 months ago

Bekommst du eine Fehlermeldung in der Browserkonsole im Tab unter "Netzwerkanalyse"?

Hab dir die wieder riesige fehlermeldung per Matermost txt datei geschickt.

eumel1602 commented 5 months ago

wenn ich Tags anlege und sie dann z.b. einem bild zuordne, werden sie nach dem speichern anscheind übernommen, ruft man das Bild aber erneut im BE auf, sieht man unter den TAG Einstellung nicht mehr das man da schon was zugeordnet hat. In der Gesamtübersicht der Tags kann man sehen das da was zugeordnet ist, anklickbar ist das aber nicht... 1

2

Elfangor93 commented 5 months ago

wenn ich Tags anlege und sie dann z.b. einem bild zuordne, werden sie nach dem speichern anscheind übernommen, ruft man das Bild aber erneut im BE auf, sieht man unter den TAG Einstellung nicht mehr das man da schon was zugeordnet hat.

Und das beobachtest du nur mit diesem PR? Ich habe hier eigentlich nichts an dem Tag-Feld geändert...

eumel1602 commented 5 months ago

Im Main brunch und im Fix migrations brunch funktioniert es! (Was überall nicht geht ist, das anklicken der anzahl zu einem Tag, um zu sehen welche Bilder mit dem jeweiligen Tag versehen ist

rowi68 commented 5 months ago

Im Main brunch und im Fix migrations brunch funktioniert es! (Was überall nicht geht ist, das anklicken der anzahl zu einem Tag, um zu sehen welche Bilder mit dem jeweiligen Tag versehen ist

Kann ich bestätigen... ( J 5.0.3 / PHP 8.2 )

Elfangor93 commented 5 months ago

Thanks @eumel1602 & @rowi68 for testing. The issue found by you should be fixed now.

rowi68 commented 5 months ago

:+1: Der zugeordnete Tag zum Bild ist bzw. bleibt jetzt sichtbar... Das Anklicken der Anzahl zu einem Tag, um zu sehen welche Bilder mit dem jeweiligen Tag versehen ist, funktioniert jedoch (noch) nicht.

Elfangor93 commented 5 months ago

Das Anklicken der Anzahl zu einem Tag, um zu sehen welche Bilder mit dem jeweiligen Tag versehen ist, funktioniert jedoch (noch) nicht.

Das hat vor diesem PR auch noch nicht funktioniert und ist nicht Teil dieses PRs. Diese Funktion müsste separat eingereicht werden. Sehe ich aktuell aber nicht als Priorität.

MrMusic commented 5 months ago

Did some tests: Created a new user group below Manager. Removed all permissions for this user group in a category. ACL-settings

The following functions are still possible for this category:

  1. Create (sub)categories
  2. Upload of images
  3. Deleting categories
  4. Editing categories
Elfangor93 commented 5 months ago

Created a new user group below Manager.

@MrMusic I wonder how you tested that since the Manager usergroup does not have access to the backend views of joomgallery. What are the permissions for this new usergroup in the JoomGallery global component parameters (option=com_config&view=component&component=com_joomgallery)?

MrMusic commented 5 months ago

Settings in Joomla configuration:

acl-joomla

Settings JoomGallery component:

ACL-joomgallery

Elfangor93 commented 5 months ago

@MrMusic Thanks for reporting this error. The acl should be correctly applied and checked now. I have chosen to set the minimal requirements for JG4 to Joomla 4.4.0 and PHP 8.0. Lets discuss this next thuesday...

MrMusic commented 5 months ago

Did a new test as SuperUser.

Error when change JoomGallery default configuration: 0 Joomgallery\Component\Joomgallery\Administrator\User\User::getAcl(): Return value must be of type Joomgallery\Component\Joomgallery\Administrator\User\AccessInterface, Joomgallery\Component\Joomgallery\Administrator\Service\Access\Access returned

1   ()  JROOT\administrator\components\com_joomgallery\src\User\User.php:51
2   Joomgallery\Component\Joomgallery\Administrator\User\User->getAcl()     JROOT\administrator\components\com_joomgallery\src\User\User.php:67
3   Joomgallery\Component\Joomgallery\Administrator\User\User->authorise()  JROOT\administrator\components\com_joomgallery\tmpl\config\edit.php:83

Error when create new configuration set: _0 Joomgallery\Component\Joomgallery\Administrator\Service\Access\Access::checkACL(): Argument #3 ($pk) must be of type int, null given, called in ...administrator\components\comjoomgallery\src\Controller\JoomFormController.php on line 160

1   ()  JROOT\administrator\components\com_joomgallery\src\Service\Access\Access.php:137
2   Joomgallery\Component\Joomgallery\Administrator\Service\Access\Access->checkACL()   JROOT\administrator\components\com_joomgallery\src\Controller\JoomFormController.php:160
3   Joomgallery\Component\Joomgallery\Administrator\Controller\JoomFormController->allowAdd()   JROOT\libraries\src\MVC\Controller\FormController.php:162 
Elfangor93 commented 5 months ago

@MrMusic your found errors are fixed. Thanks for testing.

Elfangor93 commented 4 months ago

@MrMusic can you try again?

MrMusic commented 4 months ago

Folgende Probleme sind mir aufgefallen:

Elfangor93 commented 4 months ago

Im Form-Field der Kategorie werden nur Kategorien mit 'Create' Berechtigung angezeigt, nicht die mit Upload Berechtigung.

Currently in the image form views a category dropdownfield (jgcategorydropdown) was used. But this field checks the creation permission of a user in the current category (core.create). Thats why all categories where you dont have permission to create subcategories are filtered out. Changing that is quite time consuming and can be done a later time if really needed. Me personally I like the category selection via popup box much more anyway and there all categories are always displayed.

Elfangor93 commented 4 months ago

Was einen Fehler wirft, ist das ausführen einer "verbotenen" Aktion: 0 Call to protected method Joomgallery\Component\Joomgallery\Administrator\Extension\JoomgalleryComponent::addLog() from scope Joomgallery\Component\Joomgallery\Administrator\Model\CategoryModel 1 () JROOT\administrator\components\com_joomgallery\src\Model\CategoryModel.php:271 2 Joomgallery\Component\Joomgallery\Administrator\Model\CategoryModel->delete() JROOT\libraries\src\MVC\Controller\AdminController.php:143

This error will be fixed with PR #219

MrMusic commented 4 months ago

New Tested with categories:

Elfangor93 commented 4 months ago

Edit Own = allowed: But editing of own categories is not possible.

Please provide me with your Category tree and all the parameter settings for the category in which the permission is not working. I can not reproduce your issue.

MrMusic commented 4 months ago

Please provide me with your Category tree and all the parameter settings for the category in which the permission is not working. I can not reproduce your issue.

Test Category is a top level Category Category Titel = "Edit Own = Allowed" Owner = "tester" -> Member of group "Tester" Permission: see screenshot ACL-category-edit-own

In Category Manager -> no link to edit ACL-category-edit-own-owner

Elfangor93 commented 4 months ago

Edit Own = allowed: But editing of own categories is not possible.

There was no problem with editing categories. Only the link to the edit from was not provided in the list view because of an issue in the checkACL() mezhod of the service. Should be fixed now...

Elfangor93 commented 4 months ago

Edit State = denied: But it is still possible to change the status in the category edit form and save.

Yes. This is the default behavior of Joomla. I have now added a check for core.edit.state for the category edit to try it out. The category edit is now not anymore compliant with the core!

What do you think, should we stitch here to the core behovior or should we extend the behavior to deny changing state in the edit view if core.edit.state is denied?

MrMusic commented 4 months ago

What do you think, should we stitch here to the core behovior or should we extend the behavior to deny changing state in the edit view if core.edit.state is denied?

In Joomla core the status field in the edit form is deactivated if the 'edit state' permission is missing. That's why I think it's fine the way it is now.

What does not work with images: If the permission 'edit own' = not allowed, no edit is possible even if the permission 'edit' = allowed. ACL-image-edit-allowed

Elfangor93 commented 4 months ago

In Joomla core the status field in the edit form is deactivated if the 'edit state' permission is missing. That's why I think it's fine the way it is now.

Its updated such that it behaves like com_content now

Elfangor93 commented 4 months ago

What does not work with images: If the permission 'edit own' = not allowed, no edit is possible even if the permission 'edit' = allowed.

@MrMusic Have you denied the locked permissions in the image by denying it in the category or by denying it in the image itself within a parent asset (e.g Manager)?

Is the user which does not have access, the owner of the image? Because I would expect different behavior in this situation for the owner and for everyone else...

Update comment: I am asking because it looks correct to me. If you are owner of the image you should not be able to edit the image. If you are not the owner you should be able to edit. Because the setting "Edit own" is of higher priority than "Edit". Its a combination of settings which does not make sense in practice I guess.

eumel1602 commented 4 months ago

ich habe diesen PR erfolgreich ✅ mit J 4.4.5 getestet. Ich habe in der ACL viele versuche gemacht und fast immer logische ergebnisse erhalten. alles beim testen abzudecken ist sicherlich sehr schwer. Ich denke die wichtigsten haben auf jeden Fall funktioniert. (loeschen, bearbeiten, upload. u.s.w.)

--

Was ist mir allgemein aufgefallen, welches in der alpha 3 funktionieren könnte/solte:

bildschirmabgriff

MrMusic commented 4 months ago

Its updated such that it behaves like com_content now

Thanks it looks good now.

@MrMusic Have you denied the locked permissions in the image by denying it in the category or by denying it in the image itself within a parent asset (e.g Manager)?

Edit is denied in the Category (level=1). In Image all 'New Setting' is 'Inherited'.

Is the user which does not have access, the owner of the image?

Yes, it is the owner. Otherwise access would be allowed.

I am asking because it looks correct to me. If you are owner of the image you should not be able to edit the image. If you are not the owner you should be able to edit. Because the setting "Edit own" is of higher priority than "Edit". Its a combination of settings which does not make sense in practice I guess.

This would be a different behaviour than in JoomGallery 3, where editing is allowed if 'Edit' is allowed, no matter what is defined for 'Edit own'. Here is a code snippet from the models/image.php line 274 of JG3: There the logic for the edit check looks like this:

if(!$this->_user->authorise('core.edit', $asset) && (!$this->_user->authorise('core.edit.own', $asset) || !$row->owner || $row->owner != $this->_user->get('id')))
{
  $this->setError(JText::_('COM_JOOMGALLERY_COMMON_MSG_NOT_ALLOWED_TO_EDIT_IMAGE'));

  return false;
}

In simple terms, editing is allowed if core.edit = allowed or core.edit.own = allowed and the current user is the owner. So an 'or' condition without consideration of a 'priority'.

Just tested with Joomla 4 categories: 'Edit' = allowed 'Edit own' = denied Nevertheless, editing is allowed for the owner of the category.

eumel1602 commented 4 months ago

Bild oder Kategorie: Wenn Löschen "Erlaubt" gestellt ist und DELET OWN "Verweigert" ist, kommt beim speichern folgende fehlermeldung:

0 Call to protected method Joomgallery\Component\Joomgallery\Administrator\Extension\JoomgalleryComponent::addLog() from scope Joomgallery\Component\Joomgallery\Administrator\Model\ImageModel

bildschirmabgriff2

Elfangor93 commented 4 months ago

Wenn Löschen "Erlaubt" gestellt ist und DELET OWN "Verweigert" ist, kommt beim speichern folgende fehlermeldung

Die fehlermeldung kommt, wenn du die Kategorie oder das Bild bearbeitet hast und speichern drückst?

Call to protected method Joomgallery\Component\Joomgallery\Administrator\Extension\JoomgalleryComponent::addLog()

Der PHP error wird mit PR #219 behoben sein. Interessant ist jedoch, was das Model versucht in das Log zu schreiben. Anscheinend geht das noch was anderes schief...

Elfangor93 commented 4 months ago

This would be a different behaviour than in JoomGallery 3

And whats better or what do we wanna have in JoomGallery 4? The way it is now or the way it was in JoomGallery 3? For me personally the way it is now is more intuitive...

eumel1602 commented 4 months ago

Die fehlermeldung kommt, wenn du die Kategorie oder das Bild bearbeitet hast und speichern drückst?

Sorry, die meldung kommt wenn ich das Bild loeschen will! und es wird auch nicht gelöscht...

Elfangor93 commented 4 months ago

@eumel1602 Wenn der Benutzer der das Bild löschen will, der Besitzer des Bildes ist, dann handelt es sich um das gleiche Fehlerbild wie bei @MrMusic .

Wir müssen uns da entscheiden, wie wir es in JG4 wollen.

eumel1602 commented 4 months ago

Ich persönlich denke, dass es so sein sollte wie in der JG 3! Einfach um eventuelle JG 3 Nutzer, die damit gearbeitet haben, nicht zu verwirren. (richtiger und logischer ist es vielleicht wie manuel es sagt...)

Elfangor93 commented 4 months ago

Eine Möglichkeit wäre auch mit dem Kompatibilitätsmodus zwischen den Varianten zu wechseln.

MrMusic commented 4 months ago

For me personally the way it is now is more intuitive...

Ja, vielleicht. Ich wollte nur darauf aufmerksam machen, dass die Berechtigungen nun unterschiedlich zu JoomGallery 3 und zu Joomla 4 sind. Wir können es gerne erstmal so belassen wie es jetzt ist und weiteres Feedback nach der Alpha3 abwarten.

Eine Möglichkeit wäre auch mit dem Kompatibilitätsmodus zwischen den Varianten zu wechseln.

Dies könnte zu großer Verwirrung bei den Anwendern führen. Aus meiner Sicht sind die Berechtigungen etwas sehr sensibles was man nicht einfach per Klick verändern sollte.

Mit dem testen bin ich jetzt fertig. Es haben sich keine weitere Probleme gezeigt. Dieser PR kann dann gemergt werden.

Elfangor93 commented 4 months ago

Dies könnte zu großer Verwirrung bei den Anwendern führen. Aus meiner Sicht sind die Berechtigungen etwas sehr sensibles was man nicht einfach per Klick verändern sollte.

Das stimmt. Die Berechtigungen sind zum einen etwas extrem nützliches und wichtiges, ist jedoch durch seine viele Möglichkteiten für Nutzer schwer zu verstehen und zu richtig anzuwenden. Deshalb macht es wahrscheinlich Sinn möglichst wenig zu verändern.

Verhält es sich in JoomGallery 3 denn immer so bei den "own" actions? Und wie verhält es sich denn bei Inherit / Explixit Allow / Explicit Deny?

Aktuell wird Inherit bei der Berechnung ignoriert, und bei den Expliziten argumenten wird Own priorisiert. Also own überschreibt die "normale" Berechtigung falls vorhanden.

MrMusic commented 4 months ago

Aktuell wird Inherit bei der Berechnung ignoriert, und bei den Expliziten argumenten wird Own priorisiert. Also own überschreibt die "normale" Berechtigung falls vorhanden.

Das verstehe ich nicht. Egal ob explizit oder inherited: Relevant ist doch nur was am Ende in 'Calculated Setting' als Ergebnis angezeigt wird. Gerade ausprobiert mit Kategorien erstellen: Wenn die übergeordnete Kategorie aufgrund der Einstellung 'Denied' als 'Calculated Setting' ein 'Not Allowed' hat kann keine Unterkategorie angelegt werden. Wenn dagegen die Einstellung 'Inherited' und 'Calculated Setting' ein 'Not Allowed (Inherited)' ergibt, kann trotzdem eine Unterkategorie erstellt werden. Denke hier muss noch etwas geändert werden...

Elfangor93 commented 3 months ago

@MrMusic Die own Berechtigungen sind jetzt dem core angepasst....

MrMusic commented 3 months ago

@MrMusic Die own Berechtigungen sind jetzt dem core angepasst....

Besten Dank! Bei ersten Tests sind mir keine Probleme aufgefallen. Einzig: Der Delete Button ist nur sichtbar wenn die Berechtigung 'Delete' = Erlaubt ist. Wenn nur 'Delete Own' = Erlaubt ist, erscheint der Button nicht. Betrifft Kategorien und Bilder.

Elfangor93 commented 3 months ago

Bitte ein neuer Issue eröffnen dafür.