mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
Merge commit from fork
* fix(security): check auth method permission on the user's organization ListUserAuthMethodTypes checked user.read against the caller's own organization instead of the organization owning the requested user. Any org level role holding user.read - ORG_OWNER, ORG_OWNER_VIEWER - therefore passed the check for every user of the instance, and the query itself filters on instance and user id only, so it returned that user's authentication methods across the organization boundary. Resolve the requested user first and check the permission on its resource owner, the same resolution GetUserByIDWithPermission already uses. This covers both the v2 and v2beta ListAuthenticationMethodTypes handlers, which share this function. The two login UI callers scope the context to the user itself, so they keep skipping the check and pay no extra query. Note that a request for a non existing user id now returns NotFound instead of an empty list, which also removes an existence oracle. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MeUQhL2hxvnYbPn3qhWJAL * test(user/v2): cover cross-organization auth method enumeration TestServer_ListAuthenticationMethodTypes_CrossOrg fails without the preceding fix: an ORG_OWNER and an ORG_OWNER_VIEWER of one organization could list the authentication methods of a user of another organization, leaking AUTHENTICATION_METHOD_TYPE_PASSWORD. The two own-org subtests are positive controls, not decoration. Memberships and auth methods are projected asynchronously, so without them a denial could pass for the wrong reason - a membership that has not been projected yet. The IAM_USER_MANAGER subtest pins that instance wide user.read keeps working across organizations, so the fix narrows the organization without narrowing the role set. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MeUQhL2hxvnYbPn3qhWJAL --------- Co-authored-by: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
dbbab75420
commit
f3dcfa3db6
@@ -2215,6 +2215,83 @@ func TestServer_ListAuthenticationMethodTypes(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestServer_ListAuthenticationMethodTypes_CrossOrg asserts that an org owner - or the read only
|
||||||
|
// org owner viewer - of one organization cannot list the authentication methods of a user of
|
||||||
|
// another organization. A leak shows up as AUTHENTICATION_METHOD_TYPE_PASSWORD.
|
||||||
|
func TestServer_ListAuthenticationMethodTypes_CrossOrg(t *testing.T) {
|
||||||
|
// The victim lives in another organization than the callers and has a password set, so a
|
||||||
|
// successful cross-org read shows up as a leaked AUTHENTICATION_METHOD_TYPE_PASSWORD.
|
||||||
|
orgB := Instance.CreateOrganization(IamCTX, integration.OrganizationName(), integration.Email())
|
||||||
|
victimID := Instance.CreateHumanUserVerified(
|
||||||
|
IamCTX, orgB.GetOrganizationId(), integration.Email(), integration.Phone(),
|
||||||
|
).GetUserId()
|
||||||
|
Instance.SetUserPassword(IamCTX, victimID, integration.UserPassword, false)
|
||||||
|
|
||||||
|
// The bystander lives in the callers' own organization and is the target of the positive
|
||||||
|
// controls below.
|
||||||
|
bystanderID := Instance.CreateHumanUser(OrgCTX).GetUserId()
|
||||||
|
Instance.SetUserPassword(OrgCTX, bystanderID, integration.UserPassword, false)
|
||||||
|
|
||||||
|
// Both callers hold their role in the default org only. OrgCTX is already an ORG_OWNER there.
|
||||||
|
_, viewerPAT, err := Instance.CreateMachineUserPATWithMembership(OrgCTX, "ORG_OWNER_VIEWER")
|
||||||
|
require.NoError(t, err)
|
||||||
|
viewerCTX := integration.WithAuthorizationToken(CTX, viewerPAT)
|
||||||
|
|
||||||
|
// requireReadsPassword asserts the caller sees the user's password method. Used for the
|
||||||
|
// positive controls, which are not decoration: memberships and auth methods are projected
|
||||||
|
// asynchronously, so without them a denial below could be a false pass caused by a
|
||||||
|
// membership that has not been projected yet.
|
||||||
|
requireReadsPassword := func(t *testing.T, ctx context.Context, userID string) {
|
||||||
|
retryDuration, tick := integration.WaitForAndTickWithMaxDuration(ctx, time.Minute)
|
||||||
|
require.EventuallyWithT(t, func(ttt *assert.CollectT) {
|
||||||
|
got, err := Client.ListAuthenticationMethodTypes(ctx, &user.ListAuthenticationMethodTypesRequest{
|
||||||
|
UserId: userID,
|
||||||
|
})
|
||||||
|
require.NoError(ttt, err)
|
||||||
|
assert.Equal(ttt, []user.AuthenticationMethodType{
|
||||||
|
user.AuthenticationMethodType_AUTHENTICATION_METHOD_TYPE_PASSWORD,
|
||||||
|
}, got.GetAuthMethodTypes())
|
||||||
|
}, retryDuration, tick, "timeout waiting for expected auth methods result")
|
||||||
|
}
|
||||||
|
|
||||||
|
// requireDeniedVictim reproduces the reported request. No retry: the permission check runs
|
||||||
|
// before the query, so a denial is immediate and not eventually consistent.
|
||||||
|
requireDeniedVictim := func(t *testing.T, ctx context.Context) {
|
||||||
|
got, err := Client.ListAuthenticationMethodTypes(ctx, &user.ListAuthenticationMethodTypesRequest{
|
||||||
|
UserId: victimID,
|
||||||
|
})
|
||||||
|
require.Errorf(t, err, "cross-org read succeeded and leaked auth methods: %v", got.GetAuthMethodTypes())
|
||||||
|
// The legacy permission path reports "membership not found" (NotFound) because the
|
||||||
|
// caller has no membership in the victim's org at all; the v2 permission check reports
|
||||||
|
// PermissionDenied. Either is a correct denial.
|
||||||
|
assert.Contains(t, []codes.Code{codes.NotFound, codes.PermissionDenied}, status.Code(err))
|
||||||
|
}
|
||||||
|
|
||||||
|
t.Run("org owner reads a user in its own org", func(t *testing.T) {
|
||||||
|
requireReadsPassword(t, OrgCTX, bystanderID)
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("org owner viewer reads a user in its own org", func(t *testing.T) {
|
||||||
|
requireReadsPassword(t, viewerCTX, bystanderID)
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("org owner is denied a user in another org", func(t *testing.T) {
|
||||||
|
requireDeniedVictim(t, OrgCTX)
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("org owner viewer is denied a user in another org", func(t *testing.T) {
|
||||||
|
requireDeniedVictim(t, viewerCTX)
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("instance wide user manager is allowed", func(t *testing.T) {
|
||||||
|
// IAM_USER_MANAGER holds user.read instance wide, so it must keep working across
|
||||||
|
// organizations - the check narrows the org, it must not narrow the role set.
|
||||||
|
_, pat, err := Instance.CreateMachineUserPATWithMembership(IamCTX, "IAM_USER_MANAGER")
|
||||||
|
require.NoError(t, err)
|
||||||
|
requireReadsPassword(t, integration.WithAuthorizationToken(CTX, pat), victimID)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
func TestServer_ListAuthenticationFactors(t *testing.T) {
|
func TestServer_ListAuthenticationFactors(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
|
|||||||
@@ -194,7 +194,12 @@ func (q *Queries) searchUserAuthMethods(ctx context.Context, queries *UserAuthMe
|
|||||||
func (q *Queries) ListUserAuthMethodTypes(ctx context.Context, userID string, activeOnly bool, includeWithoutDomain bool, queryDomain string) (userAuthMethodTypes *AuthMethodTypes, err error) {
|
func (q *Queries) ListUserAuthMethodTypes(ctx context.Context, userID string, activeOnly bool, includeWithoutDomain bool, queryDomain string) (userAuthMethodTypes *AuthMethodTypes, err error) {
|
||||||
ctxData := authz.GetCtxData(ctx)
|
ctxData := authz.GetCtxData(ctx)
|
||||||
if ctxData.UserID != userID {
|
if ctxData.UserID != userID {
|
||||||
if err := q.checkPermission(ctx, domain.PermissionUserRead, ctxData.OrgID, userID); err != nil {
|
// Check against the organization owning the user, not the caller's own organization.
|
||||||
|
user, err := q.GetUserByID(ctx, false, userID)
|
||||||
|
if err != nil {
|
||||||
|
return nil, err
|
||||||
|
}
|
||||||
|
if err := q.checkPermission(ctx, domain.PermissionUserRead, user.ResourceOwner, userID); err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user