diff --git a/Featurecreep.md b/Featurecreep.md index 9eca58d9..870e4b25 100644 --- a/Featurecreep.md +++ b/Featurecreep.md @@ -75,17 +75,18 @@ Sorry for some of them being in German, I'll tranlate them at some point. * [x] Panic wenn mailer nicht erreichbar -> Als workaround mailer deaktivierbar machen, bzw keine mails verschicken * [x] "unexpected EOF" * [x] Beim Login & Password reset gibt die API zurück dass der Nutzer nicht existiert -* [ ] Re-check rights checks to see if all information which is compared against is properly read from the db and not only based on user input - * [ ] Lists - * [ ] List users - * [ ] List Teams - * [ ] Labels - * [ ] Tasks - * [ ] Namespaces - * [ ] Namespace users - * [ ] Namespace teams - * [ ] Teams - * [ ] Team member handling +* [x] Re-check rights checks to see if all information which is compared against is properly read from the db and not only based on user input + * [x] Lists + * [x] List users + * [x] List Teams + * [x] Labels + * [x] Tasks + * [x] Namespaces + * [x] Namespace users + * [x] Namespace teams + * [x] Teams + * [x] Team member handling + * [x] Also check `ReadOne()` for unnessecary database operations since the inital query is already done in `CanRead()` * [x] Add a `User.AfterLoad()` which obfuscates the email address * [ ] Sometimes `done` from a task is not updated (returns `done: false` but `done:true` is being sent to the server) diff --git a/pkg/models/label_task.go b/pkg/models/label_task.go index c96f8957..c0df0396 100644 --- a/pkg/models/label_task.go +++ b/pkg/models/label_task.go @@ -109,11 +109,7 @@ func (lt *LabelTask) ReadAll(search string, a web.Auth, page int) (labels interf } // Check if the user has the right to see the task - task, err := GetListTaskByID(lt.TaskID) - if err != nil { - return nil, err - } - + task := ListTask{ID: lt.TaskID} canRead, err := task.CanRead(a) if err != nil { return nil, err diff --git a/pkg/models/list.go b/pkg/models/list.go index 4018b19e..5c13b311 100644 --- a/pkg/models/list.go +++ b/pkg/models/list.go @@ -113,11 +113,6 @@ func (l *List) ReadAll(search string, a web.Auth, page int) (interface{}, error) // @Failure 500 {object} models.Message "Internal error" // @Router /lists/{id} [get] func (l *List) ReadOne() (err error) { - err = l.GetSimpleByID() - if err != nil { - return err - } - // Get list tasks l.Tasks, err = GetTasksByListID(l.ID) if err != nil { diff --git a/pkg/models/list_create_test.go b/pkg/models/list_create_test.go index 09afac51..b3e24f08 100644 --- a/pkg/models/list_create_test.go +++ b/pkg/models/list_create_test.go @@ -46,6 +46,9 @@ func TestList_Create(t *testing.T) { // Get the list newdummy := List{ID: dummylist.ID} + canRead, err := newdummy.CanRead(&doer) + assert.NoError(t, err) + assert.True(t, canRead) err = newdummy.ReadOne() assert.NoError(t, err) assert.Equal(t, dummylist.Title, newdummy.Title) diff --git a/pkg/models/list_create_update.go b/pkg/models/list_create_update.go index ff803e41..bbdf6c24 100644 --- a/pkg/models/list_create_update.go +++ b/pkg/models/list_create_update.go @@ -48,6 +48,11 @@ func CreateOrUpdateList(list *List) (err error) { return } + err = list.GetSimpleByID() + if err != nil { + return + } + err = list.ReadOne() return diff --git a/pkg/models/list_tasks_rights.go b/pkg/models/list_tasks_rights.go index dce77d7a..cf00fb26 100644 --- a/pkg/models/list_tasks_rights.go +++ b/pkg/models/list_tasks_rights.go @@ -40,13 +40,15 @@ func (t *ListTask) CanCreate(a web.Auth) (bool, error) { } // CanRead determines if a user can read a task -func (t *ListTask) CanRead(a web.Auth) (bool, error) { +func (t *ListTask) CanRead(a web.Auth) (canRead bool, err error) { + // Get the task, error out if it doesn't exist + *t, err = getTaskByIDSimple(t.ID) + if err != nil { + return + } + // A user can read a task if it has access to the list list := &List{ID: t.ListID} - err := list.GetSimpleByID() - if err != nil { - return false, err - } return list.CanRead(a) } diff --git a/pkg/models/list_users_readall.go b/pkg/models/list_users_readall.go index e60ad357..7c2fe148 100644 --- a/pkg/models/list_users_readall.go +++ b/pkg/models/list_users_readall.go @@ -40,9 +40,6 @@ func (lu *ListUser) ReadAll(search string, a web.Auth, page int) (interface{}, e // Check if the user has access to the list l := &List{ID: lu.ListID} - if err := l.GetSimpleByID(); err != nil { - return nil, err - } canRead, err := l.CanRead(u) if err != nil { return nil, err diff --git a/pkg/models/namespace.go b/pkg/models/namespace.go index e420773c..f1923c42 100644 --- a/pkg/models/namespace.go +++ b/pkg/models/namespace.go @@ -18,6 +18,7 @@ package models import ( "code.vikunja.io/web" + "github.com/imdario/mergo" "time" ) @@ -69,13 +70,19 @@ func (n *Namespace) GetSimpleByID() (err error) { return } - exists, err := x.Get(n) + namespaceFromDB := &Namespace{} + exists, err := x.Where("id = ?", n.ID).Get(namespaceFromDB) if err != nil { return } if !exists { return ErrNamespaceDoesNotExist{ID: n.ID} } + // We don't want to override the provided user struct because this would break updating, so we have to merge it + if err := mergo.Merge(namespaceFromDB, n, mergo.WithOverride); err != nil { + return err + } + *n = *namespaceFromDB return } @@ -90,10 +97,6 @@ func GetNamespaceByID(id int64) (namespace Namespace, err error) { // Get the namespace Owner namespace.Owner, err = GetUserByID(namespace.OwnerID) - if err != nil { - return - } - return } @@ -110,7 +113,8 @@ func GetNamespaceByID(id int64) (namespace Namespace, err error) { // @Failure 500 {object} models.Message "Internal error" // @Router /namespaces/{id} [get] func (n *Namespace) ReadOne() (err error) { - *n, err = GetNamespaceByID(n.ID) + // Get the namespace Owner + n.Owner, err = GetUserByID(n.OwnerID) return } diff --git a/pkg/models/namespace_rights.go b/pkg/models/namespace_rights.go index 5eb1780b..6b1c3033 100644 --- a/pkg/models/namespace_rights.go +++ b/pkg/models/namespace_rights.go @@ -23,49 +23,17 @@ import ( // CanWrite checks if a user has write access to a namespace func (n *Namespace) CanWrite(a web.Auth) (bool, error) { - - // Get the namespace and check the right - originalNamespace := &Namespace{ID: n.ID} - err := originalNamespace.GetSimpleByID() - if err != nil { - return false, err - } - - u := getUserForRights(a) - if originalNamespace.isOwner(u) { - return true, nil - } - return originalNamespace.checkRight(u, RightWrite, RightAdmin) + return n.checkRight(a, RightWrite, RightAdmin) } // IsAdmin returns true or false if the user is admin on that namespace or not func (n *Namespace) IsAdmin(a web.Auth) (bool, error) { - originalNamespace := &Namespace{ID: n.ID} - err := originalNamespace.GetSimpleByID() - if err != nil { - return false, err - } - - u := getUserForRights(a) - if originalNamespace.isOwner(u) { - return true, nil - } - return originalNamespace.checkRight(u, RightAdmin) + return n.checkRight(a, RightAdmin) } // CanRead checks if a user has read access to that namespace func (n *Namespace) CanRead(a web.Auth) (bool, error) { - originalNamespace := &Namespace{ID: n.ID} - err := originalNamespace.GetSimpleByID() - if err != nil { - return false, err - } - - u := getUserForRights(a) - if originalNamespace.isOwner(u) { - return true, nil - } - return n.checkRight(u, RightRead, RightWrite, RightAdmin) + return n.checkRight(a, RightRead, RightWrite, RightAdmin) } // CanUpdate checks if the user can update the namespace @@ -84,12 +52,18 @@ func (n *Namespace) CanCreate(a web.Auth) (bool, error) { return true, nil } -// Small helper function to check if a user owns the namespace -func (n *Namespace) isOwner(user *User) bool { - return n.OwnerID == user.ID -} +func (n *Namespace) checkRight(a web.Auth, rights ...Right) (bool, error) { -func (n *Namespace) checkRight(user *User, rights ...Right) (bool, error) { + // Get the namespace and check the right + err := n.GetSimpleByID() + if err != nil { + return false, err + } + + user := getUserForRights(a) + if user.ID == n.OwnerID { + return true, nil + } /* The following loop creates an sql condition like this one: diff --git a/pkg/models/namespace_test.go b/pkg/models/namespace_test.go index 590d3f44..b3d0be6b 100644 --- a/pkg/models/namespace_test.go +++ b/pkg/models/namespace_test.go @@ -43,12 +43,12 @@ func TestNamespace_Create(t *testing.T) { assert.NoError(t, err) // check if it really exists - allowed, _ = dummynamespace.CanRead(&doer) - assert.True(t, allowed) - newOne := Namespace{ID: dummynamespace.ID} - err = newOne.ReadOne() + allowed, err = dummynamespace.CanRead(&doer) assert.NoError(t, err) - assert.Equal(t, newOne.Name, "Test") + assert.True(t, allowed) + err = dummynamespace.ReadOne() + assert.NoError(t, err) + assert.Equal(t, dummynamespace.Name, "Test") // Try creating one without a name n2 := Namespace{} @@ -64,12 +64,23 @@ func TestNamespace_Create(t *testing.T) { assert.True(t, IsErrUserDoesNotExist(err)) // Update it - allowed, _ = dummynamespace.CanUpdate(&doer) + allowed, err = dummynamespace.CanUpdate(&doer) + assert.NoError(t, err) assert.True(t, allowed) dummynamespace.Description = "Dolor sit amet." err = dummynamespace.Update() assert.NoError(t, err) + // Check if it was updated + assert.Equal(t, "Dolor sit amet.", dummynamespace.Description) + // Get it and check it again + allowed, err = dummynamespace.CanRead(&doer) + assert.NoError(t, err) + assert.True(t, allowed) + err = dummynamespace.ReadOne() + assert.NoError(t, err) + assert.Equal(t, "Dolor sit amet.", dummynamespace.Description) + // Try updating one with a nonexistant owner dummynamespace.Owner.ID = 94829838572 err = dummynamespace.Update() @@ -89,7 +100,8 @@ func TestNamespace_Create(t *testing.T) { assert.True(t, IsErrNamespaceDoesNotExist(err)) // Delete it - allowed, _ = dummynamespace.CanDelete(&doer) + allowed, err = dummynamespace.CanDelete(&doer) + assert.NoError(t, err) assert.True(t, allowed) err = dummynamespace.Delete() assert.NoError(t, err) @@ -100,7 +112,8 @@ func TestNamespace_Create(t *testing.T) { assert.True(t, IsErrNamespaceDoesNotExist(err)) // Check if it was successfully deleted - err = dummynamespace.ReadOne() + allowed, err = dummynamespace.CanRead(&doer) + assert.False(t, allowed) assert.Error(t, err) assert.True(t, IsErrNamespaceDoesNotExist(err)) diff --git a/pkg/models/namespace_users_readall.go b/pkg/models/namespace_users_readall.go index a7faa868..b008f8c4 100644 --- a/pkg/models/namespace_users_readall.go +++ b/pkg/models/namespace_users_readall.go @@ -39,10 +39,7 @@ func (nu *NamespaceUser) ReadAll(search string, a web.Auth, page int) (interface } // Check if the user has access to the namespace - l, err := GetNamespaceByID(nu.NamespaceID) - if err != nil { - return nil, err - } + l := Namespace{ID: nu.NamespaceID} canRead, err := l.CanRead(u) if err != nil { return nil, err diff --git a/pkg/models/team_list_readall.go b/pkg/models/team_list_readall.go index cc926269..56fc307f 100644 --- a/pkg/models/team_list_readall.go +++ b/pkg/models/team_list_readall.go @@ -40,9 +40,6 @@ func (tl *TeamList) ReadAll(search string, a web.Auth, page int) (interface{}, e // Check if the user can read the namespace l := &List{ID: tl.ListID} - if err := l.GetSimpleByID(); err != nil { - return nil, err - } canRead, err := l.CanRead(u) if err != nil { return nil, err diff --git a/pkg/models/team_namespace_readall.go b/pkg/models/team_namespace_readall.go index 16a4d7d3..8863550a 100644 --- a/pkg/models/team_namespace_readall.go +++ b/pkg/models/team_namespace_readall.go @@ -39,10 +39,7 @@ func (tn *TeamNamespace) ReadAll(search string, a web.Auth, page int) (interface } // Check if the user can read the namespace - n, err := GetNamespaceByID(tn.NamespaceID) - if err != nil { - return nil, err - } + n := Namespace{ID: tn.NamespaceID} canRead, err := n.CanRead(user) if err != nil { return nil, err diff --git a/pkg/models/teams_delete.go b/pkg/models/teams_delete.go index 0d3e401a..095dc3ca 100644 --- a/pkg/models/teams_delete.go +++ b/pkg/models/teams_delete.go @@ -34,12 +34,6 @@ import ( // @Router /teams/{id} [delete] func (t *Team) Delete() (err error) { - // Check if the team exists - _, err = GetTeamByID(t.ID) - if err != nil { - return - } - // Delete the team _, err = x.ID(t.ID).Delete(&Team{}) if err != nil { diff --git a/pkg/models/teams_rights.go b/pkg/models/teams_rights.go index 0d034da5..317b2f1f 100644 --- a/pkg/models/teams_rights.go +++ b/pkg/models/teams_rights.go @@ -28,13 +28,7 @@ func (t *Team) CanCreate(a web.Auth) (bool, error) { // CanUpdate checks if the user can update a team func (t *Team) CanUpdate(a web.Auth) (bool, error) { - u := getUserForRights(a) - - // Check if the current user is in the team and has admin rights in it - return x.Where("team_id = ?", t.ID). - And("user_id = ?", u.ID). - And("admin = ?", true). - Get(&TeamMember{}) + return t.IsAdmin(a) } // CanDelete checks if a user can delete a team @@ -46,6 +40,12 @@ func (t *Team) CanDelete(a web.Auth) (bool, error) { func (t *Team) IsAdmin(a web.Auth) (bool, error) { u := getUserForRights(a) + // Check if the team exists to be able to return a proper error message if not + _, err := GetTeamByID(t.ID) + if err != nil { + return false, err + } + return x.Where("team_id = ?", t.ID). And("user_id = ?", u.ID). And("admin = ?", true). diff --git a/pkg/models/teams_test.go b/pkg/models/teams_test.go index 6800df08..b5881756 100644 --- a/pkg/models/teams_test.go +++ b/pkg/models/teams_test.go @@ -80,13 +80,15 @@ func TestTeam_Create(t *testing.T) { assert.NoError(t, err) // Delete it - allowed, _ = dummyteam.CanDelete(&doer) + allowed, err = dummyteam.CanDelete(&doer) + assert.NoError(t, err) assert.True(t, allowed) err = dummyteam.Delete() assert.NoError(t, err) // Try deleting a (now) nonexistant team - err = dummyteam.Delete() + allowed, err = dummyteam.CanDelete(&doer) + assert.False(t, allowed) assert.Error(t, err) assert.True(t, IsErrTeamDoesNotExist(err)) diff --git a/pkg/routes/api/v1/list_by_namespace.go b/pkg/routes/api/v1/list_by_namespace.go index b0af5a08..18ec1542 100644 --- a/pkg/routes/api/v1/list_by_namespace.go +++ b/pkg/routes/api/v1/list_by_namespace.go @@ -80,12 +80,6 @@ func getNamespace(c echo.Context) (namespace models.Namespace, err error) { return } - // Get the namespace - namespace, err = models.GetNamespaceByID(namespaceID) - if err != nil { - return - } - // Check if the user has acces to that namespace user, err := models.GetCurrentUser(c) if err != nil { @@ -99,5 +93,11 @@ func getNamespace(c echo.Context) (namespace models.Namespace, err error) { return } + // Get the namespace + namespace, err = models.GetNamespaceByID(namespaceID) + if err != nil { + return + } + return }