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

Manfred Baedke edited comment on OAK-5200 at 12/1/16 1:56 PM:
--------------------------------------------------------------

[~anchela],

bq. it seems that you don't understand the difference between the ID of an 
external user/group and it's principal name

I understand the difference, but that doesn't keep me from making mistakes.

bq. 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.

That happened because the implementation used by oak-auth-ldap works perfectly 
fine with this, since the DN of an LDAP entry is used for both the id and the 
principal name, and it's the only implementation known to me. So no problems 
with integration tests or manual tests done by the customer ever occurred.

Please keep the issue descriptions factual. Thanks.

bq. 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

The customers who got this are using LDAP, so there is no need to act.


was (Author: baedke):
[~anchela],

bq. it seems that you don't understand the difference between the ID of an 
external user/group and it's principal name

I understand the difference, but that doesn't keep me from making mistakes.

bq. 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.

That happened because the implementation used by oak-auth-ldap works perfectly 
fine with this, since the DN of an LDAP entry is used for both the id and the 
principal name, and it's the only implementation known to me besides unit tests.

Please keep the issue descriptions factual. Thanks.

bq. 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

The customers who got this are using LDAP, so there is no need to act.

> 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