Fixed rights check on lists and namespaces (#62)

pkg/models/bulk_list_task.go

@@ -61,11 +61,9 @@ func (bt *BulkTask) CanUpdate(a web.Auth) bool {
 		return false
 	}
 
-	doer := getUserForRights(a)
 	// A user can update an task if he has write acces to its list
 	l := &List{ID: bt.Tasks[0].ListID}
-	l.ReadOne()
+	return l.CanWrite(a)
-	return l.CanWrite(doer)
 }
 
 // Update updates a bunch of tasks at once

pkg/models/list_rights.go

@@ -24,11 +24,19 @@ import (
 
 // CanWrite return whether the user can write on that list or not
 func (l *List) CanWrite(a web.Auth) bool {
-	user := getUserForRights(a)
 
+	// Get the list and check the right
+	originalList := &List{ID: l.ID}
+	err := originalList.GetSimpleByID()
+	if err != nil {
+		log.Log.Error("Error occurred during CanWrite for List: %s", err)
+		return false
+	}
+
+	user := getUserForRights(a)
 	// Check all the things
 	// Check if the user is either owner or can write to the list
-	return l.isOwner(user) || l.checkRight(user, RightWrite, RightAdmin)
+	return originalList.isOwner(user) || originalList.checkRight(user, RightWrite, RightAdmin)
 }
 
 // CanRead checks if a user has read access to a list
@@ -37,6 +45,8 @@ func (l *List) CanRead(a web.Auth) bool {
 
 	// Check all the things
 	// Check if the user is either owner or can read
+	// We can do this without first looking up the list because CanRead() is called after ReadOne()
+	// So are sure the list exists
 	return l.isOwner(user) || l.checkRight(user, RightRead, RightWrite, RightAdmin)
 }
 
@@ -53,7 +63,7 @@ func (l *List) CanDelete(a web.Auth) bool {
 // CanCreate checks if the user can update a list
 func (l *List) CanCreate(a web.Auth) bool {
 	// A user can create a list if he has write access to the namespace
-	n, _ := GetNamespaceByID(l.NamespaceID)
+	n := &Namespace{ID: l.NamespaceID}
 	return n.CanWrite(a)
 }
 
@@ -61,10 +71,17 @@ func (l *List) CanCreate(a web.Auth) bool {
 func (l *List) IsAdmin(a web.Auth) bool {
 	user := getUserForRights(a)
 
+	originalList := &List{ID: l.ID}
+	err := originalList.GetSimpleByID()
+	if err != nil {
+		log.Log.Error("Error occurred during IsAdmin for List: %s", err)
+		return false
+	}
+
 	// Check all the things
 	// Check if the user is either owner or can write to the list
 	// Owners are always admins
-	return l.isOwner(user) || l.checkRight(user, RightAdmin)
+	return originalList.isOwner(user) || originalList.checkRight(user, RightAdmin)
 }
 
 // Little helper function to check if a user is list owner

pkg/models/list_tasks_rights.go

@@ -37,11 +37,6 @@ func (t *ListTask) CanCreate(a web.Auth) bool {
 
 	// A user can do a task if he has write acces to its list
 	l := &List{ID: t.ListID}
-	err := l.GetSimpleByID()
-	if err != nil {
-		log.Log.Error("Error occurred during CanDelete for ListTask: %s", err)
-		return false
-	}
 	return l.CanWrite(doer)
 }
 
@@ -70,10 +65,5 @@ func (t *ListTask) canDoListTask(a web.Auth) bool {
 
 	// A user can do a task if he has write acces to its list
 	l := &List{ID: lI.ListID}
-	err = l.GetSimpleByID()
-	if err != nil {
-		log.Log.Error("Error occurred during CanDelete for ListTask: %s", err)
-		return false
-	}
 	return l.CanWrite(doer)
 }

pkg/models/list_users_create.go

@@ -33,40 +33,40 @@ import "code.vikunja.io/web"
 // @Failure 403 {object} code.vikunja.io/web.HTTPError "The user does not have access to the list"
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /lists/{id}/users [put]
-func (ul *ListUser) Create(a web.Auth) (err error) {
+func (lu *ListUser) Create(a web.Auth) (err error) {
 
 	// Check if the right is valid
-	if err := ul.Right.isValid(); err != nil {
+	if err := lu.Right.isValid(); err != nil {
 		return err
 	}
 
 	// Check if the list exists
-	l := &List{ID: ul.ListID}
+	l := &List{ID: lu.ListID}
 	if err = l.GetSimpleByID(); err != nil {
 		return
 	}
 
 	// Check if the user exists
-	if _, err = GetUserByID(ul.UserID); err != nil {
+	if _, err = GetUserByID(lu.UserID); err != nil {
 		return err
 	}
 
 	// Check if the user already has access or is owner of that list
 	// We explicitly DONT check for teams here
-	if l.OwnerID == ul.UserID {
+	if l.OwnerID == lu.UserID {
-		return ErrUserAlreadyHasAccess{UserID: ul.UserID, ListID: ul.ListID}
+		return ErrUserAlreadyHasAccess{UserID: lu.UserID, ListID: lu.ListID}
 	}
 
-	exist, err := x.Where("list_id = ? AND user_id = ?", ul.ListID, ul.UserID).Get(&ListUser{})
+	exist, err := x.Where("list_id = ? AND user_id = ?", lu.ListID, lu.UserID).Get(&ListUser{})
 	if err != nil {
 		return
 	}
 	if exist {
-		return ErrUserAlreadyHasAccess{UserID: ul.UserID, ListID: ul.ListID}
+		return ErrUserAlreadyHasAccess{UserID: lu.UserID, ListID: lu.ListID}
 	}
 
 	// Insert user <-> list relation
-	_, err = x.Insert(ul)
+	_, err = x.Insert(lu)
 
 	return
 }

pkg/models/list_users_readall.go

@@ -32,26 +32,26 @@ import "code.vikunja.io/web"
 // @Failure 403 {object} code.vikunja.io/web.HTTPError "No right to see the list."
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /lists/{id}/users [get]
-func (ul *ListUser) ReadAll(search string, a web.Auth, page int) (interface{}, error) {
+func (lu *ListUser) ReadAll(search string, a web.Auth, page int) (interface{}, error) {
 	u, err := getUserWithError(a)
 	if err != nil {
 		return nil, err
 	}
 
 	// Check if the user has access to the list
-	l := &List{ID: ul.ListID}
+	l := &List{ID: lu.ListID}
 	if err := l.GetSimpleByID(); err != nil {
 		return nil, err
 	}
 	if !l.CanRead(u) {
-		return nil, ErrNeedToHaveListReadAccess{UserID: u.ID, ListID: ul.ListID}
+		return nil, ErrNeedToHaveListReadAccess{UserID: u.ID, ListID: lu.ListID}
 	}
 
 	// Get all users
 	all := []*UserWithRight{}
 	err = x.
 		Join("INNER", "users_list", "user_id = users.id").
-		Where("users_list.list_id = ?", ul.ListID).
+		Where("users_list.list_id = ?", lu.ListID).
 		Limit(getLimitFromPageIndex(page)).
 		Where("users.username LIKE ?", "%"+search+"%").
 		Find(&all)

pkg/models/list_users_rights.go

@@ -17,45 +17,26 @@
 package models
 
 import (
-	"code.vikunja.io/api/pkg/log"
 	"code.vikunja.io/web"
 )
 
 // CanCreate checks if the user can create a new user <-> list relation
 func (lu *ListUser) CanCreate(a web.Auth) bool {
-	doer := getUserForRights(a)
-
 	// Get the list and check if the user has write access on it
 	l := List{ID: lu.ListID}
-	if err := l.GetSimpleByID(); err != nil {
+	return l.CanWrite(a)
-		log.Log.Error("Error occurred during CanCreate for ListUser: %s", err)
-		return false
-	}
-	return l.CanWrite(doer)
 }
 
 // CanDelete checks if the user can delete a user <-> list relation
 func (lu *ListUser) CanDelete(a web.Auth) bool {
-	doer := getUserForRights(a)
-
 	// Get the list and check if the user has write access on it
 	l := List{ID: lu.ListID}
-	if err := l.GetSimpleByID(); err != nil {
+	return l.CanWrite(a)
-		log.Log.Error("Error occurred during CanDelete for ListUser: %s", err)
-		return false
-	}
-	return l.CanWrite(doer)
 }
 
 // CanUpdate checks if the user can update a user <-> list relation
 func (lu *ListUser) CanUpdate(a web.Auth) bool {
-	doer := getUserForRights(a)
-
 	// Get the list and check if the user has write access on it
 	l := List{ID: lu.ListID}
-	if err := l.GetSimpleByID(); err != nil {
+	return l.CanWrite(a)
-		log.Log.Error("Error occurred during CanUpdate for ListUser: %s", err)
-		return false
-	}
-	return l.CanWrite(doer)
 }

pkg/models/namespace.go

@@ -57,35 +57,44 @@ func (Namespace) TableName() string {
 	return "namespaces"
 }
 
+// GetSimpleByID gets a namespace without things like the owner, it more or less only checks if it exists.
+func (n *Namespace) GetSimpleByID() (err error) {
+	if n.ID == 0 {
+		return ErrNamespaceDoesNotExist{ID: n.ID}
+	}
+
+	// Get the namesapce with shared lists
+	if n.ID == -1 {
+		*n = PseudoNamespace
+		return
+	}
+
+	exists, err := x.Get(n)
+	if err != nil {
+		return
+	}
+	if !exists {
+		return ErrNamespaceDoesNotExist{ID: n.ID}
+	}
+
+	return
+}
+
 // GetNamespaceByID returns a namespace object by its ID
 func GetNamespaceByID(id int64) (namespace Namespace, err error) {
-	if id == 0 {
+	namespace = Namespace{ID: id}
-		return namespace, ErrNamespaceDoesNotExist{ID: id}
+	err = namespace.GetSimpleByID()
-	}
-
-	namespace.ID = id
-	// Get the namesapce with shared lists
-	if id == -1 {
-		namespace = PseudoNamespace
-		return namespace, err
-	}
-
-	exists, err := x.Get(&namespace)
 	if err != nil {
-		return namespace, err
+		return
-	}
-
-	if !exists {
-		return namespace, ErrNamespaceDoesNotExist{ID: id}
 	}
 
 	// Get the namespace Owner
 	namespace.Owner, err = GetUserByID(namespace.OwnerID)
 	if err != nil {
-		return namespace, err
+		return
 	}
 
-	return namespace, err
+	return
 }
 
 // ReadOne gets one namespace

pkg/models/namespace_rights.go

@@ -24,8 +24,30 @@ import (
 
 // CanWrite checks if a user has write access to a namespace
 func (n *Namespace) CanWrite(a web.Auth) bool {
+
+	// Get the namespace and check the right
+	originalNamespace := &Namespace{ID: n.ID}
+	err := originalNamespace.GetSimpleByID()
+	if err != nil {
+		log.Log.Error("Error occurred during CanWrite for Namespace: %s", err)
+		return false
+	}
+
 	u := getUserForRights(a)
-	return n.isOwner(u) || n.checkRight(u, RightWrite, RightAdmin)
+	return originalNamespace.isOwner(u) || 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 {
+	originalNamespace := &Namespace{ID: n.ID}
+	err := originalNamespace.GetSimpleByID()
+	if err != nil {
+		log.Log.Error("Error occurred during IsAdmin for Namespace: %s", err)
+		return false
+	}
+
+	u := getUserForRights(a)
+	return originalNamespace.isOwner(u) || originalNamespace.checkRight(u, RightAdmin)
 }
 
 // CanRead checks if a user has read access to that namespace
@@ -50,12 +72,6 @@ func (n *Namespace) CanCreate(a web.Auth) bool {
 	return true
 }
 
-// IsAdmin returns true or false if the user is admin on that namespace or not
-func (n *Namespace) IsAdmin(a web.Auth) bool {
-	u := getUserForRights(a)
-	return n.isOwner(u) || n.checkRight(u, RightAdmin)
-}
-
 // Small helper function to check if a user owns the namespace
 func (n *Namespace) isOwner(user *User) bool {
 	return n.OwnerID == user.ID

pkg/models/namespace_users_create.go

@@ -33,42 +33,42 @@ import "code.vikunja.io/web"
 // @Failure 403 {object} code.vikunja.io/web.HTTPError "The user does not have access to the namespace"
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /namespaces/{id}/users [put]
-func (un *NamespaceUser) Create(a web.Auth) (err error) {
+func (nu *NamespaceUser) Create(a web.Auth) (err error) {
 	// Reset the id
-	un.ID = 0
+	nu.ID = 0
 
 	// Check if the right is valid
-	if err := un.Right.isValid(); err != nil {
+	if err := nu.Right.isValid(); err != nil {
 		return err
 	}
 
 	// Check if the namespace exists
-	l, err := GetNamespaceByID(un.NamespaceID)
+	l, err := GetNamespaceByID(nu.NamespaceID)
 	if err != nil {
 		return
 	}
 
 	// Check if the user exists
-	if _, err = GetUserByID(un.UserID); err != nil {
+	if _, err = GetUserByID(nu.UserID); err != nil {
 		return err
 	}
 
 	// Check if the user already has access or is owner of that namespace
 	// We explicitly DO NOT check for teams here
-	if l.OwnerID == un.UserID {
+	if l.OwnerID == nu.UserID {
-		return ErrUserAlreadyHasNamespaceAccess{UserID: un.UserID, NamespaceID: un.NamespaceID}
+		return ErrUserAlreadyHasNamespaceAccess{UserID: nu.UserID, NamespaceID: nu.NamespaceID}
 	}
 
-	exist, err := x.Where("namespace_id = ? AND user_id = ?", un.NamespaceID, un.UserID).Get(&NamespaceUser{})
+	exist, err := x.Where("namespace_id = ? AND user_id = ?", nu.NamespaceID, nu.UserID).Get(&NamespaceUser{})
 	if err != nil {
 		return
 	}
 	if exist {
-		return ErrUserAlreadyHasNamespaceAccess{UserID: un.UserID, NamespaceID: un.NamespaceID}
+		return ErrUserAlreadyHasNamespaceAccess{UserID: nu.UserID, NamespaceID: nu.NamespaceID}
 	}
 
 	// Insert user <-> namespace relation
-	_, err = x.Insert(un)
+	_, err = x.Insert(nu)
 
 	return
 }

pkg/models/namespace_users_readall.go

@@ -32,14 +32,14 @@ import "code.vikunja.io/web"
 // @Failure 403 {object} code.vikunja.io/web.HTTPError "No right to see the namespace."
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /namespaces/{id}/users [get]
-func (un *NamespaceUser) ReadAll(search string, a web.Auth, page int) (interface{}, error) {
+func (nu *NamespaceUser) ReadAll(search string, a web.Auth, page int) (interface{}, error) {
 	u, err := getUserWithError(a)
 	if err != nil {
 		return nil, err
 	}
 
 	// Check if the user has access to the namespace
-	l, err := GetNamespaceByID(un.NamespaceID)
+	l, err := GetNamespaceByID(nu.NamespaceID)
 	if err != nil {
 		return nil, err
 	}
@@ -51,7 +51,7 @@ func (un *NamespaceUser) ReadAll(search string, a web.Auth, page int) (interface
 	all := []*UserWithRight{}
 	err = x.
 		Join("INNER", "users_namespace", "user_id = users.id").
-		Where("users_namespace.namespace_id = ?", un.NamespaceID).
+		Where("users_namespace.namespace_id = ?", nu.NamespaceID).
 		Limit(getLimitFromPageIndex(page)).
 		Where("users.username LIKE ?", "%"+search+"%").
 		Find(&all)

pkg/models/namespace_users_rights.go

@@ -17,45 +17,23 @@
 package models
 
 import (
-	"code.vikunja.io/api/pkg/log"
 	"code.vikunja.io/web"
 )
 
 // CanCreate checks if the user can create a new user <-> namespace relation
 func (nu *NamespaceUser) CanCreate(a web.Auth) bool {
-	doer := getUserForRights(a)
+	n := &Namespace{ID: nu.NamespaceID}
-
+	return n.CanWrite(a)
-	// Get the namespace and check if the user has write access on it
-	n, err := GetNamespaceByID(nu.NamespaceID)
-	if err != nil {
-		log.Log.Error("Error occurred during CanCreate for NamespaceUser: %s", err)
-		return false
-	}
-	return n.CanWrite(doer)
 }
 
 // CanDelete checks if the user can delete a user <-> namespace relation
 func (nu *NamespaceUser) CanDelete(a web.Auth) bool {
-	doer := getUserForRights(a)
+	n := &Namespace{ID: nu.NamespaceID}
-
+	return n.CanWrite(a)
-	// Get the namespace and check if the user has write access on it
-	n, err := GetNamespaceByID(nu.NamespaceID)
-	if err != nil {
-		log.Log.Error("Error occurred during CanDelete for NamespaceUser: %s", err)
-		return false
-	}
-	return n.CanWrite(doer)
 }
 
 // CanUpdate checks if the user can update a user <-> namespace relation
 func (nu *NamespaceUser) CanUpdate(a web.Auth) bool {
-	doer := getUserForRights(a)
+	n := &Namespace{ID: nu.NamespaceID}
-
+	return n.CanWrite(a)
-	// Get the namespace and check if the user has write access on it
-	n, err := GetNamespaceByID(nu.NamespaceID)
-	if err != nil {
-		log.Log.Error("Error occurred during CanUpdate for NamespaceUser: %s", err)
-		return false
-	}
-	return n.CanWrite(doer)
 }

pkg/models/team_list_rights.go

@@ -17,7 +17,6 @@
 package models
 
 import (
-	"code.vikunja.io/api/pkg/log"
 	"code.vikunja.io/web"
 )
 
@@ -26,10 +25,6 @@ func (tl *TeamList) CanCreate(a web.Auth) bool {
 	u := getUserForRights(a)
 
 	l := List{ID: tl.ListID}
-	if err := l.GetSimpleByID(); err != nil {
-		log.Log.Error("Error occurred during CanCreate for TeamList: %s", err)
-		return false
-	}
 	return l.IsAdmin(u)
 }
 
@@ -38,10 +33,6 @@ func (tl *TeamList) CanDelete(a web.Auth) bool {
 	user := getUserForRights(a)
 
 	l := List{ID: tl.ListID}
-	if err := l.GetSimpleByID(); err != nil {
-		log.Log.Error("Error occurred during CanDelete for TeamList: %s", err)
-		return false
-	}
 	return l.IsAdmin(user)
 }
 
@@ -50,9 +41,5 @@ func (tl *TeamList) CanUpdate(a web.Auth) bool {
 	user := getUserForRights(a)
 
 	l := List{ID: tl.ListID}
-	if err := l.GetSimpleByID(); err != nil {
-		log.Log.Error("Error occurred during CanUpdate for TeamList: %s", err)
-		return false
-	}
 	return l.IsAdmin(user)
 }

pkg/models/team_namespace_rights.go

@@ -17,42 +17,23 @@
 package models
 
 import (
-	"code.vikunja.io/api/pkg/log"
 	"code.vikunja.io/web"
 )
 
 // CanCreate checks if one can create a new team <-> namespace relation
 func (tn *TeamNamespace) CanCreate(a web.Auth) bool {
-	user := getUserForRights(a)
+	n := &Namespace{ID: tn.NamespaceID}
-
+	return n.IsAdmin(a)
-	n, err := GetNamespaceByID(tn.NamespaceID)
-	if err != nil {
-		log.Log.Error("Error occurred during CanCreate for TeamNamespace: %s", err)
-		return false
-	}
-	return n.IsAdmin(user)
 }
 
 // CanDelete checks if a user can remove a team from a namespace. Only namespace admins can do that.
 func (tn *TeamNamespace) CanDelete(a web.Auth) bool {
-	user := getUserForRights(a)
+	n := &Namespace{ID: tn.NamespaceID}
-
+	return n.IsAdmin(a)
-	n, err := GetNamespaceByID(tn.NamespaceID)
-	if err != nil {
-		log.Log.Error("Error occurred during CanDelete for TeamNamespace: %s", err)
-		return false
-	}
-	return n.IsAdmin(user)
 }
 
 // CanUpdate checks if a user can update a team from a  Only namespace admins can do that.
 func (tn *TeamNamespace) CanUpdate(a web.Auth) bool {
-	user := getUserForRights(a)
+	n := &Namespace{ID: tn.NamespaceID}
-
+	return n.IsAdmin(a)
-	n, err := GetNamespaceByID(tn.NamespaceID)
-	if err != nil {
-		log.Log.Error("Error occurred during CanUpdate for TeamNamespace: %s", err)
-		return false
-	}
-	return n.IsAdmin(user)
 }

pkg/models/team_namespace_update.go

@@ -33,16 +33,16 @@ import _ "code.vikunja.io/web" // For swaggerdocs generation
 // @Failure 404 {object} code.vikunja.io/web.HTTPError "Team or namespace does not exist."
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /namespaces/{namespaceID}/teams/{teamID} [post]
-func (tl *TeamNamespace) Update() (err error) {
+func (tn *TeamNamespace) Update() (err error) {
 
 	// Check if the right is valid
-	if err := tl.Right.isValid(); err != nil {
+	if err := tn.Right.isValid(); err != nil {
 		return err
 	}
 
 	_, err = x.
-		Where("namespace_id = ? AND team_id = ?", tl.TeamID, tl.TeamID).
+		Where("namespace_id = ? AND team_id = ?", tn.TeamID, tn.TeamID).
 		Cols("right").
-		Update(tl)
+		Update(tn)
 	return
 }