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

feat: make user first and last name optional fields #4988

Closed

Conversation

Alexei-Barnes
Copy link
Contributor

@Alexei-Barnes Alexei-Barnes commented Jan 5, 2023

This change partially completes #4386 by making the First Name and Last Name fields optional.

It completes the following acceptance criteria:

  • I can register with only a username
  • If I have set only a username in the ZITADEL console my username is shown instead of Firstname, Lastname
  • My Display Name shows the Firstname, Lastname if filled, if not it shows my username
  • We use the display name in emails instead of First and Lastname

It does not address the following acceptance criteria:

  • If I do not have an email address on my user a password reset is not possible and shows an error
  • If I don't have an email, the email setup or email verification screen is not shown in the login flow
  • I am able to login if my user has set no email
  • I am able to login if my user has set an email

This bites off the easiest chunk of this issue, since there is still some figuring out to do around user email address.

@netlify
Copy link

netlify bot commented Jan 5, 2023

Deploy Preview for docs-zitadel-com canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 61b53d8
🔍 Latest deploy log https://app.netlify.com/sites/docs-zitadel-com/deploys/63c571e549d35c000862eeec

@@ -39,7 +39,7 @@ services:
user: '$UID'
volumes:
- .:/e2e
command: 'sh -c "npm ci --omit=dev && npm run lint && npx wait-on http://localhost:8080/debug/ready"'
command: 'sh -c "npm ci --omit=dev && npm run lint && npx wait-on -v -l http://host.docker.internal:8080/debug/ready"'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fix WSL support for this compose file - without this npx wait-on never finishes for me.

@Alexei-Barnes Alexei-Barnes changed the title Make user first and last name optional fields feat: make user first and last name optional fields Jan 5, 2023
@Alexei-Barnes
Copy link
Contributor Author

Alexei-Barnes commented Jan 5, 2023

I suggest we should create a feature branch in the zitadel repo, and re-target this PR to that branch, since this doesn't complete all AC.

This is necessary now that first and last name are optional. Display name will default to first and last name, or username.
@hifabienne
Copy link
Member

Hei @Alexei-Barnes
Thank for the PR.
Just one thing that comes to my mind is, if we merge this to main some of the existing customers will probably have a problem, because they assume that each user has a first- and a lastname.
We will probably have to implement an additional policy/settings where you can configure which fields are required and which are optional.

@Alexei-Barnes
Copy link
Contributor Author

I did some testing on this today and found that this doesn't finish the problem of making first & last name optional - during registration from an IdP that didn't provide these details, the application got confused and produced the error Errors.User.NotHuman, and then Errors.ExternalIDP.NoExternalUserData. I'll need to investigate both of these issues.

@Alexei-Barnes
Copy link
Contributor Author

I've managed to shift off other priorities and back to this, so hopefully I can fix those problems today.

@Alexei-Barnes
Copy link
Contributor Author

OK the "NotHuman" error is simple;

internal/user/repository/view/model/user.go:130

func (h *HumanView) IsZero() bool {
	return h == nil || h.FirstName == ""
}

will return true when called by

internal/auth/repository/eventsourcing/eventstore/auth_request.go:662

	// if there's an active (human) user, let's use it
	if user != nil && !user.HumanView.IsZero() && domain.UserState(user.State).NotDisabled() {
		request.SetUserInfo(user.ID, loginName, user.PreferredLoginName, "", "", user.ResourceOwner)
		return nil
	}

and conclude that this user can't be found.

@Alexei-Barnes
Copy link
Contributor Author

OK fixing that fixed both of the problems I identified, I managed to register a user via AWS Cognito with no first or last name configured, then use that to login.

This changes the `IsZero` test for `HumanView` by ensuring that we don't assume that first name must be set for a user to be valid.
@Alexei-Barnes
Copy link
Contributor Author

I've noticed another problem with this change, which is that some pages show a robot avatar for the user, while others do not. I'll need to figure out why and update the UI to ensure the user is not considered a bot.

@hifabienne
Copy link
Member

I have created a new user story regarding the topic about setting the required fields, before we can merge this, we should implement that: #5333

@muhlemmer
Copy link
Contributor

As we are currently re-writing user management as user schema, this PR seems to have become obsolete. Apologies for not merging your code. But know your effort was not in vain, it sparked a heavy and long-running debate which will result in a better zitadel that allows more flexibility in user management,

Thanks for the effort and hope seeing you around!

@muhlemmer muhlemmer closed this Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants