[ https://issues.apache.org/jira/browse/OAK-5200?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15709747#comment-15709747 ]
angela commented on OAK-5200: ----------------------------- Due to the fact that OAK-4930 introduced new public API (increasing exported version), I am not sure what would be the best way to deal with the new {{ExternalGroupRef}} (see also OAK-5198 and OAK-5199). > OAK-4930 introduced critical bug confusing id and principal name > ---------------------------------------------------------------- > > Key: OAK-5200 > URL: https://issues.apache.org/jira/browse/OAK-5200 > Project: Jackrabbit Oak > Issue Type: Bug > Components: auth-external > Reporter: angela > Assignee: Manfred Baedke > Priority: Blocker > Fix For: 1.6, 1.5.15, 1.4.11 > > > [~baedke], i rechecked your changes introduce with OAK-4930 again and spotted > a really severe bug. what you felt was a an improvement (but reported is a > bug) is simply not correct, because it seems that you don't understand the > difference between the ID of an external user/group and it's principal name. > here is what my original code did: > {code} > ExternalIdentity extId = idp.getIdentity(ref); > if (extId instanceof ExternalGroup) { > principalNames.add(extId.getPrincipalName()); > [...] > {code} > the reason why I had to retrieve the {{ExternalIdentity}} was no only because > of the recursive collection but *mainly* because I need to have access to the > principal name, which may be different from the ID. > so, what you considered to be an improvement by doing the following, is > actually introducing a critical bug. please revert your changes asap > including any backports you did right away, before testing|documenting or > even asking for a review from a second person, who might have spotted your > mistake. > {code} > for (ExternalIdentityRef ref : declaredGroupIdRefs) { > if (ref instanceof ExternalGroupRef && depth < 2) { > principalNames.add(ref.getId()); > } else { > ExternalIdentity extId = idp.getIdentity(ref); > if (extId instanceof ExternalGroup) { > principalNames.add(ref.getId()); > {code} > I really can't understand, how you could change that without noticing that > you now add the ID to the principal names instead of the {{principalName}}. > If you feel that there was some room for performance improvements, we can > discuss how we can get the principal name without forcing an extra > roundtrip... but the solution you committed is definitely wrong and must be > immediately reverted... please make sure you get in touch with the customers > that got your backported features... they may now have a messed up permission > setup in case they used your bug. -- This message was sent by Atlassian JIRA (v6.3.4#6332)