fix: remove panic() and relay password hashing errors to UI - #1014
fix: remove panic() and relay password hashing errors to UI#1014eternal-flame-AD wants to merge 1 commit into
Conversation
d945ca6 to
6f6ec40
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1014 +/- ##
==========================================
- Coverage 74.53% 74.33% -0.20%
==========================================
Files 66 66
Lines 3483 3499 +16
==========================================
+ Hits 2596 2601 +5
- Misses 688 695 +7
- Partials 199 203 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| @@ -39,9 +39,14 @@ func (s *SessionSuite) BeforeTest(suiteName, testName string) { | |||
| s.notified = false | |||
| s.a = &SessionAPI{DB: s.db, NotifyDeleted: s.notify} | |||
There was a problem hiding this comment.
Have you thought about adding validation to the model, and letting the password handling as is? E.g.
Pass string `json:"pass,omitempty" form:"pass" query:"pass" binding:"required,max=72"`
This likely makes the error message more explicit. I'm okay with the current solution, but this may be a better solution.
There was a problem hiding this comment.
We will have to hardcode this number. I can see in the future users might request we migrate to other password hashes so I didn't go that route.
There was a problem hiding this comment.
Yeah, but the error message will be better, currently it's a 500 internal server error for something that the user provided an invalid value for. How about we do both? Your change and the extra validation?
6f6ec40 to
38eccee
Compare
| if err != nil { | ||
| panic(err) | ||
| if err != nil && err != bcrypt.ErrPasswordTooLong { | ||
| err = ErrUnexpectedError |
There was a problem hiding this comment.
If we mask the error here, we don't get the initial error anywhere, meaning if a user misconfigures the passstrength, it will always return unexpected error without any log about the actual error. I think we can pass the full error without masking into the 500 internal server error, and with the extra validation the length check is done before and results in a 400.
2 participants
Fixes #1013