Add clientapi tests #2916
Add clientapi tests #2916
Conversation
S7evinK
added
tests
Codecov ReportBase: 35.75% // Head: 36.47% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2916 +/- ##
==========================================
+ Coverage 35.75% 36.47% +0.72%
==========================================
Files 494 494
Lines 54673 54657 -16
==========================================
+ Hits 19549 19937 +388
+ Misses 32573 32149 -424
- Partials 2551 2571 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Needs matrix-org/complement#573 for Complement to pass, as this also removes the deprecated (since r0.4.0) |
| @@ -93,6 +90,10 @@ func (c *ClientAPI) Verify(configErrs *ConfigErrors, isMonolith bool) { | |||
| if c.RecaptchaSitekeyClass == "" { | |||
| c.RecaptchaSitekeyClass = "g-recaptcha-response" | |||
| } | |||
| checkNotEmpty(configErrs, "client_api.recaptcha_public_key", c.RecaptchaPublicKey) | |||
There was a problem hiding this comment.
Why shift the order of checks here?
There was a problem hiding this comment.
Could remove it completely, given we either have something set or default to reCaptcha (if nothing is set).
|
|
||
| serveSuccess() | ||
| return nil | ||
| clientIP := req.RemoteAddr |
There was a problem hiding this comment.
This needs to honour the real IP header in case of reverse proxies. It's a config option iirc
There was a problem hiding this comment.
It is, but unfortunately just for the sync api :/
clientapi/routing/auth_fallback.go
Outdated
| err := req.ParseForm() | ||
| if err != nil { | ||
| util.GetLogger(req.Context()).WithError(err).Error("req.ParseForm failed") | ||
| res := jsonerror.InternalServerError() |
There was a problem hiding this comment.
Most certainly not a 500, surely 4xx?
clientapi/routing/register_test.go
Outdated
| { | ||
| name: "valid username", | ||
| localpart: "valid", | ||
| domain: "localhost", |
There was a problem hiding this comment.
Surprised we don't ever test bad domains.
There was a problem hiding this comment.
I also had that added, just to see that we either use the server_name from the configuration OR one from a virtual host.
And yea, something like http://localhost would produce a valid username
clientapi/routing/register_test.go
Outdated
| }, | ||
| }, | ||
| { | ||
| name: "invalid username", |
There was a problem hiding this comment.
Say why (caps).
Also, it would be good to add tests for:
- unicode characters e.g emoji
- ASCII symbols e.g $
- a complex but valid username beyond the existing test for "valid" e.g
f00_bar-baz.=40/is valid I think? Test it.
clientapi/routing/register_test.go
Outdated
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if got := validateUsername(tt.localpart, tt.domain); !reflect.DeepEqual(got, tt.want) { |
There was a problem hiding this comment.
Whilst we are here... please move validateUsername to internal or some other package and call it from cmd/create-account to prevent people registering usernames with caps or other things like that.
clientapi/routing/register_test.go
Outdated
| t.Errorf("validateUsername() = %v, want %v", got, tt.want) | ||
| } | ||
| if got := validateApplicationServiceUsername(tt.localpart, tt.domain); !reflect.DeepEqual(got, tt.want) { | ||
| if got != nil && got.JSON != jsonerror.InvalidUsername("Username cannot start with a '_'") { |
There was a problem hiding this comment.
Needs more comments to explain why you're doing this check. I assume because appservices are allowed to use _.
clientapi/routing/register_test.go
Outdated
| password: "shortpw", | ||
| want: &util.JSONResponse{ | ||
| Code: http.StatusBadRequest, | ||
| JSON: jsonerror.WeakPassword(fmt.Sprintf("password too weak: min %d chars", minPasswordLength)), |
There was a problem hiding this comment.
I generally dislike doing direct checks on error strings, as they aren't really part of the public API. We should be checking the errcode. Write a helper function and gjson out the code? This applies throughout these tests.
There was a problem hiding this comment.
Like above, we should just remove the JSON here and actually return error types. (Same for the other validate* functions)
This PR
/registerand auth fallback.homeserverfield from responses to/register