diff --git a/internal/api/grpc/admin/export.go b/internal/api/grpc/admin/export.go index 8394ae78d..dfb0cd346 100644 --- a/internal/api/grpc/admin/export.go +++ b/internal/api/grpc/admin/export.go @@ -468,7 +468,7 @@ func (s *Server) getUserLinks(ctx context.Context, orgID string) (_ []*idp_pb.ID if err != nil { return nil, err } - idpUserLinks, err := s.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: []query.SearchQuery{userLinksResourceOwner}}, false) + idpUserLinks, err := s.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: []query.SearchQuery{userLinksResourceOwner}}, nil) if err != nil { return nil, err } diff --git a/internal/api/grpc/admin/idp.go b/internal/api/grpc/admin/idp.go index 5528a94db..d0907d6e1 100644 --- a/internal/api/grpc/admin/idp.go +++ b/internal/api/grpc/admin/idp.go @@ -112,7 +112,7 @@ func (s *Server) RemoveIDP(ctx context.Context, req *admin_pb.RemoveIDPRequest) } userLinks, err := s.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{ Queries: []query.SearchQuery{idpQuery}, - }, true) + }, nil) if err != nil { return nil, err } diff --git a/internal/api/grpc/auth/idp.go b/internal/api/grpc/auth/idp.go index 9d1759ae8..57018b04c 100644 --- a/internal/api/grpc/auth/idp.go +++ b/internal/api/grpc/auth/idp.go @@ -13,7 +13,7 @@ func (s *Server) ListMyLinkedIDPs(ctx context.Context, req *auth_pb.ListMyLinked if err != nil { return nil, err } - links, err := s.query.IDPUserLinks(ctx, q, false) + links, err := s.query.IDPUserLinks(ctx, q, nil) if err != nil { return nil, err } diff --git a/internal/api/grpc/auth/multi_factor.go b/internal/api/grpc/auth/multi_factor.go index ed3cf421d..0771ded35 100644 --- a/internal/api/grpc/auth/multi_factor.go +++ b/internal/api/grpc/auth/multi_factor.go @@ -26,7 +26,7 @@ func (s *Server) ListMyAuthFactors(ctx context.Context, _ *auth_pb.ListMyAuthFac if err != nil { return nil, err } - authMethods, err := s.query.SearchUserAuthMethods(ctx, query, false) + authMethods, err := s.query.SearchUserAuthMethods(ctx, query, nil) if err != nil { return nil, err } diff --git a/internal/api/grpc/auth/passwordless.go b/internal/api/grpc/auth/passwordless.go index ddc0a331c..87f310e8d 100644 --- a/internal/api/grpc/auth/passwordless.go +++ b/internal/api/grpc/auth/passwordless.go @@ -30,7 +30,7 @@ func (s *Server) ListMyPasswordless(ctx context.Context, _ *auth_pb.ListMyPasswo if err != nil { return nil, err } - authMethods, err := s.query.SearchUserAuthMethods(ctx, query, false) + authMethods, err := s.query.SearchUserAuthMethods(ctx, query, nil) if err != nil { return nil, err } diff --git a/internal/api/grpc/management/idp.go b/internal/api/grpc/management/idp.go index 66b659d1e..9ef7b833a 100644 --- a/internal/api/grpc/management/idp.go +++ b/internal/api/grpc/management/idp.go @@ -91,7 +91,7 @@ func (s *Server) RemoveOrgIDP(ctx context.Context, req *mgmt_pb.RemoveOrgIDPRequ } userLinks, err := s.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{ Queries: []query.SearchQuery{idpQuery}, - }, true) + }, nil) if err != nil { return nil, err } diff --git a/internal/api/grpc/management/user.go b/internal/api/grpc/management/user.go index 981e7823a..dac651af8 100644 --- a/internal/api/grpc/management/user.go +++ b/internal/api/grpc/management/user.go @@ -608,7 +608,7 @@ func (s *Server) ListHumanAuthFactors(ctx context.Context, req *mgmt_pb.ListHuma if err != nil { return nil, err } - authMethods, err := s.query.SearchUserAuthMethods(ctx, query, false) + authMethods, err := s.query.SearchUserAuthMethods(ctx, query, nil) if err != nil { return nil, err } @@ -671,7 +671,7 @@ func (s *Server) ListHumanPasswordless(ctx context.Context, req *mgmt_pb.ListHum if err != nil { return nil, err } - authMethods, err := s.query.SearchUserAuthMethods(ctx, query, false) + authMethods, err := s.query.SearchUserAuthMethods(ctx, query, nil) if err != nil { return nil, err } @@ -892,7 +892,7 @@ func (s *Server) ListHumanLinkedIDPs(ctx context.Context, req *mgmt_pb.ListHuman if err != nil { return nil, err } - res, err := s.query.IDPUserLinks(ctx, queries, false) + res, err := s.query.IDPUserLinks(ctx, queries, nil) if err != nil { return nil, err } diff --git a/internal/api/grpc/user/v2/idp_link.go b/internal/api/grpc/user/v2/idp_link.go index 5567ab24a..bef40617c 100644 --- a/internal/api/grpc/user/v2/idp_link.go +++ b/internal/api/grpc/user/v2/idp_link.go @@ -30,11 +30,10 @@ func (s *Server) ListIDPLinks(ctx context.Context, req *user.ListIDPLinksRequest if err != nil { return nil, err } - res, err := s.query.IDPUserLinks(ctx, queries, false) + res, err := s.query.IDPUserLinks(ctx, queries, s.checkPermission) if err != nil { return nil, err } - res.RemoveNoPermission(ctx, s.checkPermission) return &user.ListIDPLinksResponse{ Result: IDPLinksToPb(res.Links), Details: object.ToListDetails(res.SearchResponse), diff --git a/internal/api/grpc/user/v2/idp_link_integration_test.go b/internal/api/grpc/user/v2/idp_link_integration_test.go index 6b85c80f9..99658b302 100644 --- a/internal/api/grpc/user/v2/idp_link_integration_test.go +++ b/internal/api/grpc/user/v2/idp_link_integration_test.go @@ -122,6 +122,16 @@ func TestServer_ListIDPLinks(t *testing.T) { want *user.ListIDPLinksResponse wantErr bool }{ + { + name: "list links, missing userID", + args: args{ + IamCTX, + &user.ListIDPLinksRequest{ + UserId: "", + }, + }, + wantErr: true, + }, { name: "list links, no permission", args: args{ @@ -130,13 +140,7 @@ func TestServer_ListIDPLinks(t *testing.T) { UserId: userOrgResp.GetUserId(), }, }, - want: &user.ListIDPLinksResponse{ - Details: &object.ListDetails{ - TotalResult: 0, - Timestamp: timestamppb.Now(), - }, - Result: []*user.IDPLink{}, - }, + wantErr: true, }, { name: "list links, no permission, org", @@ -146,13 +150,7 @@ func TestServer_ListIDPLinks(t *testing.T) { UserId: userOrgResp.GetUserId(), }, }, - want: &user.ListIDPLinksResponse{ - Details: &object.ListDetails{ - TotalResult: 0, - Timestamp: timestamppb.Now(), - }, - Result: []*user.IDPLink{}, - }, + wantErr: true, }, { name: "list idp links, org, ok", diff --git a/internal/api/grpc/user/v2/passkey.go b/internal/api/grpc/user/v2/passkey.go index bf539e252..145c1e571 100644 --- a/internal/api/grpc/user/v2/passkey.go +++ b/internal/api/grpc/user/v2/passkey.go @@ -142,8 +142,7 @@ func (s *Server) ListPasskeys(ctx context.Context, req *user.ListPasskeysRequest if err != nil { return nil, err } - authMethods, err := s.query.SearchUserAuthMethods(ctx, query, false) - authMethods.RemoveNoPermission(ctx, s.checkPermission) + authMethods, err := s.query.SearchUserAuthMethods(ctx, query, s.checkPermission) if err != nil { return nil, err } diff --git a/internal/api/grpc/user/v2/passkey_integration_test.go b/internal/api/grpc/user/v2/passkey_integration_test.go index 027005c43..12d3a6622 100644 --- a/internal/api/grpc/user/v2/passkey_integration_test.go +++ b/internal/api/grpc/user/v2/passkey_integration_test.go @@ -471,6 +471,16 @@ func TestServer_ListPasskeys(t *testing.T) { want *user.ListPasskeysResponse wantErr bool }{ + { + name: "list passkeys, no userID", + args: args{ + IamCTX, + &user.ListPasskeysRequest{ + UserId: "", + }, + }, + wantErr: true, + }, { name: "list passkeys, no permission", args: args{ @@ -479,18 +489,12 @@ func TestServer_ListPasskeys(t *testing.T) { UserId: userIDVerified, }, }, - want: &user.ListPasskeysResponse{ - Details: &object.ListDetails{ - TotalResult: 0, - Timestamp: timestamppb.Now(), - }, - Result: []*user.Passkey{}, - }, + wantErr: true, }, { name: "list passkeys, none", args: args{ - UserCTX, + IamCTX, &user.ListPasskeysRequest{ UserId: userIDWithout, }, @@ -506,7 +510,7 @@ func TestServer_ListPasskeys(t *testing.T) { { name: "list passkeys, registered", args: args{ - UserCTX, + IamCTX, &user.ListPasskeysRequest{ UserId: userIDRegistered, }, diff --git a/internal/api/grpc/user/v2/query.go b/internal/api/grpc/user/v2/query.go index d40e4d47d..564d4c1c0 100644 --- a/internal/api/grpc/user/v2/query.go +++ b/internal/api/grpc/user/v2/query.go @@ -6,7 +6,6 @@ import ( "github.com/muhlemmer/gu" "google.golang.org/protobuf/types/known/timestamppb" - "github.com/zitadel/zitadel/internal/api/authz" "github.com/zitadel/zitadel/internal/api/grpc/object/v2" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/query" @@ -15,15 +14,10 @@ import ( ) func (s *Server) GetUserByID(ctx context.Context, req *user.GetUserByIDRequest) (_ *user.GetUserByIDResponse, err error) { - resp, err := s.query.GetUserByID(ctx, true, req.GetUserId()) + resp, err := s.query.GetUserByIDWithPermission(ctx, true, req.GetUserId(), s.checkPermission) if err != nil { return nil, err } - if authz.GetCtxData(ctx).UserID != req.GetUserId() { - if err := s.checkPermission(ctx, domain.PermissionUserRead, resp.ResourceOwner, req.GetUserId()); err != nil { - return nil, err - } - } return &user.GetUserByIDResponse{ Details: object.DomainToDetailsPb(&domain.ObjectDetails{ Sequence: resp.Sequence, diff --git a/internal/api/grpc/user/v2/user.go b/internal/api/grpc/user/v2/user.go index 9ad83dfea..e46f6d3cb 100644 --- a/internal/api/grpc/user/v2/user.go +++ b/internal/api/grpc/user/v2/user.go @@ -421,7 +421,7 @@ func (s *Server) checkLinkedExternalUser(ctx context.Context, idpID, externalUse queries := []query.SearchQuery{ idQuery, externalIDQuery, } - links, err := s.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, false) + links, err := s.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, nil) if err != nil { return "", err } diff --git a/internal/api/grpc/user/v2beta/query.go b/internal/api/grpc/user/v2beta/query.go index 28e0a0c2e..4567259d1 100644 --- a/internal/api/grpc/user/v2beta/query.go +++ b/internal/api/grpc/user/v2beta/query.go @@ -6,7 +6,6 @@ import ( "github.com/muhlemmer/gu" "google.golang.org/protobuf/types/known/timestamppb" - "github.com/zitadel/zitadel/internal/api/authz" object "github.com/zitadel/zitadel/internal/api/grpc/object/v2beta" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/query" @@ -15,15 +14,10 @@ import ( ) func (s *Server) GetUserByID(ctx context.Context, req *user.GetUserByIDRequest) (_ *user.GetUserByIDResponse, err error) { - resp, err := s.query.GetUserByID(ctx, true, req.GetUserId()) + resp, err := s.query.GetUserByIDWithPermission(ctx, true, req.GetUserId(), s.checkPermission) if err != nil { return nil, err } - if authz.GetCtxData(ctx).UserID != req.GetUserId() { - if err := s.checkPermission(ctx, domain.PermissionUserRead, resp.ResourceOwner, req.GetUserId()); err != nil { - return nil, err - } - } return &user.GetUserByIDResponse{ Details: object.DomainToDetailsPb(&domain.ObjectDetails{ Sequence: resp.Sequence, diff --git a/internal/api/grpc/user/v2beta/user.go b/internal/api/grpc/user/v2beta/user.go index 8e3151a0b..b6029a0d6 100644 --- a/internal/api/grpc/user/v2beta/user.go +++ b/internal/api/grpc/user/v2beta/user.go @@ -434,7 +434,7 @@ func (s *Server) checkLinkedExternalUser(ctx context.Context, idpID, externalUse queries := []query.SearchQuery{ idQuery, externalIDQuery, } - links, err := s.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, false) + links, err := s.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, nil) if err != nil { return "", err } diff --git a/internal/api/idp/idp.go b/internal/api/idp/idp.go index b058ba5c2..3d46f029d 100644 --- a/internal/api/idp/idp.go +++ b/internal/api/idp/idp.go @@ -420,7 +420,7 @@ func (h *Handler) checkExternalUser(ctx context.Context, idpID, externalUserID s queries := []query.SearchQuery{ idQuery, externalIDQuery, } - links, err := h.queries.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, false) + links, err := h.queries.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, nil) if err != nil { return "", err } diff --git a/internal/api/ui/login/custom_action.go b/internal/api/ui/login/custom_action.go index 7beda6133..6e8054943 100644 --- a/internal/api/ui/login/custom_action.go +++ b/internal/api/ui/login/custom_action.go @@ -479,7 +479,7 @@ func (l *Login) resourceOwnerOfUserIDPLink(ctx context.Context, idpConfigID stri queries := []query.SearchQuery{ idQuery, externalIDQuery, } - links, err := l.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, false) + links, err := l.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, nil) if err != nil { return "", err } diff --git a/internal/api/ui/login/external_provider_handler.go b/internal/api/ui/login/external_provider_handler.go index a462f06a7..b9798bbcd 100644 --- a/internal/api/ui/login/external_provider_handler.go +++ b/internal/api/ui/login/external_provider_handler.go @@ -846,7 +846,7 @@ func (l *Login) updateExternalUsername(ctx context.Context, user *query.User, ex if err != nil { return err } - links, err := l.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: []query.SearchQuery{externalIDQuery, idpIDQuery, userIDQuery}}, false) + links, err := l.query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: []query.SearchQuery{externalIDQuery, idpIDQuery, userIDQuery}}, nil) if err != nil || len(links.Links) == 0 { return err } @@ -1326,6 +1326,6 @@ func (l *Login) getUserLinks(ctx context.Context, userID, idpID string) (*query. userIDQuery, idpIDQuery, }, - }, false, + }, nil, ) } diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request.go b/internal/auth/repository/eventsourcing/eventstore/auth_request.go index bef602ff0..c49ade2a0 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request.go @@ -100,7 +100,7 @@ type idpProviderViewProvider interface { } type idpUserLinksProvider interface { - IDPUserLinks(ctx context.Context, queries *query.IDPUserLinksSearchQuery, withOwnerRemoved bool) (*query.IDPUserLinks, error) + IDPUserLinks(ctx context.Context, queries *query.IDPUserLinksSearchQuery, permissionCheck domain.PermissionCheck) (*query.IDPUserLinks, error) } type userEventProvider interface { @@ -1000,7 +1000,7 @@ func (repo *AuthRequestRepo) checkExternalUserLogin(ctx context.Context, request } queries = append(queries, orgIDQuery) } - links, err := repo.Query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, false) + links, err := repo.Query.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: queries}, nil) if err != nil { return err } @@ -1200,7 +1200,7 @@ func checkExternalIDPsOfUser(ctx context.Context, idpUserLinksProvider idpUserLi if err != nil { return nil, err } - return idpUserLinksProvider.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: []query.SearchQuery{userIDQuery}}, false) + return idpUserLinksProvider.IDPUserLinks(ctx, &query.IDPUserLinksSearchQuery{Queries: []query.SearchQuery{userIDQuery}}, nil) } func (repo *AuthRequestRepo) usersForUserSelection(ctx context.Context, request *domain.AuthRequest) ([]domain.UserSelection, error) { diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go b/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go index 7308d6ce1..14d495582 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go @@ -298,7 +298,7 @@ type mockIDPUserLinks struct { idps []*query.IDPUserLink } -func (m *mockIDPUserLinks) IDPUserLinks(ctx context.Context, queries *query.IDPUserLinksSearchQuery, withOwnerRemoved bool) (*query.IDPUserLinks, error) { +func (m *mockIDPUserLinks) IDPUserLinks(ctx context.Context, queries *query.IDPUserLinksSearchQuery, permissionCheck domain.PermissionCheck) (*query.IDPUserLinks, error) { return &query.IDPUserLinks{Links: m.idps}, nil } diff --git a/internal/query/idp_user_link.go b/internal/query/idp_user_link.go index 4f13d9d31..5caf6c664 100644 --- a/internal/query/idp_user_link.go +++ b/internal/query/idp_user_link.go @@ -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) }() diff --git a/internal/query/idp_user_link_test.go b/internal/query/idp_user_link_test.go index bcbe6c206..b8ba2d087 100644 --- a/internal/query/idp_user_link_test.go +++ b/internal/query/idp_user_link_test.go @@ -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,` + diff --git a/internal/query/org_test.go b/internal/query/org_test.go index b8c6073db..db41f9ffd 100644 --- a/internal/query/org_test.go +++ b/internal/query/org_test.go @@ -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 } diff --git a/internal/query/user.go b/internal/query/user.go index 497cd89d6..415e50aae 100644 --- a/internal/query/user.go +++ b/internal/query/user.go @@ -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) }() diff --git a/internal/query/user_auth_method.go b/internal/query/user_auth_method.go index 6a93f069b..ce919a912 100644 --- a/internal/query/user_auth_method.go +++ b/internal/query/user_auth_method.go @@ -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) }() diff --git a/internal/query/user_auth_method_test.go b/internal/query/user_auth_method_test.go index 95f9a0229..7221129b0 100644 --- a/internal/query/user_auth_method_test.go +++ b/internal/query/user_auth_method_test.go @@ -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,` + diff --git a/internal/query/user_test.go b/internal/query/user_test.go index 0a6b0c36e..89556d41e 100644 --- a/internal/query/user_test.go +++ b/internal/query/user_test.go @@ -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` +