[
https://issues.apache.org/jira/browse/OAK-5200?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15712462#comment-15712462
]
Manfred Baedke edited comment on OAK-5200 at 12/1/16 4:57 PM:
--------------------------------------------------------------
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.
See OAK-5209 for a discussion on a correct way to avoid the costly roundtrips.
was (Author: baedke):
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)
