TheArchive/zitadel
1
0
Fork 0
mirror of https://github.com/zitadel/zitadel.git synced 2025-08-12 05:07:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: generalise permission check for query user information (#8458)

Browse Source 
# Which Problems Are Solved

IDPLinks list and other list endpoints can provide you with empty
results if the used user has no permission for the information.

# How the Problems Are Solved

List endpoints with subelements to users, and provided userIDQuery, will
return a PermissionDenied error if no permission for the user exsists.

# Additional Changes

Function to check for permission is re-used from the GetUserByID.

# Additional Context

Closes #8451
This commit is contained in:
Stefan Benz
2024-08-23 08:44:18 +02:00
committed by GitHub
parent 8051a63147
commit 2847806531
27 changed files with 552 additions and 111 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
internal/query/idp_user_link.go
View File

@@ -3,6 +3,7 @@ package query
import (
"context"
"database/sql"
"slices"
sq "github.com/Masterminds/squirrel"
@@ -42,6 +43,15 @@ func (q *IDPUserLinksSearchQuery) toQuery(query sq.SelectBuilder) sq.SelectBuild
return query
}
func (q *IDPUserLinksSearchQuery) hasUserID() bool {
for _, query := range q.Queries {
if query.Col() == IDPUserLinkUserIDCol {
return true
}
}
return false
}
var (
idpUserLinkTable = table{
name: projection.IDPUserLinkTable,
@@ -89,30 +99,33 @@ var (
}
)
func (l *IDPUserLinks) RemoveNoPermission(ctx context.Context, permissionCheck domain.PermissionCheck) {
removableIndexes := make([]int, 0)
for i := range l.Links {
ctxData := authz.GetCtxData(ctx)
if ctxData.UserID != l.Links[i].UserID {
if err := permissionCheck(ctx, domain.PermissionUserRead, l.Links[i].ResourceOwner, l.Links[i].UserID); err != nil {
removableIndexes = append(removableIndexes, i)
func idpLinksCheckPermission(ctx context.Context, links *IDPUserLinks, permissionCheck domain.PermissionCheck) {
links.Links = slices.DeleteFunc(links.Links,
func(link *IDPUserLink) bool {
return userCheckPermission(ctx, link.ResourceOwner, link.UserID, permissionCheck) != nil
},
)
}
func (q *Queries) IDPUserLinks(ctx context.Context, queries *IDPUserLinksSearchQuery, permissionCheck domain.PermissionCheck) (idps *IDPUserLinks, err error) {
links, err := q.idpUserLinks(ctx, queries, false)
if err != nil {
return nil, err
}
if permissionCheck != nil && len(links.Links) > 0 {
// when userID for query is provided, only one check has to be done
if queries.hasUserID() {
if err := userCheckPermission(ctx, links.Links[0].ResourceOwner, links.Links[0].UserID, permissionCheck); err != nil {
return nil, err
}
} else {
idpLinksCheckPermission(ctx, links, permissionCheck)
}
}
removed := 0
for _, removeIndex := range removableIndexes {
l.Links = removeIDPLink(l.Links, removeIndex-removed)
removed++
}
// reset count as some users could be removed
l.SearchResponse.Count = uint64(len(l.Links))
return links, nil
}
func removeIDPLink(slice []*IDPUserLink, s int) []*IDPUserLink {
return append(slice[:s], slice[s+1:]...)
}
func (q *Queries) IDPUserLinks(ctx context.Context, queries *IDPUserLinksSearchQuery, withOwnerRemoved bool) (idps *IDPUserLinks, err error) {
func (q *Queries) idpUserLinks(ctx context.Context, queries *IDPUserLinksSearchQuery, withOwnerRemoved bool) (idps *IDPUserLinks, err error) {
ctx, span := tracing.NewSpan(ctx)
defer func() { span.EndWithError(err) }()

167
internal/query/idp_user_link_test.go
View File

@@ -1,6 +1,7 @@
package query
import (
"context"
"database/sql"
"database/sql/driver"
"errors"
@@ -8,9 +9,175 @@ import (
"regexp"
"testing"
"github.com/stretchr/testify/require"
"github.com/zitadel/zitadel/internal/api/authz"
"github.com/zitadel/zitadel/internal/domain"
)
func TestUser_idpLinksCheckPermission(t *testing.T) {
type want struct {
links []*IDPUserLink
}
type args struct {
user string
links *IDPUserLinks
}
tests := []struct {
name string
args args
want want
permissions []string
}{
{
"permissions for all users",
args{
"none",
&IDPUserLinks{
Links: []*IDPUserLink{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
links: []*IDPUserLink{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
[]string{"first", "second", "third"},
},
{
"permissions for one user, first",
args{
"none",
&IDPUserLinks{
Links: []*IDPUserLink{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
links: []*IDPUserLink{
{UserID: "first"},
},
},
[]string{"first"},
},
{
"permissions for one user, second",
args{
"none",
&IDPUserLinks{
Links: []*IDPUserLink{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
links: []*IDPUserLink{
{UserID: "second"},
},
},
[]string{"second"},
},
{
"permissions for one user, third",
args{
"none",
&IDPUserLinks{
Links: []*IDPUserLink{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
links: []*IDPUserLink{
{UserID: "third"},
},
},
[]string{"third"},
},
{
"permissions for two users, first",
args{
"none",
&IDPUserLinks{
Links: []*IDPUserLink{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
links: []*IDPUserLink{
{UserID: "first"}, {UserID: "third"},
},
},
[]string{"first", "third"},
},
{
"permissions for two users, second",
args{
"none",
&IDPUserLinks{
Links: []*IDPUserLink{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
links: []*IDPUserLink{
{UserID: "second"}, {UserID: "third"},
},
},
[]string{"second", "third"},
},
{
"no permissions",
args{
"none",
&IDPUserLinks{
Links: []*IDPUserLink{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
links: []*IDPUserLink{},
},
[]string{},
},
{
"no permissions, self",
args{
"second",
&IDPUserLinks{
Links: []*IDPUserLink{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
links: []*IDPUserLink{{UserID: "second"}},
},
[]string{},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
checkPermission := func(ctx context.Context, permission, orgID, resourceID string) (err error) {
for _, perm := range tt.permissions {
if resourceID == perm {
return nil
}
}
return errors.New("failed")
}
idpLinksCheckPermission(authz.SetCtxData(context.Background(), authz.CtxData{UserID: tt.args.user}), tt.args.links, checkPermission)
require.Equal(t, tt.want.links, tt.args.links.Links)
})
}
}
var (
idpUserLinksQuery = regexp.QuoteMeta(`SELECT projections.idp_user_links3.idp_id,` +
` projections.idp_user_links3.user_id,` +

3
internal/query/org_test.go
View File

@@ -439,11 +439,10 @@ func TestQueries_IsOrgUnique(t *testing.T) {
t.Errorf("expectation was met: %v", err)
}
})
}
}
func TestOrg_RemoveNoPermission(t *testing.T) {
func TestOrg_orgsCheckPermission(t *testing.T) {
type want struct {
orgs []*Org
}

29
internal/query/user.go
View File

@@ -125,15 +125,9 @@ type NotifyUser struct {
}
func usersCheckPermission(ctx context.Context, users *Users, permissionCheck domain.PermissionCheck) {
ctxData := authz.GetCtxData(ctx)
users.Users = slices.DeleteFunc(users.Users,
func(user *User) bool {
if ctxData.UserID != user.ID {
if err := permissionCheck(ctx, domain.PermissionUserRead, user.ResourceOwner, user.ID); err != nil {
return true
}
}
return false
return userCheckPermission(ctx, user.ResourceOwner, user.ID, permissionCheck) != nil
},
)
}
@@ -347,6 +341,27 @@ var (
//go:embed user_by_id.sql
var userByIDQuery string
func userCheckPermission(ctx context.Context, resourceOwner string, userID string, permissionCheck domain.PermissionCheck) error {
ctxData := authz.GetCtxData(ctx)
if ctxData.UserID != userID {
if err := permissionCheck(ctx, domain.PermissionUserRead, resourceOwner, userID); err != nil {
return err
}
}
return nil
}
func (q *Queries) GetUserByIDWithPermission(ctx context.Context, shouldTriggerBulk bool, userID string, permissionCheck domain.PermissionCheck) (*User, error) {
user, err := q.GetUserByID(ctx, shouldTriggerBulk, userID)
if err != nil {
return nil, err
}
if err := userCheckPermission(ctx, user.ResourceOwner, user.ID, permissionCheck); err != nil {
return nil, err
}
return user, nil
}
func (q *Queries) GetUserByID(ctx context.Context, shouldTriggerBulk bool, userID string) (user *User, err error) {
ctx, span := tracing.NewSpan(ctx)
defer func() { span.EndWithError(err) }()

57
internal/query/user_auth_method.go
View File

@@ -4,6 +4,7 @@ import (
"context"
"database/sql"
"errors"
"slices"
"time"
sq "github.com/Masterminds/squirrel"
@@ -98,27 +99,12 @@ type AuthMethods struct {
AuthMethods []*AuthMethod
}
func (l *AuthMethods) RemoveNoPermission(ctx context.Context, permissionCheck domain.PermissionCheck) {
removableIndexes := make([]int, 0)
for i := range l.AuthMethods {
ctxData := authz.GetCtxData(ctx)
if ctxData.UserID != l.AuthMethods[i].UserID {
if err := permissionCheck(ctx, domain.PermissionUserRead, l.AuthMethods[i].ResourceOwner, l.AuthMethods[i].UserID); err != nil {
removableIndexes = append(removableIndexes, i)
}
}
}
removed := 0
for _, removeIndex := range removableIndexes {
l.AuthMethods = removeAuthMethod(l.AuthMethods, removeIndex-removed)
removed++
}
// reset count as some users could be removed
l.SearchResponse.Count = uint64(len(l.AuthMethods))
}
func removeAuthMethod(slice []*AuthMethod, s int) []*AuthMethod {
return append(slice[:s], slice[s+1:]...)
func authMethodsCheckPermission(ctx context.Context, methods *AuthMethods, permissionCheck domain.PermissionCheck) {
methods.AuthMethods = slices.DeleteFunc(methods.AuthMethods,
func(method *AuthMethod) bool {
return userCheckPermission(ctx, method.ResourceOwner, method.UserID, permissionCheck) != nil
},
)
}
type AuthMethod struct {
@@ -144,7 +130,34 @@ type UserAuthMethodSearchQueries struct {
Queries []SearchQuery
}
func (q *Queries) SearchUserAuthMethods(ctx context.Context, queries *UserAuthMethodSearchQueries, withOwnerRemoved bool) (userAuthMethods *AuthMethods, err error) {
func (q *UserAuthMethodSearchQueries) hasUserID() bool {
for _, query := range q.Queries {
if query.Col() == UserAuthMethodColumnUserID {
return true
}
}
return false
}
func (q *Queries) SearchUserAuthMethods(ctx context.Context, queries *UserAuthMethodSearchQueries, permissionCheck domain.PermissionCheck) (userAuthMethods *AuthMethods, err error) {
methods, err := q.searchUserAuthMethods(ctx, queries, false)
if err != nil {
return nil, err
}
if permissionCheck != nil && len(methods.AuthMethods) > 0 {
// when userID for query is provided, only one check has to be done
if queries.hasUserID() {
if err := userCheckPermission(ctx, methods.AuthMethods[0].ResourceOwner, methods.AuthMethods[0].UserID, permissionCheck); err != nil {
return nil, err
}
} else {
authMethodsCheckPermission(ctx, methods, permissionCheck)
}
}
return methods, nil
}
func (q *Queries) searchUserAuthMethods(ctx context.Context, queries *UserAuthMethodSearchQueries, withOwnerRemoved bool) (userAuthMethods *AuthMethods, err error) {
ctx, span := tracing.NewSpan(ctx)
defer func() { span.EndWithError(err) }()

165
internal/query/user_auth_method_test.go
View File

@@ -10,11 +10,176 @@ import (
"testing"
sq "github.com/Masterminds/squirrel"
"github.com/stretchr/testify/require"
"github.com/zitadel/zitadel/internal/api/authz"
"github.com/zitadel/zitadel/internal/domain"
"github.com/zitadel/zitadel/internal/zerrors"
)
func TestUser_authMethodsCheckPermission(t *testing.T) {
type want struct {
methods []*AuthMethod
}
type args struct {
user string
methods *AuthMethods
}
tests := []struct {
name string
args args
want want
permissions []string
}{
{
"permissions for all users",
args{
"none",
&AuthMethods{
AuthMethods: []*AuthMethod{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
methods: []*AuthMethod{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
[]string{"first", "second", "third"},
},
{
"permissions for one user, first",
args{
"none",
&AuthMethods{
AuthMethods: []*AuthMethod{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
methods: []*AuthMethod{
{UserID: "first"},
},
},
[]string{"first"},
},
{
"permissions for one user, second",
args{
"none",
&AuthMethods{
AuthMethods: []*AuthMethod{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
methods: []*AuthMethod{
{UserID: "second"},
},
},
[]string{"second"},
},
{
"permissions for one user, third",
args{
"none",
&AuthMethods{
AuthMethods: []*AuthMethod{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
methods: []*AuthMethod{
{UserID: "third"},
},
},
[]string{"third"},
},
{
"permissions for two users, first",
args{
"none",
&AuthMethods{
AuthMethods: []*AuthMethod{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
methods: []*AuthMethod{
{UserID: "first"}, {UserID: "third"},
},
},
[]string{"first", "third"},
},
{
"permissions for two users, second",
args{
"none",
&AuthMethods{
AuthMethods: []*AuthMethod{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
methods: []*AuthMethod{
{UserID: "second"}, {UserID: "third"},
},
},
[]string{"second", "third"},
},
{
"no permissions",
args{
"none",
&AuthMethods{
AuthMethods: []*AuthMethod{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
methods: []*AuthMethod{},
},
[]string{},
},
{
"no permissions, self",
args{
"second",
&AuthMethods{
AuthMethods: []*AuthMethod{
{UserID: "first"}, {UserID: "second"}, {UserID: "third"},
},
},
},
want{
methods: []*AuthMethod{{UserID: "second"}},
},
[]string{},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
checkPermission := func(ctx context.Context, permission, orgID, resourceID string) (err error) {
for _, perm := range tt.permissions {
if resourceID == perm {
return nil
}
}
return errors.New("failed")
}
authMethodsCheckPermission(authz.SetCtxData(context.Background(), authz.CtxData{UserID: tt.args.user}), tt.args.methods, checkPermission)
require.Equal(t, tt.want.methods, tt.args.methods.AuthMethods)
})
}
}
var (
prepareUserAuthMethodsStmt = `SELECT projections.user_auth_methods4.token_id,` +
` projections.user_auth_methods4.creation_date,` +

83
internal/query/user_test.go
View File

@@ -9,15 +9,17 @@ import (
"regexp"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/text/language"
"github.com/zitadel/zitadel/internal/api/authz"
"github.com/zitadel/zitadel/internal/database"
"github.com/zitadel/zitadel/internal/domain"
"github.com/zitadel/zitadel/internal/zerrors"
)
func TestUser_RemoveNoPermission(t *testing.T) {
func TestUser_usersCheckPermission(t *testing.T) {
type want struct {
users []*User
}
@@ -140,6 +142,85 @@ func TestUser_RemoveNoPermission(t *testing.T) {
}
}
func TestUser_userCheckPermission(t *testing.T) {
type args struct {
ctxData string
resourceowner string
user string
}
type perm struct {
resourceowner string
user string
}
tests := []struct {
name string
wantErr bool
args args
permissions []perm
}{
{
name: "permission, self",
args: args{
resourceowner: "org",
user: "user",
ctxData: "user",
},
permissions: []perm{},
},
{
name: "permission, user",
args: args{
resourceowner: "org1",
user: "user1",
ctxData: "user2",
},
permissions: []perm{{"org1", "user1"}},
wantErr: false,
},
{
name: "permission, org",
args: args{
resourceowner: "org1",
user: "user1",
ctxData: "user2",
},
permissions: []perm{{"org1", "user3"}},
},
{
name: "permission, none",
args: args{
resourceowner: "org1",
user: "user1",
ctxData: "user2",
},
permissions: []perm{},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
checkPermission := func(ctx context.Context, permission, orgID, resourceID string) (err error) {
for _, perm := range tt.permissions {
if resourceID == perm.user {
return nil
}
if orgID == perm.resourceowner {
return nil
}
}
return errors.New("failed")
}
granted := userCheckPermission(authz.SetCtxData(context.Background(), authz.CtxData{UserID: tt.args.ctxData}), tt.args.resourceowner, tt.args.user, checkPermission)
if tt.wantErr {
assert.Error(t, granted)
} else {
assert.NoError(t, granted)
}
})
}
}
var (
loginNamesQuery = `SELECT login_names.user_id, ARRAY_AGG(login_names.login_name)::TEXT[] AS loginnames, ARRAY_AGG(LOWER(login_names.login_name))::TEXT[] AS loginnames_lower, login_names.instance_id` +
` FROM projections.login_names3 AS login_names` +