Uploaded image for project: 'Jackrabbit Oak'
  1. Jackrabbit Oak
  2. OAK-5200

OAK-4930 introduced critical bug confusing id and principal name

Agile BoardAttach filesAttach ScreenshotVotersWatch issueWatchersCreate sub-taskLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4.11, 1.5.15, 1.6.0
    • Component/s: auth-external
    • Labels:
      None

      Description

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

        Attachments

        Issue Links

          Activity

            People

            • Assignee:
              baedke Manfred Baedke
              Reporter:
              angela Angela Schreiber

              Dates

              • Created:
                Updated:
                Resolved:

                Issue deployment