Kuali Rice Development
  1. Kuali Rice Development
  2. KULRICE-2996

Allow for kim principal names to be case insensitive without requring principal name to be stored all lowercase in KRIM_PRNCPL_T

    Details

    • Type: Bug Fix Bug Fix
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Not version specific
    • Component/s: Development
    • Labels:
      None
    • Similar issues:
      KULRICE-7060KIM Person Maintenance screen allows a new user to be created with mixed-case or uppercase letters in Principal Name field
      KULRICE-4533KIM allows usernames to be entered with mixed case, but the IdentityService searches in lowercase so user can't login
      KULRICE-4283IdentityServiceImpl methods convert principal name to lower case before going to the persistence layer but Person maintenance allows upper case principal names.
      KULRICE-6687Person lookup isn't case-insensitive in Rice 2.0
      KULRICE-4213Allow principal names in Kim screens instead of only principal Ids
      KULRICE-7566Should consider if a principal should be allowed to have a null principal name
      KULRICE-4229Principal Id/Name discrepancy on System notifications
      KULRICE-4248KimGroupDao assumes Principals in local database
      KULRICE-6817Document search by document type doesn't allow wildcards or case insensitive searches
      KULRICE-4019allow header block to show initiator person name instead of principal name
    • Rice Module:
      KIM

      Description

      The current implementation of case insensitivity in principal names in KIM requires that the PRNCPL_NM column in KRIM_PRNCPL_T be stored in all lowercase (see linked issue for original work surrounding this). This is implemented by a toLowerCase inside of the IdentityServiceImpl.getPrincipalByPrincipalName

      Instead we should implement case insenstivity on principal names without requiring the data to be stored lowercase. I think the only effective way to do this in SQL is to do an UPPER on both ends of the query, for example;

      SELECT * FROM KRIM_PRNCPL_T WHERE UPPER=UPPER(PRNCPL_NM)

      Of course, there are some potential inefficiencies here related to indexing on the PRNCPL_NM column. There are a few ways we could attempt to address this:

      1) If someone is implementing on Oracle they can put a function-based index on this column (not sure if mysql supports that or not)
      2) We could add configuration parameters that allow implementers to specify whether KIM principal names are stored in all upper or all lowercase.

      Of course, if they are going to be overridding the identity service and not storing data in these tables, it doesn't really matter. But that's going to depend on how a particular institution chooses to implement (and I could definitely see certain implementors choosing to store data in this table).

        Issue Links

          Activity

          Eric Westfall made changes -
          Field Original Value New Value
          Link This issue relates to KFSMI-1740 [ KFSMI-1740 ]
          Hide
          Eric Westfall added a comment -

          Added jonathan as a watcher in case he has any thoughts on the performance aspects of this.

          Show
          Eric Westfall added a comment - Added jonathan as a watcher in case he has any thoughts on the performance aspects of this.
          Hide
          Jonathan Keller added a comment -

          From the email I just sent:

          That field work has always been somewhat of a hack. But, here's the problem with principal lookups being case insensitive:

          If an institution does use mixed case principals and allows them to vary only by case, then we could end up retrieving multiple rows if we perform a truly case-insensitive search on that field. Now, in general, I don't think this would be the case. But, we would need to define the behavior when someone enters a principal name into a field on a document and there are multiple matching active records in the database, none of which may match the capitalization used by the user.

          The other problem is that a completely case insensitive search ( UPPER( prncpl_nm ) = UPPER( 'khuntley' ) ) will prevent the database from using the index on that column. Oracle has function-based indexes but I don't know that MySQL does. Given how much the getPrincipalByPrincipalName() methods are used within the system, we need to make sure that a database index is being used so we are not performing a full table scan every time.

          Show
          Jonathan Keller added a comment - From the email I just sent: That field work has always been somewhat of a hack. But, here's the problem with principal lookups being case insensitive: If an institution does use mixed case principals and allows them to vary only by case, then we could end up retrieving multiple rows if we perform a truly case-insensitive search on that field. Now, in general, I don't think this would be the case. But, we would need to define the behavior when someone enters a principal name into a field on a document and there are multiple matching active records in the database, none of which may match the capitalization used by the user. The other problem is that a completely case insensitive search ( UPPER( prncpl_nm ) = UPPER( 'khuntley' ) ) will prevent the database from using the index on that column. Oracle has function-based indexes but I don't know that MySQL does. Given how much the getPrincipalByPrincipalName() methods are used within the system, we need to make sure that a database index is being used so we are not performing a full table scan every time.
          Hide
          Jonathan Keller added a comment -

          Now, on that last item. If, as you say, we expect that most implementors will be overriding this service, then don't we impact a smaller number of implementors if we require that field to be in lower case?

          If we have to allow it to be in either all upper OR lower case, then I would support a parameter so that we can sidestep the performance issues. I would think that, at least, we can require an implementor to be consistent with their principal names.

          Show
          Jonathan Keller added a comment - Now, on that last item. If, as you say, we expect that most implementors will be overriding this service, then don't we impact a smaller number of implementors if we require that field to be in lower case? If we have to allow it to be in either all upper OR lower case, then I would support a parameter so that we can sidestep the performance issues. I would think that, at least, we can require an implementor to be consistent with their principal names.
          Hide
          Jeremy Hanson added a comment -

          Resolved based on email from Eric regarding making this change and indexing problems:


          I think that puts us right back where we started since all lower vs. all upper is kind of arbitrary. I think if we want to go this route we can just boil this down to proper documentation:

          1) Document that all data in PRNCPL_NM column of KRIM_PRNCPL_T is required to be all lowercase
          2) Document that, if this does not work for your implementation, you can override the identity service getPrincipalByPrincpalName method (is there anywhere else that I should be aware of?)

          That has the plus side of not requiring any changes to the current code

          Show
          Jeremy Hanson added a comment - Resolved based on email from Eric regarding making this change and indexing problems: – I think that puts us right back where we started since all lower vs. all upper is kind of arbitrary. I think if we want to go this route we can just boil this down to proper documentation: 1) Document that all data in PRNCPL_NM column of KRIM_PRNCPL_T is required to be all lowercase 2) Document that, if this does not work for your implementation, you can override the identity service getPrincipalByPrincpalName method (is there anywhere else that I should be aware of?) That has the plus side of not requiring any changes to the current code –
          Jeremy Hanson made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Eric Westfall added a comment -

          Yeah, as i mentioned in my email I think we can probably boil this down to a documentation issue. Jeremy is going to resolve this one and create a separate JIRA to make sure this gets documented.

          Show
          Eric Westfall added a comment - Yeah, as i mentioned in my email I think we can probably boil this down to a documentation issue. Jeremy is going to resolve this one and create a separate JIRA to make sure this gets documented.
          Peter Giles (Inactive) made changes -
          Comment [ A database independent and performant solution might be to store the lowercase username in an additional column w/ index. Just a thought. ]
          Hide
          Peter Giles (Inactive) added a comment -

          I need to read the full log before I comment :-0

          Show
          Peter Giles (Inactive) added a comment - I need to read the full log before I comment :-0
          Jessica Coltrin (Inactive) made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Shem Patterson (Inactive) made changes -
          Workflow custom [ 66397 ] Copy of custom for rice [ 210896 ]
          Shem Patterson (Inactive) made changes -
          Workflow Copy of custom for rice [ 210896 ] custom [ 220644 ]
          Shem Patterson (Inactive) made changes -
          Workflow custom [ 220644 ] Rice Workflow [ 230392 ]
          Claus Niesen made changes -
          Fix Version/s Not version specific [ 17967 ]

            People

            • Assignee:
              Jeremy Hanson
              Reporter:
              Eric Westfall
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Structure Helper Panel