From 1262d254736229618582f0963c9c30c4e66efb98 Mon Sep 17 00:00:00 2001 From: Christopher Speller Date: Wed, 31 Jan 2018 09:49:15 -0800 Subject: User based rate limiting (#8152) --- api/context.go | 45 +++++++++++++-------------------------------- 1 file changed, 13 insertions(+), 32 deletions(-) (limited to 'api') diff --git a/api/context.go b/api/context.go index 34a87e633..84967659d 100644 --- a/api/context.go +++ b/api/context.go @@ -114,38 +114,14 @@ func (h handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { metrics.IncrementHttpRequest() } - token := "" - isTokenFromQueryString := false - - // Attempt to parse token out of the header - authHeader := r.Header.Get(model.HEADER_AUTH) - if len(authHeader) > 6 && strings.ToUpper(authHeader[0:6]) == model.HEADER_BEARER { - // Default session token - token = authHeader[7:] - - } else if len(authHeader) > 5 && strings.ToLower(authHeader[0:5]) == model.HEADER_TOKEN { - // OAuth token - token = authHeader[6:] - } - - // Attempt to parse the token from the cookie - if len(token) == 0 { - if cookie, err := r.Cookie(model.SESSION_COOKIE_TOKEN); err == nil { - token = cookie.Value - - if (h.requireSystemAdmin || h.requireUser) && !h.trustRequester { - if r.Header.Get(model.HEADER_REQUESTED_WITH) != model.HEADER_REQUESTED_WITH_XML { - c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token+" Appears to be a CSRF attempt", http.StatusUnauthorized) - token = "" - } - } - } - } + token, tokenLocation := app.ParseAuthTokenFromRequest(r) - // Attempt to parse token out of the query string - if len(token) == 0 { - token = r.URL.Query().Get("access_token") - isTokenFromQueryString = true + // CSRF Check + if tokenLocation == app.TokenLocationCookie && (h.requireSystemAdmin || h.requireUser) && !h.trustRequester { + if r.Header.Get(model.HEADER_REQUESTED_WITH) != model.HEADER_REQUESTED_WITH_XML { + c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token+" Appears to be a CSRF attempt", http.StatusUnauthorized) + token = "" + } } c.SetSiteURLHeader(app.GetProtocol(r) + "://" + r.Host) @@ -175,11 +151,16 @@ func (h handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if h.requireUser || h.requireSystemAdmin { c.Err = model.NewAppError("ServeHTTP", "api.context.session_expired.app_error", nil, "token="+token, http.StatusUnauthorized) } - } else if !session.IsOAuth && isTokenFromQueryString { + } else if !session.IsOAuth && tokenLocation == app.TokenLocationQueryString { c.Err = model.NewAppError("ServeHTTP", "api.context.token_provided.app_error", nil, "token="+token, http.StatusUnauthorized) } else { c.Session = *session } + + // Rate limit by UserID + if c.App.Srv.RateLimiter != nil && c.App.Srv.RateLimiter.UserIdRateLimit(c.Session.UserId, w) { + return + } } if h.isApi || h.isTeamIndependent { -- cgit v1.2.3-1-g7c22 From 81e67f8759aaa069117dadfcfec8819649f6590b Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 5 Feb 2018 10:54:13 -0500 Subject: ABC-179: check email verification last (#8172) * ABC-179: check email verification last This change changes the authentication checks to be: * "preflight checks" ** mfa ** not disabled ** login attempts * password * "postflight checks" ** email verified Checking whether the email is verified or not last avoids the weird edge case where entering any bogus password for an account with an unverified email shows a message about verifying the email and offering to resend. * fix invalid unit test assertion Client.CreateUser returns a user whose password has been sanitized. Adopt the pattern in the previous assertions to use a new variable name and test the password on the original model.User object. This didn't expose any underlying broken behaviour, but the test wouldn't have caught it if it had regressed. Also fix a minor typo. --- api/user.go | 2 +- api/user_test.go | 45 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 5 deletions(-) (limited to 'api') diff --git a/api/user.go b/api/user.go index ae8a6cab6..a2e06c013 100644 --- a/api/user.go +++ b/api/user.go @@ -1159,7 +1159,7 @@ func completeSaml(c *Context, w http.ResponseWriter, r *http.Request) { } return } else { - if err := c.App.CheckUserAdditionalAuthenticationCriteria(user, ""); err != nil { + if err := c.App.CheckUserAllAuthenticationCriteria(user, ""); err != nil { c.Err = err c.Err.StatusCode = http.StatusFound return diff --git a/api/user_test.go b/api/user_test.go index b74039ce9..8d6aad22b 100644 --- a/api/user_test.go +++ b/api/user_test.go @@ -16,6 +16,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/mattermost/mattermost-server/app" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" @@ -187,7 +189,7 @@ func TestLogin(t *testing.T) { } if _, err := Client.Login(ruser2.Data.(*model.User).Email, user2.Password); err != nil { - t.Fatal("From verfied hash") + t.Fatal("From verified hash") } Client.AuthToken = authToken @@ -199,14 +201,49 @@ func TestLogin(t *testing.T) { Password: "passwd1", AuthService: model.USER_AUTH_SERVICE_LDAP, } - user3 = Client.Must(Client.CreateUser(user3, "")).Data.(*model.User) - store.Must(th.App.Srv.Store.User().VerifyEmail(user3.Id)) + ruser3 := Client.Must(Client.CreateUser(user3, "")).Data.(*model.User) + store.Must(th.App.Srv.Store.User().VerifyEmail(ruser3.Id)) - if _, err := Client.Login(user3.Id, user3.Password); err == nil { + if _, err := Client.Login(ruser3.Id, user3.Password); err == nil { t.Fatal("AD/LDAP user should not be able to log in with AD/LDAP disabled") } } +func TestLoginUnverifiedEmail(t *testing.T) { + assert := assert.New(t) + + th := Setup().InitBasic() + defer th.TearDown() + + Client := th.BasicClient + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.EmailSettings.EnableSignInWithEmail = true + cfg.EmailSettings.RequireEmailVerification = true + }) + + Client.Logout() + + user := &model.User{ + Email: strings.ToLower(model.NewId()) + "success+test@simulator.amazonses.com", + Nickname: "Corey Hulen", + Username: "corey" + model.NewId(), + Password: "passwd1", + EmailVerified: false, + } + user = Client.Must(Client.CreateUser(user, "")).Data.(*model.User) + + _, err := Client.Login(user.Email, user.Password+"invalid") + if assert.NotNil(err) { + assert.Equal("api.user.check_user_password.invalid.app_error", err.Id) + } + + _, err = Client.Login(user.Email, "passwd1") + if assert.NotNil(err) { + assert.Equal("api.user.login.not_verified.app_error", err.Id) + } +} + func TestLoginByLdap(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() -- cgit v1.2.3-1-g7c22