[ 
https://issues.apache.org/jira/browse/OAK-5200?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15712462#comment-15712462
 ] 

Manfred Baedke commented on OAK-5200:
-------------------------------------

Reverted the critical parts of the commits, see 
https://issues.apache.org/jira/browse/OAK-4930?focusedCommentId=15712262&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15712262.


> 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)

Reply via email to