[OAK-5200] OAK-4930 introduced critical bug confusing id and principal name

• Type: Bug
• Status: Closed
• Priority: Blocker
• Resolution: Fixed
• Fix Version/s: 1.4.11, 1.5.15, 1.6.0
• Component/s: auth-external
• Assignee: Manfred Baedke
• Reporter: Angela Schreiber
• Created: 30/Nov/16 20:10
• Updated: 12/Dec/16 14:36
• Resolved: 01/Dec/16 16:57

Description

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:

ExternalIdentity extId = idp.getIdentity(ref);
if (extId instanceof ExternalGroup) {
    principalNames.add(extId.getPrincipalName());
     [...]

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.

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

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.

Issue Links

• is broken by: OAK-4930 External Principal Management: DynamicSyncContext makes redundant calls to IdentityProvider.getIdentity()
• is related to: OAK-5210 Ability to resolve principal name from ExternalIdentityRef without IDP roundtrip
• is related to: OAK-5209 Save roundtrips to external identity servers
• relates to: OAK-5198 Javadoc and Documentation of ExternalGroupRef
• relates to: OAK-5199 Test coverage for ExternalGroupRef