jackrabbit-oak-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Manfred Baedke (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (OAK-5200) OAK-4930 introduced critical bug confusing id and principal name
Date Thu, 01 Dec 2016 16:55:58 GMT

    [ 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

View raw message