Project

General

Profile

bug #8849

NPE in synonym DetailsViewer for synonyms without accepted taxon

Added by Katja Luther 8 months ago. Updated 4 months ago.

Status:
Closed
Priority:
Highest
Category:
taxeditor
Target version:
Start date:
02/18/2020
Due date:
% Done:

100%

Severity:
normal
Found in Version:

Description

if a synonym has no accepted taxon an exception appears.

in DetailsViewerE4.createTaxonSections():

        Taxon acceptedTaxon = null;
        if (taxonBase instanceof Taxon){
            acceptedTaxon = (Taxon)taxonBase;
        }else{
           acceptedTaxon = ((Synonym)taxonBase).getAcceptedTaxon();
        }
        Set<TaxonNode> nodes = acceptedTaxon.getTaxonNodes();
        // check for subtree permissions as well.
        if (hasPermission){
            for (TaxonNode node: nodes){
                hasPermission &= CdmStore.currentAuthentiationHasPermission(node, requiredCrud);
                if (!hasPermission){
                    //check whether there are explicit TaxonNode rights
                    boolean taxonnodePermissionExists = false;
                    Collection<? extends GrantedAuthority> authorities = CdmStore.getCurrentAuthentiation().getAuthorities();
                    for (GrantedAuthority grantedAuthority: authorities){
                        if (grantedAuthority.getAuthority().startsWith("TAXONNODE")){
                            taxonnodePermissionExists = true;
                        }
                    }
                    if (!taxonnodePermissionExists){
                        hasPermission = true;
                    }
                }
            }
        }

Das sollte abgefangen werden. Allerdings ist die Frage, wie wir dann mit der Permission umgehen (nur für diese wird ja das akzeptierte Taxon benötigt.
Da es sich um einen sehr großen Ausnahmefall handelt, der aber vorkommen kann (auch im Taxon Bulkeditor kann ich Synonyme ohne akzeptiertes Taxon erzeugen), fände ich es nicht schlimm dort einfach zu sagen, wenn ein reines Synonym ohne acc vorliegt, ist der Subtree Filter unrelevant und man bekommt die Permission.
Aber vielleicht habt ihr ja eine bessere Idee. Ich hab da gerade nicht die Zeit drüber nachzudenken.


Related issues

Copied to Edit - bug #9083: Improve authorization handling in synonym details view (and generally) New 06/19/2020

Associated revisions

Revision cd5fdc98 (diff)
Added by Andreas Müller 5 months ago

fix #8849 fix NPE for synonyms without accepted taxon in details view and some code cleaning

History

#1 Updated by Katja Luther 8 months ago

  • Description updated (diff)

#2 Updated by Katja Luther 8 months ago

  • Target version changed from Release 5.13 to Release 5.14

move to 5.14

#3 Updated by Katja Luther 7 months ago

  • Target version changed from Release 5.14 to Release 5.15

#4 Updated by Katja Luther 6 months ago

  • Target version changed from Release 5.15 to Release 5.18

#5 Updated by Andreas Müller 5 months ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 50

#6 Updated by Andreas Müller 5 months ago

  • Assignee changed from Katja Luther to Andreas Kohlbecker
  • Priority changed from New to Highest
  • Target version changed from Release 5.18 to Release 5.15

I solved the NPE now in a way that synonyms without accepted taxon are not checked for subtree permission (as they simply are not part of any subtree).

Andreas K., can you please check if this is correct in terms of permission concepts and then pass the ticket to KL for reviewing the TaxEditor code?

#7 Updated by Andreas Kohlbecker 4 months ago

  • Status changed from Resolved to Feedback
  • Assignee changed from Andreas Kohlbecker to Andreas Müller

Generally for permission checks the class CdmUserHelper should be used which provides a couple of methods like userHasPermission(CdmBase entity, Object... args). String comparison is not always save as you may miss the rules implemented into by specific voters in CdmPermissionVoter.furtherVotingDescisions(CdmAuthority CdmAuthority, TargetEntityStates targetEntityStates, Collection<ConfigAttribute> attributes, ValidationResult validationResult).

Instead of using the string TAXONNODE it would be better to use PermissionClass.TAXONNODE instead.

Now to the logic. I think it is correct to only do the second step check for TaxonNode permissions if there is an accepted taxon with TaxonNodes at all. Otherwise the operation should be granted.

Apart from the above critics, code looks good but should be put into the TaxonBaseVoter as furtherVotingDescisions() implementation. As this can be breaking things we should do this only after the release. It should not be forgotten though as it is crucial to have all permission deccission logic at a central place.

One last comment on the CdmStore.currentAuthentiationHasPermission(...) methods: These should also use the CdmUserHelper internally or should be replaced by calls to the according CdmUserHelper methods.

#8 Updated by Andreas Müller 4 months ago

Good points.

I suggest to split the ticket. The NPE is fixed so this ticket can be closed.
The improved handling for authorization should be moved to a new ticket with high priority in 5.16.

Ok?

#9 Updated by Andreas Müller 4 months ago

  • Copied to bug #9083: Improve authorization handling in synonym details view (and generally) added

#10 Updated by Andreas Müller 4 months ago

  • Status changed from Feedback to Closed
  • % Done changed from 50 to 100

Andreas Müller wrote:

Good points.

I suggest to split the ticket. The NPE is fixed so this ticket can be closed.
The improved handling for authorization should be moved to a new ticket with high priority in 5.16.

Ok?

As nobody voted against splitting I do this. New ticket is #8903

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 40 MB)