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

Fix #4752: validate the username and email. #4756

Merged
merged 3 commits into from
Oct 24, 2024
Merged

Conversation

zyhfish
Copy link
Contributor

@zyhfish zyhfish commented Oct 21, 2024

No description provided.

@sbwalker
Copy link
Member

@zyhfish can you please explain why there is a need for InstallUserManager? UserManager already pulls in UserManager<IdentityUser> through its constructor. I am also not understanding why you are calling var installUserManager = new InstallUserManager<IdentityUser>() - this is not using dependency injection and is a pattern we are trying to avoid in Oqtane.

@sbwalker
Copy link
Member

sbwalker commented Oct 22, 2024

@zyhfish on a side note I am wondering why so much of the code has been reformatted ie. 75% of the changes in this PR are not actually modifications at all, they are just reformatting. This makes the PR very difficult to review. It also makes the formatting of this class different than the rest of the Oqtane solution - which is not good for consistency (which is one of the Oqtane philosophies). Is the reformatting being caused by a third party development tool such as Resharper?

@zyhfish
Copy link
Contributor Author

zyhfish commented Oct 22, 2024

Hi @sbwalker ,
the introduce of InstallUserManager is because in the built-in user manager, it will try to validate exsiting user by username or email in the database, but actually we don't have any valid database yet before installation, so that it will throw out exception, so that I overwrite the methods to avoid the exception, this more work like a fake class and will only been used in the validation method, so it's not needed to register in DI.

the code formatting should be the different of "Tab" settings, it used 4 spaces instead of a real Tab char.

@sbwalker
Copy link
Member

@zyhfish the validation logic in the .NET Identity library is rather basic (https://github.com/aspnet/Identity/blob/master/src/Core/UserValidator.cs) - basically Oqtane does not require unique email addresses, so the only thing this class is doing is validating the value conforms to the AllowedUserNameCharacters and then validating that the username is unique. So I believe that it may be better for Oqtane to have its own implementation for the ValidateUser method which does not rely on the .NET Identity classes at all. This would greatly simplify the code (ie. avoid the need for the mock class) and would provide more flexibility in the future for custom validations if necessary.

@zyhfish zyhfish closed this Oct 23, 2024
@zyhfish zyhfish deleted the task/fix-4752 branch October 23, 2024 14:52
@sbwalker
Copy link
Member

the majority of this PR was fine - I was just suggesting that the validation logic could be implemented in the UserManager without bothering to leverage the .NET Identity classes/methods. This would make the code simpler, reduce the number of dependencies, and provide more flexibility. Does this make sense?

@zyhfish zyhfish reopened this Oct 24, 2024
@zyhfish
Copy link
Contributor Author

zyhfish commented Oct 24, 2024

Hi @sbwalker , the PR has been updated to use the simple chars validation.

@sbwalker sbwalker merged commit 6719d24 into oqtane:dev Oct 24, 2024
1 check passed
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.

2 participants