MF-798 - Add utf8 support for email validation #1082
Conversation
Signed-off-by: Vadim Filin <[email protected]>
filinvadim
force-pushed
the
master
branch
2 times, most recently
from
f7101e0 to
2252e7a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1082 +/- ##
=======================================
Coverage 77.97% 77.97%
=======================================
Files 95 95
Lines 6698 6698
=======================================
Hits 5223 5223
Misses 1161 1161
Partials 314 314 Continue to review full report at Codecov.
|
users/users.go
Outdated
|
|
||
| at := strings.LastIndex(email, "@") | ||
| if at <= 0 || at > len(email)-3 { | ||
| if len(local) == 0 || len(host) == 0 { |
There was a problem hiding this comment.
Wait, we are not checking the extension anymore. Or you revert this len(host) - 3 == 0 or you can maybe do an extra Split to know the real size of the extension.
users/users.go
Outdated
|
|
||
| if len(user) > 64 { | ||
| hs := strings.Split(host, ".") | ||
| if len(es) != 2 { |
users/users.go
Outdated
| return false | ||
| } | ||
|
|
||
| if len(local) > maxLocalLen || len(host) > maxHostLen { |
There was a problem hiding this comment.
Please, add here a maxExtensionLen check
users/users.go
Outdated
|
|
||
| user := email[:at] | ||
| host := email[at+1:] | ||
| if len(local) == 0 { |
There was a problem hiding this comment.
I would do if len(local) == 0 || len(local) > maxLocalLen{ here
users/users.go
Outdated
| } | ||
| domain, ext := hs[0], hs[1] | ||
|
|
||
| if len(domain) == 0 || len(ext) == 0 { |
There was a problem hiding this comment.
and here if len(domain) == 0 || len(domain) > maxDomainLen
users/users.go
Outdated
| return false | ||
| } | ||
|
|
||
| if len(ext) > maxTLDLen { |
There was a problem hiding this comment.
and finally here if len(ext) == 0 || len(ext) > maxTLDLen
Signed-off-by: Vadim Filin <[email protected]>
|
Let's stop here. |
|
@filinvadim how should I understand this comment? Maybe you wanted to say "Thanks @manuio for taking your time in reviewing my PR, I understand that Mainflux is high-quality project that demands though peer-reviews and skills and patience are needed to get my PR to the adequate level to be accepted"? |
|
@filinvadim of course it will not be merged, not until all remarks are resolved. And this is only one reviewer/maintainter, there are several of us. I am closing the issue, I hope this also clarifies why you and other in the industry can trust Mainflux code. |
|
Fixed. |
users/users.go
Outdated
| } | ||
| local, host := es[0], es[1] | ||
|
|
||
| if len(local) == 0 || len(local) > maxLocalLen { |
There was a problem hiding this comment.
Please, replace all len(local) == 0 by local == ""
users/users_test.go
Outdated
| }, | ||
| "validate user with too long email tld": { | ||
| user: users.User{ | ||
| Email: "user@example.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", |
There was a problem hiding this comment.
This and specially the next one are too long. You need a function here. We already did in other places things like:
letters = "abcdefghijklmnopqrstuvwxyz"
func randomString(n int) string {
b := make([]byte, n)
for i := range b {
b[i] = letters[rand.Intn(len(letters))]
}
return string(b)
}
longDomain := randomString(maxDomainLen + 1)
Signed-off-by: Vadim Filin <[email protected]>
|
Thank you @filinvadim ! |
* Add utf8 support for email validation Signed-off-by: Vadim Filin <[email protected]> * Fix review comments Signed-off-by: Vadim Filin <[email protected]> * Refactor Signed-off-by: Vadim Filin <[email protected]> Co-authored-by: Drasko DRASKOVIC <[email protected]>
What does this do?
Fix email address segments legth validation.
Adds validation of email addresses with utf8 encoded characters.
Which issue(s) does this PR fix/relate to?
Resolves #798 .
List any changes that modify/break current functionality
External dependency:
golang.org/x/net/idna.Have you included tests for your changes?
Yes. Tests are extended.
Did you document any new/modified functionality?
No.
Notes