Fix rights checks (#70)

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
+* [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
-  * [ ] Lists
+  * [x] Lists
-  * [ ] List users
+  * [x] List users
-  * [ ] List Teams
+  * [x] List Teams
-  * [ ] Labels
+  * [x] Labels
-  * [ ] Tasks
+  * [x] Tasks
-  * [ ] Namespaces
+  * [x] Namespaces
-  * [ ] Namespace users
+  * [x] Namespace users
-  * [ ] Namespace teams
+  * [x] Namespace teams
-  * [ ] Teams
+  * [x] Teams
-  * [ ] Team member handling
+  * [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)
 

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)
+	task := ListTask{ID: lt.TaskID}
-	if err != nil {
-		return nil, err
-	}
-
 	canRead, err := task.CanRead(a)
 	if err != nil {
 		return nil, err

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 {

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)

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
 

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)
 }
 

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

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
 }
 

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) {
-
+	return n.checkRight(a, RightWrite, RightAdmin)
-	// 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)
 }
 
 // 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}
+	return n.checkRight(a, RightAdmin)
-	err := originalNamespace.GetSimpleByID()
-	if err != nil {
-		return false, err
-	}
-
-	u := getUserForRights(a)
-	if originalNamespace.isOwner(u) {
-		return true, nil
-	}
-	return originalNamespace.checkRight(u, 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}
+	return n.checkRight(a, RightRead, RightWrite, RightAdmin)
-	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)
 }
 
 // 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) checkRight(a web.Auth, rights ...Right) (bool, error) {
-func (n *Namespace) isOwner(user *User) bool {
-	return n.OwnerID == user.ID
-}
 
-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:

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)
+	allowed, err = dummynamespace.CanRead(&doer)
-	assert.True(t, allowed)
-	newOne := Namespace{ID: dummynamespace.ID}
-	err = newOne.ReadOne()
 	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))
 

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)
+	l := Namespace{ID: nu.NamespaceID}
-	if err != nil {
-		return nil, err
-	}
 	canRead, err := l.CanRead(u)
 	if err != nil {
 		return nil, err

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

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)
+	n := Namespace{ID: tn.NamespaceID}
-	if err != nil {
-		return nil, err
-	}
 	canRead, err := n.CanRead(user)
 	if err != nil {
 		return nil, err

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 {

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)
+	return t.IsAdmin(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{})
 }
 
 // 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).

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))
 

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
 }