Skip to content
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

Do not allow users to change username when logging for the first time #17265

Closed
alexeykazakov opened this issue Jun 26, 2020 · 28 comments
Closed
Labels
area/install Issues related to installation, including offline/air gap and initial setup kind/enhancement A feature request - must adhere to the feature request template. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes severity/P1 Has a major impact to usage or development of the system.
Milestone

Comments

@alexeykazakov
Copy link

Is your enhancement related to a problem? Please describe.

We have a Che operator 7.14.2 installed an OCP cluster with the following configuration:

  auth:
    externalIdentityProvider: false
    openShiftoAuth: true
  server:
    customCheProperties:
      CHE_INFRA_KUBERNETES_NAMESPACE_DEFAULT: <username>-code
    allowUserDefinedWorkspaceNamespaces: false

Each user has <openshift-username>-code namespace pre-created.
When a user is trying to login into Che Dashboard the very first time Che's Keycloak shows the following form:

che

And now if the user edit the username and use another username (not OpenShift username) then he won't be able to start a workspace because Che tries to use not-existing namespace to create a workspace: <che-username>-code instead of `-code.

Describe the solution you'd like

Ideally, user should not be asked to re-enter username, email, names when loging to Che at all. Che just should use the user from OpenShift. Email and Name is not available in OpenShift, but why Che needs them? If it needs them for Git commits or something then it should ask it when/if actually needed. Not in advance.

But at least we should have an option to forbid users to change usernames during Che login because it breaks users.

@alexeykazakov alexeykazakov added the kind/enhancement A feature request - must adhere to the feature request template. label Jun 26, 2020
@che-bot che-bot added the status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. label Jun 26, 2020
@xcoulon
Copy link
Contributor

xcoulon commented Jun 29, 2020

it's even worse if the user has an autofill plugin that will tend to set the fields with real names, including spaces. This is what I did by mistake, and now I have the following error message when trying to create a workspace on Hosted Toolchain:

The workspace would be started in a namespace/project 'xavier coulon-code', which is not a valid namespace/project name.

@l0rd l0rd added severity/P1 Has a major impact to usage or development of the system. team/deploy and removed status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. labels Jun 29, 2020
@l0rd
Copy link
Contributor

l0rd commented Jun 29, 2020

I agree on the username. email/name are a different story and can be discussed in a different issue.

@l0rd l0rd added the new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes label Jun 29, 2020
@ibuziuk
Copy link
Member

ibuziuk commented Jul 27, 2020

@tolusha will you be able to prioritize the issue for the upcoming sprint. This is a fairly important integration issue for Hosted Toolchain

@tolusha
Copy link
Contributor

tolusha commented Jul 27, 2020

@ibuziuk
I haven't investigated yet, that's why I am not sure if something can be done on deployment level.
Is it something related to dashboard ?

Maybe @skabashnyuk knows

@tolusha
Copy link
Contributor

tolusha commented Jul 27, 2020

@ibuziuk
I see. We can improve it.
I will plan for the next sprint.

@tolusha tolusha added this to the Backlog - Deploy milestone Jul 27, 2020
@l0rd l0rd added area/install Issues related to installation, including offline/air gap and initial setup and removed team/deploy labels Jul 29, 2020
@l0rd l0rd changed the title Do not allow users to change usernames when logging to Che Dashboard Do not allow users to change username when logging for the first time Jul 29, 2020
@ibuziuk
Copy link
Member

ibuziuk commented Jul 29, 2020

@tolusha thanks a lot this is yet another issue that is important for Hosted Toolchain, hopefully, it will be possible to have a fix for CRW 2.4

@tolusha tolusha mentioned this issue Jul 30, 2020
42 tasks
@tolusha tolusha modified the milestones: Backlog - Deploy, 7.18 Aug 4, 2020
@tolusha
Copy link
Contributor

tolusha commented Aug 10, 2020

I thought having editUsernameAllowed: false in che realm would solve the issue.
But it doesn't work at review profile config stage

@tolusha
Copy link
Contributor

tolusha commented Aug 19, 2020

There is mixing up with usernames.
When OpenShift user is logged in the corresponding keycloak user is created. The keycloak username can be different from the OpenShift one. The problem is that we use the keycloak username instead of OpenShift username for namespace name.

@tolusha
Copy link
Contributor

tolusha commented Aug 19, 2020

For keycloak user there is a provider username which is supposed to be taken instead of Keycloak username.
Untitled

@tolusha tolusha removed this from the 7.18 milestone Aug 26, 2020
@ibuziuk
Copy link
Member

ibuziuk commented Sep 2, 2020

@tolusha looks like the issue was moved out from the sprint, but we absolutely need a solution for it. Can we simply make the username field uneditable?

@tolusha
Copy link
Contributor

tolusha commented Sep 2, 2020

@ibuziuk
I can introduce a non default theme where username will be a readonly field.
It will require configuring keycloak to select appropriate theme. Does it work for you?

Untitled

@RickJWagner
Copy link
Contributor

Please leave some way for the user to choose the username if needed.
Example: If OAuth is used, the user will have OpenShift username. But OpenShift allows some names that CRW does not handle well (i.e. myuser@mycompany).
In this case, it is a useful workaround to allow the user to choose a username of their choosing (one without the '@' character that CRW forbids.)
Perhaps we could just provide the default username pre-populated in the form? This will encourage users to accept this value, if they change it they should be assumed to know what they are doing. (If they break things by choosing a username that won't have a namespace, it is their own fault).

@alexeykazakov
Copy link
Author

It's pre-populated right now. And doesn't work well. The form also requires to provide Name & email (not sure why, but it's a different issue). And it's usual for auto-fill in browser, suggest Name, email. But by using the auto-fill the username can be automatically changed too! So users don't even notice that they username is changed. We experience this issue with every other user. So it's very common.

@alexeykazakov
Copy link
Author

Maybe this option (disable changing usernames) can be in a Che configuration option? In Hosted Toolchain we don't have the issue with invalid usernames. All openshift usernames are guaranteed to be be valid for Che.

@tolusha
Copy link
Contributor

tolusha commented Sep 3, 2020

For me disabling username is kind of hack.
The long term solution should be #15323

@RickJWagner
If I introduce something then it won't be a default behavior. There won't be impact on CRW customers.

@ibuziuk
Copy link
Member

ibuziuk commented Sep 3, 2020

@tolusha as a quickfix before #15323 I would propose having a username input field readonly / uneditable on the registration level. Another option would be making the username editable but have the validation in place to make it k8s DNS compatible.

@tolusha
Copy link
Contributor

tolusha commented Sep 3, 2020

@ibuziuk @alexeykazakov
Keeps username field editable but adds validation:
#17780

@RickJWagner
No impact on customers since CRW uses another identity provide image.

@l0rd
Copy link
Contributor

l0rd commented Sep 3, 2020

@tolusha validation is a good compromise and should be ported to CRW as well. Is there any reason why we should not port the upstream PR to the CRW identity provider image?

@alexeykazakov
Copy link
Author

alexeykazakov commented Sep 3, 2020

Just for clarification. In Hosted Toolchain the problem is not that users can set Che username to a not DNS complaint one. The problem is that users can set Che username which won't match the OpenShift username.
We pre-create namespaces for Che workspaces. Users don't have permissions to create namespaces. So, if some user sets the Che username to a different username then the workspace namespace won't match the pre-created one: <openshift-username>-code and Che won't work for this user at all.
All OpenShift usernames in Hosted Toolchain are DNS complaint.

@tolusha
Copy link
Contributor

tolusha commented Sep 3, 2020

@alexeykazakov
I will introduce a new theme for your case..

@tolusha
Copy link
Contributor

tolusha commented Sep 8, 2020

@l0rd
I don't think that we are the only one who use image registry.redhat.io/rh-sso-7/sso74-openshift-rhel8:7.4 for authentication and adding limitation might not be acceptable in general.

@tolusha tolusha closed this as completed Sep 8, 2020
@tolusha tolusha added this to the 7.19 milestone Sep 8, 2020
@l0rd
Copy link
Contributor

l0rd commented Sep 9, 2020

@l0rd I don't think that we are the only one who use image registry.redhat.io/rh-sso-7/sso74-openshift-rhel8:7.4 for authentication and adding limitation might not be acceptable in general.

I don't think neither. But that doesn't justify the fact that a feature is available upstream but not downstream.

@tolusha
Copy link
Contributor

tolusha commented Sep 10, 2020

@l0rd I see. I will check what we can do here.

@tolusha
Copy link
Contributor

tolusha commented Sep 10, 2020

I've created issue to add changed to CRW as well.
#17827

@alexeykazakov
Copy link
Author

That new theme will be available in Che operator 7.19, right?

@tolusha
Copy link
Contributor

tolusha commented Sep 16, 2020

@alexeykazakov
yes, 7.19

@ibuziuk
Copy link
Member

ibuziuk commented Oct 2, 2020

@tolusha could you please clarify how to enable the theme for making the username uneditable in 7.19.0 ?

@tolusha
Copy link
Contributor

tolusha commented Oct 2, 2020

@ibuziuk
By default 7.19.0 adds check if username complies DNS naming convention.
If you would like make username uneditable then you have to login into keycloak and change login theme to che-username-readonly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/install Issues related to installation, including offline/air gap and initial setup kind/enhancement A feature request - must adhere to the feature request template. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

7 participants