[jira] [Issue Comment Deleted] (OAK-5200) OAK-4930 introduced critical bug confusing id and principal name

["angela (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/OAK-5200?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

angela updated OAK-5200:
------------------------
    Comment: was deleted

(was: 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)