-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Changed profile.username to profile.displayName for header username display #1388
Fix: Changed profile.username to profile.displayName for header username display #1388
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, could get some simplification.
Actually the replace(' ms-ad',...
) is probably wrong at all places, as e.g. the oidc.
prefix would need the same handling.
Users without a profile should be limited to the system accounts, if I am not mistaken.
@@ -22,7 +22,7 @@ export const selectCurrentUserName = createSelector( | |||
selectCurrentUser, | |||
(profile, user) => { | |||
if (profile) { | |||
return profile.username.replace("ms-ad.", ""); | |||
return profile.displayName.replace("ms-ad.", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the replace is probably not needed here then. The backend prepends a marker for the auth source to the username, but should not do that for the displayName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked and all user types do implement the user profile (local, oidc, ldap) . jwt auth relies on the other strategies for initial login/account creation, so we proably can rely on the profile being there (or adopt the BE to ensure that),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bpedersen2 hopefully I understood your comment above correctly.
To clarify further: user profile is created for all accounts, no matter which auth strategy they use. It is safe to assume that profile is always present.
I agree with you that the displayName should not be stored in the db with a prefix.
We should display it as it is saved in the database.
@@ -22,7 +22,7 @@ export const selectCurrentUserName = createSelector( | |||
selectCurrentUser, | |||
(profile, user) => { | |||
if (profile) { | |||
return profile.username.replace("ms-ad.", ""); | |||
return profile.displayName.replace("ms-ad.", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bpedersen2 hopefully I understood your comment above correctly.
To clarify further: user profile is created for all accounts, no matter which auth strategy they use. It is safe to assume that profile is always present.
I agree with you that the displayName should not be stored in the db with a prefix.
We should display it as it is saved in the database.
…ion-process-confi
…ion-process-confi
updated e2e docker compose file to use latest scichat-loopback image from github image repo
…ion-process-confi
removed replace instruction in case username has a prefix in it. We assume that the prefix is an error to be fixed in data with an additional script.The suffix needs to be removed in data.
Restored docker compose
…ion-process-confi
…ion-process-confi
Description
This PR adapts to the SciCatProject/scicat-backend-next#1041
In the above backend PR we have changed
profile.username
to storeoidc.sub
by default, which in some cases might return string of numbers instead of user's name. So the change of displayingprofile.username
toprofile.displayName
on the header section is required.Tests included/Docs Updated?