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

Create User invalid request has a response which returns 500 #5228 #5229

Merged

Conversation

armaganpekatik
Copy link
Contributor

Fixes #5228

Summary

When you try to change any user password, invalid password set causes an error response has a status code of 500 which is system exception.

@armaganpekatik armaganpekatik marked this pull request as draft August 6, 2022 15:54
@armaganpekatik armaganpekatik marked this pull request as ready for review August 6, 2022 15:58
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

Although I understand the desired goal here, this does introduce a process change, and you might be masking an error that would have historically been logged.

For example, UserCreate can fail for password, duplicate email, or other reasons. This process now assumes that all failures are invalid passwords which is not correct. I think if any attempt to bypass logging etc should be adjusted.

Now, I could be convinced that ALL errors should be a 400 response, rather than a 500 though?

@dnnsoftware/approvers any thoughts?

@david-poindexter
Copy link
Contributor

david-poindexter commented Aug 7, 2022

I'm thinking we should be using 403 Forbidden in this case, no? I agree that 500 Internal Server Error is not good.

Perhaps you are on to something though with the 400.

@armaganpekatik
Copy link
Contributor Author

Although I understand the desired goal here, this does introduce a process change, and you might be masking an error that would have historically been logged.

For example, UserCreate can fail for password, duplicate email, or other reasons. This process now assumes that all failures are invalid passwords which is not correct. I think if any attempt to bypass logging etc should be adjusted.

Now, I could be convinced that ALL errors should be a 400 response, rather than a 500 though?

@dnnsoftware/approvers any thoughts?

You are right for the naming, we can change InvalidPasswordException to InvalidUserRegistrationException. This was because I was thinking that I can define password error at the beginning. So the error is coming from https://github.com/dnnsoftware/Dnn.Platform/blob/develop/DNN%20Platform/Library/Entities/Users/UserController.cs#L779 and all of them are user validation error. Please see last commit.

Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

This looks acceptable to me

Copy link
Contributor

@donker donker left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create User invalid request has a response which returns 500
4 participants