From cbc5995ad35eaf27f71fd16e0e853bc6a3111810 Mon Sep 17 00:00:00 2001 From: konrad Date: Wed, 19 Dec 2018 21:05:25 +0000 Subject: [PATCH] Various user fixes (#38) --- Featurecreep.md | 4 ++-- REST-Tests/users.http | 2 +- config.yml.sample | 2 ++ docs/config.md | 2 ++ pkg/config/config.go | 1 + pkg/mail/mail.go | 4 ++++ pkg/models/fixtures/users.yml | 17 ++++++++++++----- pkg/models/list_users_test.go | 4 ++-- pkg/models/namespace_users_test.go | 4 ++-- pkg/models/unit_tests.go | 6 ------ pkg/models/user.go | 10 ++++++---- pkg/models/user_add_update.go | 15 +++++++++------ pkg/models/user_password_reset.go | 9 +++++---- pkg/models/user_test.go | 6 +++--- pkg/routes/api/v1/user_password_reset.go | 4 ++++ pkg/routes/routes.go | 16 ---------------- 16 files changed, 55 insertions(+), 51 deletions(-) diff --git a/Featurecreep.md b/Featurecreep.md index b9f2614ee..2d4d8269a 100644 --- a/Featurecreep.md +++ b/Featurecreep.md @@ -237,9 +237,9 @@ Teams sind global, d.h. Ein Team kann mehrere Namespaces verwalten. ### Bugfixes -* [ ] Panic wenn mailer nicht erreichbar -> Als workaround mailer deaktivierbar machen, bzw keine mails verschicken +* [x] Panic wenn mailer nicht erreichbar -> Als workaround mailer deaktivierbar machen, bzw keine mails verschicken * [x] "unexpected EOF" -* [ ] Beim Login & Password reset gibt die API zurück dass der Nutzer nicht existiert +* [x] Beim Login & Password reset gibt die API zurück dass der Nutzer nicht existiert ### Docs diff --git a/REST-Tests/users.http b/REST-Tests/users.http index 2f3ca6ff7..aa6be1287 100644 --- a/REST-Tests/users.http +++ b/REST-Tests/users.http @@ -27,7 +27,7 @@ Content-Type: application/json Accept: application/json { - "user_name": "user" + "email": "k@knt.li" } ### Request a token to reset a password diff --git a/config.yml.sample b/config.yml.sample index 3e5c79d33..fd9071918 100644 --- a/config.yml.sample +++ b/config.yml.sample @@ -54,6 +54,8 @@ redis: db: 0 mailer: + # Whether to enable the mailer or not. If it is disabled, all users are enabled right away and password reset is not possible. + enabled: false # SMTP Host host: "" # SMTP Host port diff --git a/docs/config.md b/docs/config.md index 71e12bc05..81760ca06 100644 --- a/docs/config.md +++ b/docs/config.md @@ -78,6 +78,8 @@ redis: db: 0 mailer: + # Whether to enable the mailer or not. If it is disabled, all users are enabled right away and password reset is not possible. + enabled: false # SMTP Host host: "" # SMTP Host port diff --git a/pkg/config/config.go b/pkg/config/config.go index 09a327efe..e1ab0ab9d 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -63,6 +63,7 @@ func init() { viper.SetDefault("cache.type", "memory") viper.SetDefault("cache.maxelementsize", 1000) // Mailer + viper.SetDefault("mailer.enabled", false) viper.SetDefault("mailer.host", "") viper.SetDefault("mailer.port", "587") viper.SetDefault("mailer.user", "user") diff --git a/pkg/mail/mail.go b/pkg/mail/mail.go index 17f7b54f2..6401957ce 100644 --- a/pkg/mail/mail.go +++ b/pkg/mail/mail.go @@ -31,6 +31,10 @@ var Queue chan *gomail.Message func StartMailDaemon() { Queue = make(chan *gomail.Message, viper.GetInt("mailer.queuelength")) + if !viper.GetBool("mailer.enabled") { + return + } + if viper.GetString("mailer.host") == "" { log.Log.Warning("Mailer seems to be not configured! Please see the config docs for more details.") return diff --git a/pkg/models/fixtures/users.yml b/pkg/models/fixtures/users.yml index 46be8c211..8dc3dcc1a 100644 --- a/pkg/models/fixtures/users.yml +++ b/pkg/models/fixtures/users.yml @@ -2,20 +2,27 @@ id: 1 username: 'user1' password: '1234' - email: 'johndoe@example.com' + email: 'user1@example.com' - id: 2 username: 'user2' password: '1234' - email: 'johndoe@example.com' + email: 'user2@example.com' - id: 3 username: 'user3' password: '1234' - email: 'johndoe@example.com' + email: 'user3@example.com' - id: 4 username: 'user4' password: '1234' - email: 'johndoe@example.com' - email_confirm_token: tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael \ No newline at end of file + email: 'user4@example.com' + email_confirm_token: tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael +- + id: 5 + username: 'user5' + password: '1234' + email: 'user4@example.com' + email_confirm_token: tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael + is_active: false \ No newline at end of file diff --git a/pkg/models/list_users_test.go b/pkg/models/list_users_test.go index f1cff13c6..748347aa6 100644 --- a/pkg/models/list_users_test.go +++ b/pkg/models/list_users_test.go @@ -160,7 +160,7 @@ func TestListUser_ReadAll(t *testing.T) { ID: 1, Username: "user1", Password: "1234", - Email: "johndoe@example.com", + Email: "user1@example.com", }, Right: UserRightRead, }, @@ -169,7 +169,7 @@ func TestListUser_ReadAll(t *testing.T) { ID: 2, Username: "user2", Password: "1234", - Email: "johndoe@example.com", + Email: "user2@example.com", }, Right: UserRightRead, }, diff --git a/pkg/models/namespace_users_test.go b/pkg/models/namespace_users_test.go index 16b659d88..c1d310174 100644 --- a/pkg/models/namespace_users_test.go +++ b/pkg/models/namespace_users_test.go @@ -161,7 +161,7 @@ func TestNamespaceUser_ReadAll(t *testing.T) { ID: 1, Username: "user1", Password: "1234", - Email: "johndoe@example.com", + Email: "user1@example.com", }, Right: UserRightRead, }, @@ -170,7 +170,7 @@ func TestNamespaceUser_ReadAll(t *testing.T) { ID: 2, Username: "user2", Password: "1234", - Email: "johndoe@example.com", + Email: "user2@example.com", }, Right: UserRightRead, }, diff --git a/pkg/models/unit_tests.go b/pkg/models/unit_tests.go index 2925e813c..de21a64fb 100644 --- a/pkg/models/unit_tests.go +++ b/pkg/models/unit_tests.go @@ -30,10 +30,6 @@ import ( "testing" ) -// IsTesting is set to true when we're running tests. -// We don't have a good solution to test email sending yet, so we disable email sending when testing -var IsTesting bool - // MainTest creates the test engine func MainTest(m *testing.M, pathToRoot string) { var err error @@ -42,8 +38,6 @@ func MainTest(m *testing.M, pathToRoot string) { log.Log.Fatalf("Error creating test engine: %v\n", err) } - IsTesting = true - // Start the pseudo mail queue mail.StartMailDaemon() diff --git a/pkg/models/user.go b/pkg/models/user.go index 493ef2618..7ae9cd253 100644 --- a/pkg/models/user.go +++ b/pkg/models/user.go @@ -78,9 +78,9 @@ func getUserWithError(a web.Auth) (*User, error) { // APIUserPassword represents a user object without timestamps and a json password field. type APIUserPassword struct { ID int64 `json:"id"` - Username string `json:"username"` - Password string `json:"password"` - Email string `json:"email"` + Username string `json:"username" valid:"length(3|250)"` + Password string `json:"password" valid:"length(8|250)"` + Email string `json:"email" valid:"email,length(0|250)"` } // APIFormat formats an API User into a normal user struct @@ -125,7 +125,9 @@ func CheckUserCredentials(u *UserLogin) (User, error) { // Check if the user exists user, err := GetUser(User{Username: u.Username}) if err != nil { - return User{}, err + // hashing the password takes a long time, so we hash something to not make it clear if the username was wrong + bcrypt.GenerateFromPassword([]byte(u.Username), 14) + return User{}, ErrWrongUsernameOrPassword{} } // User is invalid if it needs to verify its email address diff --git a/pkg/models/user_add_update.go b/pkg/models/user_add_update.go index 07996b1a5..0490b8035 100644 --- a/pkg/models/user_add_update.go +++ b/pkg/models/user_add_update.go @@ -20,6 +20,7 @@ import ( "code.vikunja.io/api/pkg/mail" "code.vikunja.io/api/pkg/metrics" "code.vikunja.io/api/pkg/utils" + "github.com/spf13/viper" "golang.org/x/crypto/bcrypt" ) @@ -67,11 +68,13 @@ func CreateUser(user User) (newUser User, err error) { return User{}, err } - // Generate a confirm token - newUser.EmailConfirmToken = utils.MakeRandomString(400) - - // The new user should not be activated until it confirms his mail address - newUser.IsActive = false + newUser.IsActive = true + if viper.GetBool("mailer.enabled") { + // The new user should not be activated until it confirms his mail address + newUser.IsActive = false + // Generate a confirm token + newUser.EmailConfirmToken = utils.MakeRandomString(400) + } // Insert it _, err = x.Insert(newUser) @@ -96,7 +99,7 @@ func CreateUser(user User) (newUser User, err error) { } // Dont send a mail if we're testing - if IsTesting { + if !viper.GetBool("mailer.enabled") { return newUserOut, err } diff --git a/pkg/models/user_password_reset.go b/pkg/models/user_password_reset.go index b480b4c13..9ab0e67c1 100644 --- a/pkg/models/user_password_reset.go +++ b/pkg/models/user_password_reset.go @@ -19,6 +19,7 @@ package models import ( "code.vikunja.io/api/pkg/mail" "code.vikunja.io/api/pkg/utils" + "github.com/spf13/viper" ) // PasswordReset holds the data to reset a password @@ -59,7 +60,7 @@ func UserPasswordReset(reset *PasswordReset) (err error) { } // Dont send a mail if we're testing - if IsTesting { + if !viper.GetBool("mailer.enabled") { return } @@ -75,13 +76,13 @@ func UserPasswordReset(reset *PasswordReset) (err error) { // PasswordTokenRequest defines the request format for password reset resqest type PasswordTokenRequest struct { - Username string `json:"user_name"` + Email string `json:"email" valid:"email,length(0|250)"` } // RequestUserPasswordResetToken inserts a random token to reset a users password into the databsse func RequestUserPasswordResetToken(tr *PasswordTokenRequest) (err error) { // Check if the user exists - user, err := GetUser(User{Username: tr.Username}) + user, err := GetUser(User{Email: tr.Email}) if err != nil { return } @@ -96,7 +97,7 @@ func RequestUserPasswordResetToken(tr *PasswordTokenRequest) (err error) { } // Dont send a mail if we're testing - if IsTesting { + if !viper.GetBool("mailer.enabled") { return } diff --git a/pkg/models/user_test.go b/pkg/models/user_test.go index 7feb9ad7b..730da45e8 100644 --- a/pkg/models/user_test.go +++ b/pkg/models/user_test.go @@ -78,7 +78,7 @@ func TestCreateUser(t *testing.T) { assert.True(t, IsErrUserDoesNotExist(err)) // Check the user credentials with an unverified email - user, err := CheckUserCredentials(&UserLogin{"testuu", "1234"}) + user, err := CheckUserCredentials(&UserLogin{"user5", "1234"}) assert.Error(t, err) assert.True(t, IsErrEmailNotConfirmed(err)) @@ -97,7 +97,7 @@ func TestCreateUser(t *testing.T) { // Check usercredentials for a nonexistent user (should fail) _, err = CheckUserCredentials(&UserLogin{"dfstestuu", "1234"}) assert.Error(t, err) - assert.True(t, IsErrUserDoesNotExist(err)) + assert.True(t, IsErrWrongUsernameOrPassword(err)) // Update the user uuser, err := UpdateUser(User{ID: theuser.ID, Password: "444444"}) @@ -146,7 +146,7 @@ func TestCreateUser(t *testing.T) { func TestUserPasswordReset(t *testing.T) { // Request a new token tr := &PasswordTokenRequest{ - Username: "user1", + Email: "user1@example.com", } err := RequestUserPasswordResetToken(tr) assert.NoError(t, err) diff --git a/pkg/routes/api/v1/user_password_reset.go b/pkg/routes/api/v1/user_password_reset.go index 7e7f199f4..aa44caa77 100644 --- a/pkg/routes/api/v1/user_password_reset.go +++ b/pkg/routes/api/v1/user_password_reset.go @@ -67,6 +67,10 @@ func UserRequestResetPasswordToken(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "No username provided.") } + if err := c.Validate(pwTokenReset); err != nil { + return echo.NewHTTPError(http.StatusBadRequest, err) + } + err := models.RequestUserPasswordResetToken(&pwTokenReset) if err != nil { return handler.HandleHTTPError(err, c) diff --git a/pkg/routes/routes.go b/pkg/routes/routes.go index 407e38d67..4b486150c 100644 --- a/pkg/routes/routes.go +++ b/pkg/routes/routes.go @@ -14,22 +14,6 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -// Vikunja is a todo-list application to facilitate your life. -// Copyright 2018 Vikunja and contributors. All rights reserved. -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License -// along with this program. If not, see . - // @title Vikunja API // @license.name GPLv3 // @BasePath /api/v1