Project

General

Profile

Actions

feature request #7148

closed

feature request #6867: explicitely assign and revoke UPDATE & DELETE permission per enitity in the registration workflow

GrantedAuthorityRevokingRegistrationUpdateLister: delete orphan references to GrantedAuthorityImpl in User and Group

Added by Andreas Kohlbecker about 6 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Priority14
Category:
cdm-vaadin
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
Severity:
major

Description

The GrantedAuthorityRevokingRegistrationUpdateLister deletes GrantedAuthorityImpl entities but the references to GrantedAuthorityImpl in User and Group are not deleted with the entities.

Update of the description with documentation from the GrantedAuthorityRevokingRegistrationUpdateListe.class:

This Hibernate PostUpdateEventListener is responsible for revoking GrantedAuthorities from any user which is having per entity permissions in the object graph of the Registrationbeing updated This encompasses GrantedAuthotities with the CRUD values CRUD.UPDATE, CRUD.DELETE. Please refer to the method documentation of collectDeleteCandidates(Registration) for further details.

The according permissions are revoked when the RegistrationStatus is being changed by a database update. The RegistrationStatus causing this are contained in the constant MODIFICATION_STOP_STATES which are

  • RegistrationStatus.PUBLISHED,
  • RegistrationStatus.READY,
  • RegistrationStatus.REJECTED

Related issues

Related to EDIT - bug #7531: PermissionDeniedException on flushing registration with modified status even if the user has the required authorityClosedAndreas Kohlbecker

Actions
Related to EDIT - bug #7147: GrantedAuthorityRevokingDeleteListener implementedNewAndreas Müller

Actions
Copied to EDIT - task #7274: GrantedAuthorityRevokingRegistrationUpdateLister: add the exception handling for failing flush or commitClosedAndreas Kohlbecker

Actions
Actions #1

Updated by Andreas Kohlbecker about 6 years ago

Hallo Andreas und Katja,

ich versuche per hql GrantedAuthorityImpls und die zugehörigen Relationen von User und Group auf die zu löschenden Entities zu löschen.
Hier ist meine Methode

    /**
     * @param deleteCandidates
     */
    private void deleteAuthorities(EventSource session, Set<CdmAuthority> deleteCandidates) {

        Collection<String> authorityStrings = new ArrayList<String>(deleteCandidates.size());
        deleteCandidates.forEach( dc -> authorityStrings.add(dc.toString()));

        String hql3 = "update Group g set g.grantedAuthorities = null where g.grantedAuthorities in (select ga from GrantedAuthorityImpl ga where ga.authority in (:authorities))";
        Query query3 = session.createQuery(hql3);
        query3.setParameterList("authorities", authorityStrings);
        query3.setFlushMode(FlushMode.MANUAL); // workaround for  HHH-11822 (https://hibernate.atlassian.net/browse/HHH-11822)
        query3.executeUpdate();

        String hql2 = "update User u set u.grantedAuthorities = null where u.grantedAuthorities in (select ga from GrantedAuthorityImpl ga where ga.authority in (:authorities))";
        Query query2 = session.createQuery(hql2);
        query2.setParameterList("authorities", authorityStrings);
        query2.setFlushMode(FlushMode.MANUAL); // workaround for  HHH-11822 (https://hibernate.atlassian.net/browse/HHH-11822)
        query2.executeUpdate();

        String hql1 = "delete from GrantedAuthorityImpl as ga where ga.authority in (:authorities)";
        Query query1 = session.createQuery(hql1);
        query1.setParameterList("authorities", authorityStrings);
        query1.setFlushMode(FlushMode.MANUAL); // workaround for  HHH-11822 (https://hibernate.atlassian.net/browse/HHH-11822)
        query1.executeUpdate();
    }

Abgesehen davon dass schon die erste hql query probleme bei der Übersetzung nach MySQL macht:

Caused by: com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: You have an error in your SQL syntax; 
check the manual that corresponds to your MySQL server version for the right syntax to use near 
'set .=null where . in (select grantedaut5_.id from GrantedAuthorityImpl granteda' at line 1

habe ich ohnehin Zweifel, ob die Delete-Query für die Foreign keys übehaupt funktionieren, da Group.grantedAuthorities und User.grantedAuthorities nicht null sein dürfen.
Allerdings habe ich bisher auch keine andere Möglichkeit gefunden. Eine ältere Syntax wird von Hibernate 5 leider nicht mehr unterstützt:

Delete g.grantedAuthorities from Group g where ....

Viele Grüße
Andreas

Actions #2

Updated by Andreas Kohlbecker about 6 years ago

Hallo Andreas,

ist das Problem noch aktuell?
Warum machst Du das delete über hql queries und nicht über die persistence Methoden?

Wieso setzt Du denn grantedAuthorities auf null, Du willst Doch nur die eine GrantedAuthority löschen bzw alle die, die in der Liste enthalten sind, oder?

Weil wenn grantedAuthorieties nicht null sein darf, dann sollte man ja die Gruppe auch löschen, wenn es keine grantedAuthorities mehr gibt, oder bevor die letzte gelöscht wird, noch einmal nachfragen, ob die Gruppe auch gelöscht werden soll.

Viele Grüße,
Katja

Actions #3

Updated by Andreas Kohlbecker about 6 years ago

Hallo Katja,

das Problem ist noch aktuell.

Warum machst Du das delete über hql queries und nicht über die persistence Methoden?

Diese Methode steckt in einem HibernateEventListener der auf PostUpdateEvents reagiert: eu.etaxonomy.cdm.persistence.hibernate.GrantedAuthorityRevokingRegistrationUpdateLister in cdm-vaadin
Das heißt deleteAuthorities() wird in einer Situation aufgerufen in der die Liste der zu prozessierenden Hibernate-Actions schon komplett ist, daher dachte ich, dass es vermutlich besser ist nicht die Persistence-Methoden zu verwenden, sondern die GrantedAuthorities die per HQL zu löschen. Es handelt sich bei diesen GrantedAuthorities um, per-entity permissions, also solche die direkt an User vergeben worden sind. Wenn lediglich ich die GrantedAuthorities lösche, bleiben die Einträge in der m-n-Tabelle UserAccount_GrantedAuthorityImpl erhalten. Das macht Hibernate leider nicht automatisch. Sofern das über das Cascading oder die ToMany Annotation gelöst werden kann wäre das auch eine prima Lösung.

Aber vielleicht sollte ich es doch einfach mit den persistence Methoden probieren, eventuell verursacht das gar keine Probleme wenn man diese innerhalb eines HibernateEventListeners aufruft?

Wieso setzt Du denn grantedAuthorities auf null, Du willst Doch nur die eine GrantedAuthority löschen bzw alle die, die in der Liste enthalten sind, oder?

Um die Einträge in der UserAccount_GrantedAuthorityImpl tabelle zu löschen, die übrig bleiben würden nach dem die GrantedAuthorities gelöscht sind.

Viele Grüße
Andreas

Actions #4

Updated by Andreas Kohlbecker about 6 years ago

Hallo Andreas,

wir hatten vor Weihnachten da ja schon mal drüber gesprochen. Hier noch ein paar Gedanken.

Dass das null setzen nicht hinhaut, sagst du ja selber. V.a. weil ja nur eine/einige, aber nicht alle granted authorities des Users/der Gruppe gelöscht werden sollen.

Ich habe auch noch mal selber recherchiert und keinen simplen Weg über HQL gefunden, wie man das machen kann

http://www.codejava.net/frameworks/hibernate/hibernate-basics-3-ways-to-delete-an-entity-from-the-datastore war ganz interessant. Hibernate scheint sich um FKs wirklich nicht zu scheren.
Mir sieht es so aus als ob es in diesem Fall wirklich am besten ist, die User/Group Instanz zu laden und die authorities per Hand zu entfernen und dann erst die Authority zu löschen.

Aber ich müsste das auch manuell ausprobieren um sicher zu sein. Katja hat da vielleicht noch mehr Erfahrung mit.

Viele Grüße,
Andreas M.

Actions #5

Updated by Andreas Kohlbecker about 6 years ago

  • Priority changed from New to Highest
  • Severity changed from normal to critical
Actions #6

Updated by Andreas Kohlbecker about 6 years ago

  • Priority changed from Highest to Priority14
  • Severity changed from critical to major
Actions #7

Updated by Andreas Kohlbecker about 6 years ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 30

ich habe eine Lösung gefunden. Es funkioniert die User Instanzen zu laden um dann die assoziierten GrantedAuthorities zu löschen. Allerdings muss man das in einer neuen, also separazen Session tun, ansonsten bekommt man ConcurrentModificationExeptions!

hier der code für User:

// -----------------------------------------------------------------------------------------
// this needs to be executed in a separate session to avoid concurrent modification problems
Session newSession = session.getSessionFactory().openSession();
Transaction txState = newSession.beginTransaction();
Query userQuery = newSession.createQuery("select u from User u join u.grantedAuthorities ga where ga.authority in (:authorities)");
userQuery.setParameterList("authorities", authorityStrings);
List<User> users = userQuery.list();
for(User user : users){
    List<GrantedAuthority> deleteFromUser = user.getGrantedAuthorities().stream().filter(
                ga -> authorityStrings.contains(ga.getAuthority())
            )
            .collect(Collectors.toList());
    user.getGrantedAuthorities().removeAll(deleteFromUser);
}
newSession.flush();
txState.commit();
newSession.close();
// -----------------------------------------------------------------------------------------

mit den Groups geht das äquivalent

Actions #8

Updated by Andreas Kohlbecker about 6 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 30 to 50
Actions #9

Updated by Andreas Kohlbecker about 6 years ago

  • % Done changed from 50 to 30

do final review after phycobank playday, by setting all newly created Registrations to the state PUBLISHED and check if all per emtity permissions are gone.

Actions #10

Updated by Andreas Müller about 6 years ago

Andreas Kohlbecker wrote:

ich habe eine Lösung gefunden. Es funkioniert die User Instanzen zu laden um dann die assoziierten GrantedAuthorities zu löschen. Allerdings muss man das in einer neuen, also separazen Session tun, ansonsten bekommt man ConcurrentModificationExeptions!

In welcher Reihenfolge löscht du denn? Wenn du erst die Relationen löscht und dann die GrantedAuthority selber, sollte das eigentlich in der gleichen Session gehen, oder hast du das ausprobiert? Ist auch sauberer.

Actions #11

Updated by Andreas Kohlbecker about 6 years ago

Andreas Müller wrote:

Andreas Kohlbecker wrote:

ich habe eine Lösung gefunden. Es funkioniert die User Instanzen zu laden um dann die assoziierten GrantedAuthorities zu löschen. Allerdings muss man das in einer neuen, also separazen Session tun, ansonsten bekommt man ConcurrentModificationExeptions!

In welcher Reihenfolge löscht du denn? Wenn du erst die Relationen löscht und dann die GrantedAuthority selber, sollte das eigentlich in der gleichen Session gehen, oder hast du das ausprobiert? Ist auch sauberer.

Nein das geht eben nicht. Zuerst hatte ich das auch so versucht und dann Concurrent-Modification-Probleme bekommen, daher auch der Kommentar im Kode:

// this needs to be executed in a separate session to avoid concurrent modification problems
Actions #12

Updated by Andreas Müller about 6 years ago

  • Assignee changed from Andreas Kohlbecker to Katja Luther

Ja, das hatte ich gesehen, war mir aber nicht sicher, auf welche Stelle des Codes sich das bezog.

Katja, kannst du da mal drauf schauen? Wie löschen wir denn sonst Objekte aus einer Collection, wenn das Objekt selber auch gelöscht werden soll. Das müsste doch eigentlich ein Standard sein.

Actions #13

Updated by Andreas Müller about 6 years ago

... immer 2 Sessions zu öffnen, kann da ja eigentlich nicht die Standardlösung sein und klingt auch transaktional unsauber.

Actions #14

Updated by Katja Luther about 6 years ago

Andreas Müller wrote:

Ja, das hatte ich gesehen, war mir aber nicht sicher, auf welche Stelle des Codes sich das bezog.

Katja, kannst du da mal drauf schauen? Wie löschen wir denn sonst Objekte aus einer Collection, wenn das Objekt selber auch gelöscht werden soll. Das müsste doch eigentlich ein Standard sein.

in anderen Fällen entfernen wir das Objekt aus der Collection und rufen danach für das entfernte Objekt delete auf (siehe z.B. TaxonNodeDao, wenn auch die Kinder gelöscht werden sollen)

for (TaxonNode node:nodes) {

                node = HibernateProxyHelper.deproxy(node, TaxonNode.class);

                if (node.equals(persistentObject)){
                    if (node.hasChildNodes()){
                        Iterator<TaxonNode> childNodes = node.getChildNodes().iterator();
                        TaxonNode childNode;
                        List<TaxonNode> listForDeletion = new ArrayList<TaxonNode>();
                        while (childNodes.hasNext()){
                            childNode = childNodes.next();
                            listForDeletion.add(childNode);
                            childNodes.remove();

                        }
                        for (TaxonNode deleteNode:listForDeletion){
                            delete(deleteNode, deleteChildren);
                        }
                    }

                    taxon.removeTaxonNode(node, deleteChildren);
                    taxonDao.saveOrUpdate(taxon);
                    taxon = HibernateProxyHelper.deproxy(taxonDao.findByUuid(taxon.getUuid()), Taxon.class);
                    taxonDao.delete(taxon);

                }
            }
Actions #15

Updated by Katja Luther about 6 years ago

Andreas Kohlbecker wrote:

Andreas Müller wrote:

Andreas Kohlbecker wrote:

ich habe eine Lösung gefunden. Es funkioniert die User Instanzen zu laden um dann die assoziierten GrantedAuthorities zu löschen. Allerdings muss man das in einer neuen, also separazen Session tun, ansonsten bekommt man ConcurrentModificationExeptions!

In welcher Reihenfolge löscht du denn? Wenn du erst die Relationen löscht und dann die GrantedAuthority selber, sollte das eigentlich in der gleichen Session gehen, oder hast du das ausprobiert? Ist auch sauberer.

Nein das geht eben nicht. Zuerst hatte ich das auch so versucht und dann Concurrent-Modification-Probleme bekommen, daher auch der Kommentar im Kode:

// this needs to be executed in a separate session to avoid concurrent modification problems

Diese concurrent modification Probleme bekommt man durch das Verändern der Collection und die muss man umgehen, indem man zweimal eine Schleife durchläuft, erst einmal alle zu löschenden Objekte in eine neue Liste und aus der Collection entfernen und danach erst löschen.

Actions #16

Updated by Andreas Müller about 6 years ago

Katja Luther wrote:

Andreas Kohlbecker wrote:

Diese concurrent modification Probleme bekommt man durch das Verändern der Collection und die muss man umgehen, indem man zweimal eine Schleife durchläuft, erst einmal alle zu löschenden Objekte in eine neue Liste und aus der Collection entfernen und danach erst löschen.

Why don't you use an iterator. With iterator.remove() you don't get concurrent modification. See Katjas code above.

Actions #17

Updated by Andreas Kohlbecker about 6 years ago

Andreas Müller wrote:

Katja Luther wrote:

Andreas Kohlbecker wrote:

Diese concurrent modification Probleme bekommt man durch das Verändern der Collection und die muss man umgehen, indem man zweimal eine Schleife durchläuft, erst einmal alle zu löschenden Objekte in eine neue Liste und aus der Collection entfernen und danach erst löschen.

Genau das mach ich doch in meinem code!!!

Die concurrent modification Probleme treten innerhalb von hibernate auf. An der Stelle wo sich der PostUpdateEventListener einklinkt befindet hibernate sich in Mitten der Bearbeitung der ActionQueue also in org.hibernate.engine.spi.ActionQueue.executeActions() irgendo hier drin tritt die concurrent modification auf, das lässt sich meines Wissens nur mit einer separaten Session umgehen.

An einer separaten Session kann ich übrigens nichts schlechtes erkennen, denn beide Sessions sind transaktional sauber getrennt. Es fehlt eventuell in meinem Code noch die Fehlerbehandlung für den Fall dass das der flush oder commit der zweiten Session fehl schlägt.

Actions #18

Updated by Katja Luther about 6 years ago

  • Status changed from Resolved to Feedback
  • Assignee changed from Katja Luther to Andreas Kohlbecker

do you want to add the exception handling for failing flush or commit?

Actions #19

Updated by Andreas Kohlbecker about 6 years ago

  • Copied to task #7274: GrantedAuthorityRevokingRegistrationUpdateLister: add the exception handling for failing flush or commit added
Actions #20

Updated by Andreas Kohlbecker about 6 years ago

  • Category changed from cdm-vaadin to cdmlib
  • Status changed from Feedback to Closed
  • % Done changed from 30 to 100

remaining task copied to new issue

Actions #21

Updated by Andreas Kohlbecker over 5 years ago

  • Category changed from cdmlib to cdm-vaadin
Actions #22

Updated by Andreas Kohlbecker over 5 years ago

  • Related to bug #7531: PermissionDeniedException on flushing registration with modified status even if the user has the required authority added
Actions #23

Updated by Andreas Kohlbecker over 5 years ago

  • Related to bug #7147: GrantedAuthorityRevokingDeleteListener implemented added
Actions #24

Updated by Andreas Kohlbecker over 5 years ago

  • Description updated (diff)
Actions

Also available in: Atom PDF