From 4216ed7277f8d2a11a6367f5fb29351c348ece39 Mon Sep 17 00:00:00 2001 From: konrad Date: Tue, 13 Jul 2021 20:56:02 +0000 Subject: [PATCH] Refactor user email confirmation + password reset handling (#919) Co-authored-by: kolaente Reviewed-on: https://kolaente.dev/vikunja/api/pulls/919 Co-authored-by: konrad Co-committed-by: konrad --- pkg/cmd/user.go | 16 +++-- pkg/db/fixtures/user_tokens.yml | 18 +++++ pkg/db/fixtures/users.yml | 16 +---- pkg/db/test_fixtures.go | 6 +- pkg/initialize/init.go | 1 + pkg/integrations/integrations.go | 34 +++++----- pkg/migration/20210711173657.go | 101 ++++++++++++++++++++++++++++ pkg/migration/20210713213622.go | 71 ++++++++++++++++++++ pkg/models/label_test.go | 2 - pkg/models/list_users_test.go | 1 - pkg/models/namespace_users_test.go | 1 - pkg/models/task_collection_test.go | 2 - pkg/models/unit_tests.go | 1 + pkg/models/users_list_test.go | 16 +---- pkg/modules/auth/openid/openid.go | 2 +- pkg/user/db.go | 1 + pkg/user/notifications.go | 13 ++-- pkg/user/test.go | 2 +- pkg/user/token.go | 104 +++++++++++++++++++++++++++++ pkg/user/update_email.go | 30 ++++++--- pkg/user/user.go | 29 ++++++-- pkg/user/user_create.go | 31 +++++---- pkg/user/user_email_confirm.go | 31 +++++---- pkg/user/user_password_reset.go | 40 ++++++----- pkg/user/user_test.go | 1 - 25 files changed, 436 insertions(+), 134 deletions(-) create mode 100644 pkg/db/fixtures/user_tokens.yml create mode 100644 pkg/migration/20210711173657.go create mode 100644 pkg/migration/20210713213622.go create mode 100644 pkg/user/token.go diff --git a/pkg/cmd/user.go b/pkg/cmd/user.go index e7c9a295..2c2e9900 100644 --- a/pkg/cmd/user.go +++ b/pkg/cmd/user.go @@ -135,7 +135,7 @@ var userListCmd = &cobra.Command{ "ID", "Username", "Email", - "Active", + "Status", "Created", "Updated", }) @@ -145,7 +145,7 @@ var userListCmd = &cobra.Command{ strconv.FormatInt(u.ID, 10), u.Username, u.Email, - strconv.FormatBool(u.IsActive), + u.Status.String(), u.Created.Format(time.RFC3339), u.Updated.Format(time.RFC3339), }) @@ -277,11 +277,15 @@ var userChangeEnabledCmd = &cobra.Command{ u := getUserFromArg(s, args[0]) if userFlagEnableUser { - u.IsActive = true + u.Status = user.StatusActive } else if userFlagDisableUser { - u.IsActive = false + u.Status = user.StatusDisabled } else { - u.IsActive = !u.IsActive + if u.Status == user.StatusActive { + u.Status = user.StatusDisabled + } else { + u.Status = user.StatusActive + } } _, err := user.UpdateUser(s, u) if err != nil { @@ -293,6 +297,6 @@ var userChangeEnabledCmd = &cobra.Command{ log.Fatalf("Error saving everything: %s", err) } - fmt.Printf("User status successfully changed, user is now active: %t.\n", u.IsActive) + fmt.Printf("User status successfully changed, status is now \"%s\"\n", u.Status) }, } diff --git a/pkg/db/fixtures/user_tokens.yml b/pkg/db/fixtures/user_tokens.yml new file mode 100644 index 00000000..95080ba3 --- /dev/null +++ b/pkg/db/fixtures/user_tokens.yml @@ -0,0 +1,18 @@ +- + id: 1 + user_id: 3 + token: 'passwordresettesttoken' + kind: 1 + created: 2021-07-12 00:00:11 +- + id: 2 + user_id: 4 + token: 'tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael' + kind: 2 + created: 2021-07-12 00:00:12 +- + id: 3 + user_id: 5 + token: 'tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Aei' + kind: 2 + created: 2021-07-12 00:00:13 diff --git a/pkg/db/fixtures/users.yml b/pkg/db/fixtures/users.yml index 5f5153e5..7ad37281 100644 --- a/pkg/db/fixtures/users.yml +++ b/pkg/db/fixtures/users.yml @@ -3,7 +3,6 @@ username: 'user1' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user1@example.com' - is_active: true issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -20,7 +19,6 @@ username: 'user3' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user3@example.com' - password_reset_token: passwordresettesttoken issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -29,7 +27,7 @@ username: 'user4' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user4@example.com' - email_confirm_token: tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael + status: 1 issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -38,8 +36,7 @@ username: 'user5' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user5@example.com' - email_confirm_token: tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael - is_active: false + status: 1 issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -48,7 +45,6 @@ username: 'user6' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user6@example.com' - is_active: true issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -56,7 +52,6 @@ username: 'user7' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user7@example.com' - is_active: true issuer: local discoverable_by_email: true updated: 2018-12-02 15:13:12 @@ -65,7 +60,6 @@ username: 'user8' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user8@example.com' - is_active: true issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -73,7 +67,6 @@ username: 'user9' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user9@example.com' - is_active: true issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -81,7 +74,6 @@ username: 'user10' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user10@example.com' - is_active: true issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -90,7 +82,6 @@ name: 'Some one else' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user11@example.com' - is_active: true issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -99,7 +90,6 @@ name: 'Name with spaces' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user12@example.com' - is_active: true issuer: local discoverable_by_name: true updated: 2018-12-02 15:13:12 @@ -108,7 +98,6 @@ username: 'user13' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user14@example.com' - is_active: true issuer: local updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 @@ -116,7 +105,6 @@ username: 'user14' password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.' # 1234 email: 'user15@some.service.com' - is_active: true issuer: 'https://some.service.com' subject: '12345' updated: 2018-12-02 15:13:12 diff --git a/pkg/db/test_fixtures.go b/pkg/db/test_fixtures.go index c71f1ead..a2dede6d 100644 --- a/pkg/db/test_fixtures.go +++ b/pkg/db/test_fixtures.go @@ -59,10 +59,6 @@ func InitFixtures(tablenames ...string) (err error) { } fixtures, err = testfixtures.New(loaderOptions...) - if err != nil { - return err - } - return err } @@ -106,7 +102,7 @@ func LoadFixtures() error { } } } - return err + return nil } // LoadAndAssertFixtures loads all fixtures defined before and asserts they are correctly loaded diff --git a/pkg/initialize/init.go b/pkg/initialize/init.go index 32e067ac..d6ca0f4a 100644 --- a/pkg/initialize/init.go +++ b/pkg/initialize/init.go @@ -94,6 +94,7 @@ func FullInit() { cron.Init() models.RegisterReminderCron() models.RegisterOverdueReminderCron() + user.RegisterTokenCleanupCron() // Start processing events go func() { diff --git a/pkg/integrations/integrations.go b/pkg/integrations/integrations.go index f9c8a69e..45ed29ef 100644 --- a/pkg/integrations/integrations.go +++ b/pkg/integrations/integrations.go @@ -24,11 +24,11 @@ import ( "strings" "testing" - "code.vikunja.io/api/pkg/events" + "code.vikunja.io/api/pkg/files" "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/db" - "code.vikunja.io/api/pkg/files" + "code.vikunja.io/api/pkg/events" "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/modules/auth" "code.vikunja.io/api/pkg/routes" @@ -47,7 +47,6 @@ var ( Username: "user1", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", Email: "user1@example.com", - IsActive: true, } testuser2 = user.User{ ID: 2, @@ -56,26 +55,23 @@ var ( Email: "user2@example.com", } testuser3 = user.User{ - ID: 3, - Username: "user3", - Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - Email: "user3@example.com", - PasswordResetToken: "passwordresettesttoken", + ID: 3, + Username: "user3", + Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", + Email: "user3@example.com", } testuser4 = user.User{ - ID: 4, - Username: "user4", - Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - Email: "user4@example.com", - EmailConfirmToken: "tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael", + ID: 4, + Username: "user4", + Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", + Email: "user4@example.com", } testuser5 = user.User{ - ID: 4, - Username: "user5", - Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - Email: "user5@example.com", - EmailConfirmToken: "tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael", - IsActive: false, + ID: 4, + Username: "user5", + Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", + Email: "user5@example.com", + Status: user.StatusDisabled, } ) diff --git a/pkg/migration/20210711173657.go b/pkg/migration/20210711173657.go new file mode 100644 index 00000000..15e04b79 --- /dev/null +++ b/pkg/migration/20210711173657.go @@ -0,0 +1,101 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-2021 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 Affero General Public Licensee 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 Affero General Public Licensee for more details. +// +// You should have received a copy of the GNU Affero General Public Licensee +// along with this program. If not, see . + +package migration + +import ( + "time" + + "src.techknowlogick.com/xormigrate" + "xorm.io/xorm" +) + +type user20210711173657 struct { + ID int64 `xorm:"bigint autoincr not null unique pk" json:"id"` + PasswordResetToken string `xorm:"varchar(450) null" json:"-"` + EmailConfirmToken string `xorm:"varchar(450) null" json:"-"` +} + +func (u user20210711173657) TableName() string { + return "users" +} + +type userTokens20210711173657 struct { + ID int64 `xorm:"bigint autoincr not null unique pk"` + UserID int64 `xorm:"not null"` + Token string `xorm:"not null"` + Kind int `xorm:"not null"` + Created time.Time `xorm:"created not null"` +} + +func (userTokens20210711173657) TableName() string { + return "user_tokens" +} + +func init() { + migrations = append(migrations, &xormigrate.Migration{ + ID: "20210711173657", + Description: "Add user tokens table", + Migrate: func(tx *xorm.Engine) error { + err := tx.Sync2(userTokens20210711173657{}) + if err != nil { + return err + } + + users := []*user20210711173657{} + err = tx.Where(`password_reset_token != '' OR email_confirm_token != ''`).Find(&users) + if err != nil { + return err + } + + const tokenPasswordReset = 1 + const tokenEmailConfirm = 2 + + for _, user := range users { + if user.PasswordResetToken != "" { + _, err = tx.Insert(&userTokens20210711173657{ + UserID: user.ID, + Token: user.PasswordResetToken, + Kind: tokenPasswordReset, + }) + if err != nil { + return err + } + } + + if user.EmailConfirmToken != "" { + _, err = tx.Insert(&userTokens20210711173657{ + UserID: user.ID, + Token: user.EmailConfirmToken, + Kind: tokenEmailConfirm, + }) + if err != nil { + return err + } + } + } + + err = dropTableColum(tx, "users", "password_reset_token") + if err != nil { + return err + } + return dropTableColum(tx, "users", "email_confirm_token") + }, + Rollback: func(tx *xorm.Engine) error { + return nil + }, + }) +} diff --git a/pkg/migration/20210713213622.go b/pkg/migration/20210713213622.go new file mode 100644 index 00000000..30ac6ebe --- /dev/null +++ b/pkg/migration/20210713213622.go @@ -0,0 +1,71 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-2021 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 Affero General Public Licensee 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 Affero General Public Licensee for more details. +// +// You should have received a copy of the GNU Affero General Public Licensee +// along with this program. If not, see . + +package migration + +import ( + "src.techknowlogick.com/xormigrate" + "xorm.io/xorm" +) + +type users20210713213622 struct { + ID int64 `xorm:"bigint autoincr not null unique pk" json:"id"` + IsActive bool `xorm:"null" json:"-"` + Status int `xorm:"default 0" json:"-"` +} + +func (users20210713213622) TableName() string { + return "users" +} + +func init() { + migrations = append(migrations, &xormigrate.Migration{ + ID: "20210713213622", + Description: "Add users status instead of is_active", + Migrate: func(tx *xorm.Engine) error { + err := tx.Sync2(users20210713213622{}) + if err != nil { + return err + } + + users := []*users20210713213622{} + err = tx.Find(&users) + if err != nil { + return err + } + + for _, user := range users { + if user.IsActive { + continue + } + + user.Status = 1 // 1 is "email confirmation required" - as that's the only way is_active was used before we'll use that + _, err := tx. + Where("id = ?", user.ID). + Cols("status"). + Update(user) + if err != nil { + return err + } + } + + return dropTableColum(tx, "users", "is_active") + }, + Rollback: func(tx *xorm.Engine) error { + return nil + }, + }) +} diff --git a/pkg/models/label_test.go b/pkg/models/label_test.go index 9666695c..52151d01 100644 --- a/pkg/models/label_test.go +++ b/pkg/models/label_test.go @@ -51,7 +51,6 @@ func TestLabel_ReadAll(t *testing.T) { ID: 1, Username: "user1", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -166,7 +165,6 @@ func TestLabel_ReadOne(t *testing.T) { ID: 1, Username: "user1", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, diff --git a/pkg/models/list_users_test.go b/pkg/models/list_users_test.go index f36858b2..f8c3968a 100644 --- a/pkg/models/list_users_test.go +++ b/pkg/models/list_users_test.go @@ -180,7 +180,6 @@ func TestListUser_ReadAll(t *testing.T) { ID: 1, Username: "user1", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, diff --git a/pkg/models/namespace_users_test.go b/pkg/models/namespace_users_test.go index f9fe51d9..6f75140a 100644 --- a/pkg/models/namespace_users_test.go +++ b/pkg/models/namespace_users_test.go @@ -179,7 +179,6 @@ func TestNamespaceUser_ReadAll(t *testing.T) { ID: 1, Username: "user1", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, diff --git a/pkg/models/task_collection_test.go b/pkg/models/task_collection_test.go index 14e98606..28b70dbd 100644 --- a/pkg/models/task_collection_test.go +++ b/pkg/models/task_collection_test.go @@ -34,7 +34,6 @@ func TestTaskCollection_ReadAll(t *testing.T) { ID: 1, Username: "user1", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -56,7 +55,6 @@ func TestTaskCollection_ReadAll(t *testing.T) { Username: "user6", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", Issuer: "local", - IsActive: true, EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, Created: testCreatedTime, diff --git a/pkg/models/unit_tests.go b/pkg/models/unit_tests.go index d3a0e7b2..4641f44a 100644 --- a/pkg/models/unit_tests.go +++ b/pkg/models/unit_tests.go @@ -55,6 +55,7 @@ func SetupTests() { "team_namespaces", "teams", "users", + "user_tokens", "users_lists", "users_namespaces", "buckets", diff --git a/pkg/models/users_list_test.go b/pkg/models/users_list_test.go index dc4c17ea..cf0ed846 100644 --- a/pkg/models/users_list_test.go +++ b/pkg/models/users_list_test.go @@ -29,7 +29,6 @@ func TestListUsersFromList(t *testing.T) { ID: 1, Username: "user1", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -50,7 +49,6 @@ func TestListUsersFromList(t *testing.T) { ID: 3, Username: "user3", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - PasswordResetToken: "passwordresettesttoken", Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -61,8 +59,7 @@ func TestListUsersFromList(t *testing.T) { ID: 4, Username: "user4", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: false, - EmailConfirmToken: "tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael", + Status: user.StatusEmailConfirmationRequired, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -73,8 +70,7 @@ func TestListUsersFromList(t *testing.T) { ID: 5, Username: "user5", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: false, - EmailConfirmToken: "tiepiQueed8ahc7zeeFe1eveiy4Ein8osooxegiephauph2Ael", + Status: user.StatusEmailConfirmationRequired, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -85,7 +81,6 @@ func TestListUsersFromList(t *testing.T) { ID: 6, Username: "user6", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -96,7 +91,6 @@ func TestListUsersFromList(t *testing.T) { ID: 7, Username: "user7", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, DiscoverableByEmail: true, @@ -108,7 +102,6 @@ func TestListUsersFromList(t *testing.T) { ID: 8, Username: "user8", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -119,7 +112,6 @@ func TestListUsersFromList(t *testing.T) { ID: 9, Username: "user9", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -130,7 +122,6 @@ func TestListUsersFromList(t *testing.T) { ID: 10, Username: "user10", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -142,7 +133,6 @@ func TestListUsersFromList(t *testing.T) { Username: "user11", Name: "Some one else", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, @@ -154,7 +144,6 @@ func TestListUsersFromList(t *testing.T) { Username: "user12", Name: "Name with spaces", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, DiscoverableByName: true, @@ -166,7 +155,6 @@ func TestListUsersFromList(t *testing.T) { ID: 13, Username: "user13", Password: "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.", - IsActive: true, Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, diff --git a/pkg/modules/auth/openid/openid.go b/pkg/modules/auth/openid/openid.go index a86fd88c..3268c664 100644 --- a/pkg/modules/auth/openid/openid.go +++ b/pkg/modules/auth/openid/openid.go @@ -216,7 +216,7 @@ func getOrCreateUser(s *xorm.Session, cl *claims, issuer, subject string) (u *us uu := &user.User{ Username: cl.PreferredUsername, Email: cl.Email, - IsActive: true, + Status: user.StatusActive, Issuer: issuer, Subject: subject, } diff --git a/pkg/user/db.go b/pkg/user/db.go index d68ec61e..25fbc03d 100644 --- a/pkg/user/db.go +++ b/pkg/user/db.go @@ -36,5 +36,6 @@ func GetTables() []interface{} { return []interface{}{ &User{}, &TOTP{}, + &Token{}, } } diff --git a/pkg/user/notifications.go b/pkg/user/notifications.go index 2fa527ce..d900825e 100644 --- a/pkg/user/notifications.go +++ b/pkg/user/notifications.go @@ -23,8 +23,9 @@ import ( // EmailConfirmNotification represents a EmailConfirmNotification notification type EmailConfirmNotification struct { - User *User - IsNew bool + User *User + IsNew bool + ConfirmToken string } // ToMail returns the mail notification for EmailConfirmNotification @@ -45,7 +46,7 @@ func (n *EmailConfirmNotification) ToMail() *notifications.Mail { return nn. Line("To confirm your email address, click the link below:"). - Action("Confirm your email address", config.ServiceFrontendurl.GetString()+"?userEmailConfirm="+n.User.EmailConfirmToken). + Action("Confirm your email address", config.ServiceFrontendurl.GetString()+"?userEmailConfirm="+n.ConfirmToken). Line("Have a nice day!") } @@ -85,7 +86,8 @@ func (n *PasswordChangedNotification) Name() string { // ResetPasswordNotification represents a ResetPasswordNotification notification type ResetPasswordNotification struct { - User *User + User *User + Token *Token } // ToMail returns the mail notification for ResetPasswordNotification @@ -94,7 +96,8 @@ func (n *ResetPasswordNotification) ToMail() *notifications.Mail { Subject("Reset your password on Vikunja"). Greeting("Hi "+n.User.GetName()+","). Line("To reset your password, click the link below:"). - Action("Reset your password", config.ServiceFrontendurl.GetString()+"?userPasswordReset="+n.User.PasswordResetToken). + Action("Reset your password", config.ServiceFrontendurl.GetString()+"?userPasswordReset="+n.Token.Token). + Line("This link will be valid for 24 hours."). Line("Have a nice day!") } diff --git a/pkg/user/test.go b/pkg/user/test.go index 86efd6d5..5644cca4 100644 --- a/pkg/user/test.go +++ b/pkg/user/test.go @@ -34,7 +34,7 @@ func InitTests() { log.Fatal(err) } - err = db.InitTestFixtures("users") + err = db.InitTestFixtures("users", "user_tokens") if err != nil { log.Fatal(err) } diff --git a/pkg/user/token.go b/pkg/user/token.go new file mode 100644 index 00000000..8ffd8dc1 --- /dev/null +++ b/pkg/user/token.go @@ -0,0 +1,104 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-2021 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 Affero General Public Licensee 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 Affero General Public Licensee for more details. +// +// You should have received a copy of the GNU Affero General Public Licensee +// along with this program. If not, see . + +package user + +import ( + "time" + + "code.vikunja.io/api/pkg/cron" + "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/log" + "code.vikunja.io/api/pkg/utils" + "xorm.io/xorm" +) + +// TokenKind represents a user token kind +type TokenKind int + +const ( + TokenUnknown TokenKind = iota + TokenPasswordReset + TokenEmailConfirm + + tokenSize = 64 +) + +// Token is a token a user can use to do things like verify their email or resetting their password +type Token struct { + ID int64 `xorm:"bigint autoincr not null unique pk"` + UserID int64 `xorm:"not null"` + Token string `xorm:"not null"` + Kind TokenKind `xorm:"not null"` + Created time.Time `xorm:"created not null"` +} + +// TableName returns the real table name for user tokens +func (t *Token) TableName() string { + return "user_tokens" +} + +func generateNewToken(s *xorm.Session, u *User, kind TokenKind) (token *Token, err error) { + token = &Token{ + UserID: u.ID, + Kind: kind, + Token: utils.MakeRandomString(tokenSize), + } + + _, err = s.Insert(token) + return +} + +func getToken(s *xorm.Session, token string, kind TokenKind) (t *Token, err error) { + t = &Token{} + has, err := s.Where("kind = ? AND token = ?", kind, token). + Get(t) + if err != nil || !has { + return nil, err + } + + return +} + +func removeTokens(s *xorm.Session, u *User, kind TokenKind) (err error) { + _, err = s.Where("user_id = ? AND kind = ?", u.ID, kind). + Delete(&Token{}) + return +} + +// RegisterTokenCleanupCron registers a cron function to clean up all password reset tokens older than 24 hours +func RegisterTokenCleanupCron() { + const logPrefix = "[User Token Cleanup Cron] " + + err := cron.Schedule("0 * * * *", func() { + s := db.NewSession() + defer s.Close() + + deleted, err := s. + Where("created > ? AND kind = ?", time.Now().Add(time.Hour*24*-1), TokenPasswordReset). + Delete(&Token{}) + if err != nil { + log.Errorf(logPrefix+"Error removing old password reset tokens: %s", err) + return + } + if deleted > 0 { + log.Debugf(logPrefix+"Deleted %d old password reset tokens", deleted) + } + }) + if err != nil { + log.Fatalf("Could not register token cleanup cron: %s", err) + } +} diff --git a/pkg/user/update_email.go b/pkg/user/update_email.go index 2e3b9cee..6b4b35d9 100644 --- a/pkg/user/update_email.go +++ b/pkg/user/update_email.go @@ -19,7 +19,6 @@ package user import ( "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/notifications" - "code.vikunja.io/api/pkg/utils" "xorm.io/xorm" ) @@ -52,26 +51,35 @@ func UpdateEmail(s *xorm.Session, update *EmailUpdate) (err error) { return } - update.User.IsActive = false update.User.Email = update.NewEmail - update.User.EmailConfirmToken = utils.MakeRandomString(64) + + // Send the confirmation mail + if !config.MailerEnabled.GetBool() { + _, err = s. + Where("id = ?", update.User.ID). + Cols("email"). + Update(update.User) + return + } + + update.User.Status = StatusEmailConfirmationRequired + token, err := generateNewToken(s, update.User, TokenEmailConfirm) + if err != nil { + return + } _, err = s. Where("id = ?", update.User.ID). - Cols("email", "is_active", "email_confirm_token"). + Cols("email", "is_active"). // TODO: Status change Update(update.User) if err != nil { return } - // Send the confirmation mail - if !config.MailerEnabled.GetBool() { - return - } - // Send the user a mail with a link to confirm the mail n := &EmailConfirmNotification{ - User: update.User, - IsNew: false, + User: update.User, + IsNew: false, + ConfirmToken: token.Token, } err = notifications.Notify(update.User, n) diff --git a/pkg/user/user.go b/pkg/user/user.go index 796de8c9..87c1d360 100644 --- a/pkg/user/user.go +++ b/pkg/user/user.go @@ -44,6 +44,27 @@ type Login struct { TOTPPasscode string `json:"totp_passcode"` } +type Status int + +func (s Status) String() string { + switch s { + case StatusActive: + return "Active" + case StatusEmailConfirmationRequired: + return "Email Confirmation required" + case StatusDisabled: + return "Disabled" + } + + return "Unknown" +} + +const ( + StatusActive = iota + StatusEmailConfirmationRequired + StatusDisabled +) + // User holds information about an user type User struct { // The unique, numeric id of this user. @@ -54,11 +75,9 @@ type User struct { Username string `xorm:"varchar(250) not null unique" json:"username" valid:"length(1|250)" minLength:"1" maxLength:"250"` Password string `xorm:"varchar(250) null" json:"-"` // The user's email address. - Email string `xorm:"varchar(250) null" json:"email,omitempty" valid:"email,length(0|250)" maxLength:"250"` - IsActive bool `xorm:"null" json:"-"` + Email string `xorm:"varchar(250) null" json:"email,omitempty" valid:"email,length(0|250)" maxLength:"250"` - PasswordResetToken string `xorm:"varchar(450) null" json:"-"` - EmailConfirmToken string `xorm:"varchar(450) null" json:"-"` + Status Status `xorm:"default 0" json:"-"` AvatarProvider string `xorm:"varchar(255) null" json:"-"` AvatarFileID int64 `xorm:"null" json:"-"` @@ -255,7 +274,7 @@ func CheckUserCredentials(s *xorm.Session, u *Login) (*User, error) { } // The user is invalid if they need to verify their email address - if !user.IsActive { + if user.Status == StatusEmailConfirmationRequired { return &User{}, ErrEmailNotConfirmed{UserID: user.ID} } diff --git a/pkg/user/user_create.go b/pkg/user/user_create.go index d9ac0352..f445eb88 100644 --- a/pkg/user/user_create.go +++ b/pkg/user/user_create.go @@ -20,7 +20,6 @@ import ( "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/events" "code.vikunja.io/api/pkg/notifications" - "code.vikunja.io/api/pkg/utils" "golang.org/x/crypto/bcrypt" "xorm.io/xorm" ) @@ -54,14 +53,7 @@ func CreateUser(s *xorm.Session, user *User) (newUser *User, err error) { } } - user.IsActive = true - if config.MailerEnabled.GetBool() && user.Issuer == issuerLocal { - // The new user should not be activated until it confirms his mail address - user.IsActive = false - // Generate a confirm token - user.EmailConfirmToken = utils.MakeRandomString(60) - } - + user.Status = StatusActive user.AvatarProvider = "initials" // Insert it @@ -84,13 +76,28 @@ func CreateUser(s *xorm.Session, user *User) (newUser *User, err error) { } // Dont send a mail if no mailer is configured - if !config.MailerEnabled.GetBool() { + if !config.MailerEnabled.GetBool() || user.Issuer != issuerLocal { return newUserOut, err } + user.Status = StatusEmailConfirmationRequired + token, err := generateNewToken(s, user, TokenEmailConfirm) + if err != nil { + return nil, err + } + + _, err = s. + Where("id = ?", user.ID). + Cols("email", "is_active"). + Update(user) + if err != nil { + return + } + n := &EmailConfirmNotification{ - User: user, - IsNew: false, + User: user, + IsNew: true, + ConfirmToken: token.Token, } err = notifications.Notify(user, n) diff --git a/pkg/user/user_email_confirm.go b/pkg/user/user_email_confirm.go index 4aafce8b..374e5bef 100644 --- a/pkg/user/user_email_confirm.go +++ b/pkg/user/user_email_confirm.go @@ -16,7 +16,9 @@ package user -import "xorm.io/xorm" +import ( + "xorm.io/xorm" +) // EmailConfirm holds the token to confirm a mail address type EmailConfirm struct { @@ -32,24 +34,27 @@ func ConfirmEmail(s *xorm.Session, c *EmailConfirm) (err error) { return ErrInvalidEmailConfirmToken{} } - // Check if the token is valid - user := User{} - has, err := s. - Where("email_confirm_token = ?", c.Token). - Get(&user) + token, err := getToken(s, c.Token, TokenEmailConfirm) + if err != nil { + return + } + if token == nil { + return ErrInvalidEmailConfirmToken{Token: c.Token} + } + + user, err := GetUserByID(s, token.UserID) if err != nil { return } - if !has { - return ErrInvalidEmailConfirmToken{Token: c.Token} + user.Status = StatusActive + err = removeTokens(s, user, TokenEmailConfirm) + if err != nil { + return } - - user.IsActive = true - user.EmailConfirmToken = "" _, err = s. Where("id = ?", user.ID). - Cols("is_active", "email_confirm_token"). - Update(&user) + Cols("is_active"). + Update(user) return } diff --git a/pkg/user/user_password_reset.go b/pkg/user/user_password_reset.go index debdc283..b03a7807 100644 --- a/pkg/user/user_password_reset.go +++ b/pkg/user/user_password_reset.go @@ -19,7 +19,6 @@ package user import ( "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/notifications" - "code.vikunja.io/api/pkg/utils" "xorm.io/xorm" ) @@ -44,16 +43,17 @@ func ResetPassword(s *xorm.Session, reset *PasswordReset) (err error) { } // Check if we have a token - user := &User{} - exists, err := s. - Where("password_reset_token = ?", reset.Token). - Get(user) + token, err := getToken(s, reset.Token, TokenPasswordReset) if err != nil { - return + return err + } + if token == nil { + return ErrInvalidPasswordResetToken{Token: reset.Token} } - if !exists { - return ErrInvalidPasswordResetToken{Token: reset.Token} + user, err := GetUserByID(s, token.UserID) + if err != nil { + return } // Hash the password @@ -62,17 +62,20 @@ func ResetPassword(s *xorm.Session, reset *PasswordReset) (err error) { return } - // Save it - user.PasswordResetToken = "" + err = removeTokens(s, user, TokenEmailConfirm) + if err != nil { + return + } + _, err = s. - Cols("password", "password_reset_token"). + Cols("password"). Where("id = ?", user.ID). Update(user) if err != nil { return } - // Dont send a mail if we're testing + // Dont send a mail if no mailer is configured if !config.MailerEnabled.GetBool() { return } @@ -108,24 +111,19 @@ func RequestUserPasswordResetTokenByEmail(s *xorm.Session, tr *PasswordTokenRequ // RequestUserPasswordResetToken sends a user a password reset email. func RequestUserPasswordResetToken(s *xorm.Session, user *User) (err error) { - // Generate a token and save it - user.PasswordResetToken = utils.MakeRandomString(400) - - // Save it - _, err = s. - Where("id = ?", user.ID). - Update(user) + token, err := generateNewToken(s, user, TokenPasswordReset) if err != nil { return } - // Dont send a mail if we're testing + // Dont send a mail if no mailer is configured if !config.MailerEnabled.GetBool() { return } n := &ResetPasswordNotification{ - User: user, + User: user, + Token: token, } err = notifications.Notify(user, n) diff --git a/pkg/user/user_test.go b/pkg/user/user_test.go index 495870a5..f36f276e 100644 --- a/pkg/user/user_test.go +++ b/pkg/user/user_test.go @@ -322,7 +322,6 @@ func TestUpdateUser(t *testing.T) { } func TestUpdateUserPassword(t *testing.T) { - t.Run("normal", func(t *testing.T) { db.LoadAndAssertFixtures(t) s := db.NewSession()