AD authentication display name for authors not showing

[alondhe]

Expected behavior

When using AD authentication, the author name shows the AD display name ("Last Name, First Name")

Actual behavior

The AD user name (SAMAccountName) is shown instead

Steps to reproduce behavior

Connect to AD, log in, create a new concept set or cohort definition

[konstjar]

It comes from default settings. You can configure it by setting needed AD parameter in

security.ad.userMapping.displaynameAttr

https://github.com/OHDSI/WebAPI/blob/master/pom.xml#L122

[chrisknoll]

@konstjar : that attribute controls how the user name is displayed in the datatables?

I'm not suggesting that we configure it to work this way: just clarifying what that setting actually does.

I think @alondhe is pointing out that the display name is not being shown in the datatables. I don't think we should change the column value, but rather return an additional 'user display name' column (or 2 for each create/update)...because we do not want to break backwards behavior by making one version return an userId and another version return the value as a userName.

[gklebanov]

It is a bit of a legacy design issue:

1. A few years ago, the half working Shiro solution was in place where the user ID was used to be stored in database if it was available e.g. user authenticated. Then this is what is being used in table, when it is available

2. Proper authentication and authorization solution was added later and authentication fixed, e.g. now user could be authenticated against various identity providers, including AD. That authenticated user's name can now be displayed in top right corner - either ID (to be consistent) or full name. It is a session level variable that is cached when user is logged in.

Now, back to tables.

To display a full name of a user in tables, we need to consider that it cannot be a separate call to AD/IP to resolve an ID to display name for each record in a table - this would be a bad design and extremely slow. In a proper design, all ATLAS users key attributes should be cached locally, for all users. Then ATLAs needs to understand how to handle cases when people leave the company and their account becomes disables in AD/IP or lose access to ATLAS? Records should never be deleted, only disabled - otherwise any records created by these users would only display user IDs while active users would still be displayed. But none of these mechanisms are in ATLAS today.

All of these issues can be properly addressed, of course. But maybe this is more of a v3 kind of work a significant user management redesign would be required.

[alondhe]

I don't know if this helps, but as I was stepping through the code, I noticed that the "name" value in sec_user is correct after logging in initially. The value gets overwritten with the login name once I navigate to the Concept Sets datatable. Why should that step need to update the sec_user record? Why not just use the record as-is?

[alondhe]

So it looks like 2 issues:

1. WebAPI - UserEntity.java will replace the name with the login value. At some point, 1 of the calls to setName will supply the login rather than the display name.
2. Atlas - The DatatableUtils.js uses the "login" value rather than the "name"

For the first issue, this is a hack:

  public void setName(String name) {
    if (this.name == null) {
      this.name = name; // ensure we have a value as this is a required field
    }
    else if (!this.name.contains(",") && name.contains(",")) {
      this.name = name; // update the value if our current looks like a login and the new value looks like a display name
    }
  }

For the second, just changing any instance of createdBy.login to createdBy.name then uses this hack.

I'd prefer not to hack this up, though. Any ideas on why one of the calls to setName supplies the login value?

[chrisknoll]

Thanks for the information, @gklebanov . Regardless of the legacy concerns, I think we should make some changes based on @alondhe's findings.

Firstly, I don't think it's appropriate for sec_user should be overwritten when navigating to Concept Sets, if that's the actual case as @alondhe describes. Just smells bad, and I think we can do better:

1. There is an initial 'user setup' action that happens when a person logs in for the first time. It populates sec_user and some additional steps. It's at this point that we should have all necessary information from the security provider (ad, oid, etc) to populate the login and name of the user. We should understand the reason why this 'double update' is necessary, but remove it if possible.
2. We should provide a UI for the user to modify their own user attributes (and with possible configuration options to prevent this, if we don't want people to change certain attributes, like display name). But I'd rather have my JnJ display name changed to something more friendly, and it would be a nice feature if there was a UI to adjust that if security is enabled.
3. I agree that checking for a name that contains ',' is a hack :)

So, maybe there is a simple solution to this. If we're worried about 'pre-existing data' in the user table where the login name is overwritten, then point (2) could address cases where people want to set their own name.

@alondhe , since it looks like you traced through his quite a bit, could you just confirm the 'state flow' of how the database is touched between the login of the user and then subsequent access of the WebAPI services? Maybe we can come up with an alternate flow which lets you get the result you want in the UI.

[alondhe]

Hi @chrisknoll,

Did some more digging, and it looks like the underlying issue is in the prehandle function of the UpdateAccessTokenFilter class. For my AD implementation, the principal object is just a string, so it can't be used to grab a "name"; instead, all we have here is a login name to work with.

From there, the registerUser function in the PermissionManager class attempts to update the name field.

So my new hack, which is slightly better than the comma-based one, is to basically take out the update step in registerUser. But what I'd like to understand better is why the principal object can't be resolved as anything more than a string.

Thanks,
Ajit

[gklebanov]

The reason for me going back in time is that I think there are 3 separate issues here, and I am not sure which one we are trying to resolve:

1. Alternative to what is being displayed in the right top corner for the authenticated user
2. The value of that is being overwritten when a user goes to the concept sets. Sounds weird and probably is a bug
3. What is being displayed as names of created / changed by field in all tables which are listing artifacts, e.g. concept sets, cohorts, TxPathways, IRs, PLEs, PLP and so on

While No. 1 and No. 2 are relevant for the currently logged in user - where No. 2 is a a probable bug, No. 3 is relevant for other users who ever created and edited the artifacts, thus having nothing to do with a current session.

Which problem exactly are we trying to solve here? Or all 3 of them?

No. 1 - make it configurable. Why? Display name is exactly what it means - how the organization wants your name to be displayed - consistently. Fine, I understand that some IPs might not have this option and it can be made configurable. Or do they? someone needs to do some research and make a definite statement for all supported IPs, not just AD. For AD - "display name" is how your organization wants it to be displayed, consistently across many apps. Is it really a good idea to go against it?

No. 2 - sounds like a bug. Needs to be fixed.

No. 3 - this is a can of worms we are opening here. The artifacts are storing a reference to the user record vs. a display name of the user. Big difference. In the proper design, it is a reference user ID while display name should be kept somewhere else and resolved based on user ID. If we start storing the name we want to be displayed vs. user IDs, what are the consequences?

Which one of these 3 issues are we trying to solve here?

[gklebanov]

from Chris > @konstjar : that attribute controls how the user name is displayed in the datatables?

this is he key question here. Since the artifact author in most of the cases has nothing to do with the currently authenticated user. And no - it is not controlled by that setting - only what is being displayed for the currently displayed person

[alondhe]

@gklebanov -- good questions! Personally, I just care about showing the display name that the auth provider has. I'm not that interested in being able to personalize it, I think that adds unneeded complexity.

The reason the display name is important is because we may have a large group of users with similar logins, and to avoid confusion, being able to show the Last Name, First Name that our AD provides would be great.

In my hack setup as described here, where I prevent updates of the sec_user.name value:

• The initial user login attempt grabs the display name into the sec_user record
• The author name resolving step will use the user id (e.g. 1000) to join to the sec_user table and grab the name value.

Once I get this on a server with users, I can test it some more.

[chrisknoll]

Can we close this issue issue if the PR addresses the concern?