bug #8849
closedNPE in synonym DetailsViewer for synonyms without accepted taxon
100%
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
Updated by Katja Luther about 4 years ago
- Target version changed from Release 5.13 to Release 5.14
move to 5.14
Updated by Katja Luther about 4 years ago
- Target version changed from Release 5.14 to Release 5.15
Updated by Katja Luther about 4 years ago
- Target version changed from Release 5.15 to Release 5.18
Updated by Andreas Müller almost 4 years ago
- Status changed from New to Resolved
- % Done changed from 0 to 50
Applied in changeset taxeditor|cd5fdc98f90ec4d26348a785e4c70369144f28f0.
Updated by Andreas Müller almost 4 years 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?
Updated by Andreas Kohlbecker almost 4 years 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.
Updated by Andreas Müller almost 4 years 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?
Updated by Andreas Müller almost 4 years ago
- Copied to bug #9083: Improve authorization handling in synonym details view (and generally) added
Updated by Andreas Müller almost 4 years 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