From 314d88252d7d0f2549714b3cc25640bd612b7a88 Mon Sep 17 00:00:00 2001 From: Eduard Gert Date: Mon, 21 Sep 2026 10:34:04 +0200 Subject: [PATCH] [management] Name the account owner in the pending approval error (#7533) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Name the account owner in the pending approval error A user refused because their account is pending approval had no way to learn who could approve them. The refusal now carries the account owner's address, masked, so a caller can name someone to contact without being handed the address itself. Resolving the owner is best effort: a lookup failure, or an account predating the stored email, falls back to the refusal as it was. * Name only the caller's own owner in the pending approval error The refusal is raised before ValidateAccountAccess has established that the caller belongs to the account the request asked about, and the user is loaded by ID alone. Resolving the owner of the requested account therefore disclosed that owner's address to a pending user with no claim to it, reachable through any handler that takes an account ID from the caller — DELETE /api/accounts/{accountId} passes one straight through. The owner who can approve a pending user is the owner of their own account, so resolve that one. The requested account is never read. * Mask short local parts whole in MaskedEmail Keeping the first two characters and the last hides nothing until the local part is four long: at three or fewer they are the whole of it, so "abc@example.com" masked to "ab****c@example.com" and a pending user could recover the owner's address in full from what is meant to conceal it. Short local parts are now replaced entirely. * Name the owner from GetCurrentUserInfo instead of the permission gate The gate could only read the stored user row, which carries no address when an external IdP owns the identities — the usual case — so it named no one in practice. It also had no way to reach the IdP without being handed the account manager, which meant restoring bootstrap wiring that a refactor had dropped. GetCurrentUserInfo already holds that account manager, so it answers for a pending user itself and reuses GetOwnerInfo, the same lookup /msp uses to resolve an owner's address. The gate returns to exactly what it was, and with it goes the risk of naming the owner of an account the caller only asked about. MaskedEmail becomes MaskEmail: with a UserInfo in hand there is no stored row to hang it off. * [management] Cover the pending approval refusal in GetCurrentUserInfo The branch that names the owner had no coverage at the manager level, so neither the named refusal nor the fallback for an owner without a resolvable address was pinned down. * [management] Cover the failed owner lookup in the pending approval refusal The generic fallback has two ways in: no address on the resolved owner, and no owner to resolve at all. Only the first was pinned down. * [management] Pin the owner lookup to the caller's own account A mismatched account claim must not steer which owner the refusal names, and a blocked user is still answered before the claim is validated. Both are load bearing and neither was covered. --- management/server/types/user.go | 19 ++++ management/server/types/user_test.go | 141 +++++++++++++++++++++++++++ management/server/user.go | 27 +++++ management/server/user_test.go | 64 ++++++++++++ shared/management/status/error.go | 5 + 5 files changed, 256 insertions(+) diff --git a/management/server/types/user.go b/management/server/types/user.go index 02358ebc2..bebc20ea4 100644 --- a/management/server/types/user.go +++ b/management/server/types/user.go @@ -285,6 +285,25 @@ func (u *User) EncryptSensitiveData(enc *crypt.FieldEncrypt) error { return nil } +func MaskEmail(email string) string { + local, domain, found := strings.Cut(email, "@") + if !found || local == "" || domain == "" { + return "" + } + + // Runes, not bytes, so a non-ASCII local part is not cut mid-character. + runes := []rune(local) + + // Keeping the first two and the last needs a local part of at least four to + // hide anything at all: at three or fewer those are the whole of it, and the + // address would be recoverable in full from what is meant to conceal it. + if len(runes) < 4 { + return "****@" + domain + } + + return string(runes[:2]) + "****" + string(runes[len(runes)-1]) + "@" + domain +} + // DecryptSensitiveData decrypts the user's sensitive fields (Email and Name) in place. func (u *User) DecryptSensitiveData(enc *crypt.FieldEncrypt) error { if enc == nil { diff --git a/management/server/types/user_test.go b/management/server/types/user_test.go index e11df96aa..1b3ce7ce6 100644 --- a/management/server/types/user_test.go +++ b/management/server/types/user_test.go @@ -296,3 +296,144 @@ func TestUser_EncryptDecryptRoundTrip(t *testing.T) { }) } } + +func TestMaskEmail(t *testing.T) { + testCases := []struct { + name string + email string + expected string + }{ + { + name: "ordinary address keeps the first two, the last, and the domain", + email: "admin@example.com", + expected: "ad****n@example.com", + }, + { + name: "four characters is the shortest local part that reveals anything", + email: "abcd@example.com", + expected: "ab****d@example.com", + }, + { + name: "three character local part is masked whole, since a lead and tail would be all of it", + email: "abc@example.com", + expected: "****@example.com", + }, + { + name: "two character local part is masked whole", + email: "ab@example.com", + expected: "****@example.com", + }, + { + name: "single character local part is masked whole", + email: "a@b.co", + expected: "****@b.co", + }, + { + name: "mask width does not report the length it stands in for", + email: "a.very.long.local.part@example.com", + expected: "a.****t@example.com", + }, + { + name: "a local part far longer than the mask is still reduced to three characters", + email: "finance.department.notifications.owner.account@example.com", + expected: "fi****t@example.com", + }, + { + name: "plus addressing is masked along with the rest of the local part", + email: "admin+netbird@example.com", + expected: "ad****d@example.com", + }, + { + name: "separators inside the local part are not treated specially", + email: "first.last-name_x@example.com", + expected: "fi****x@example.com", + }, + { + name: "case is preserved rather than normalised", + email: "Admin@Example.COM", + expected: "Ad****n@Example.COM", + }, + { + name: "subdomains stay intact", + email: "owner@mail.corp.example.com", + expected: "ow****r@mail.corp.example.com", + }, + { + name: "german umlauts count as single characters", + email: "müller@example.de", + expected: "mü****r@example.de", + }, + { + name: "cyrillic local part is cut on runes", + email: "иванов@example.ru", + expected: "ив****в@example.ru", + }, + { + name: "cjk local part of three runes is masked whole, counted in runes not bytes", + email: "用户名@example.cn", + expected: "****@example.cn", + }, + { + name: "cjk local part of four runes reveals the first two and the last", + email: "用户名字@example.cn", + expected: "用户****字@example.cn", + }, + { + name: "arabic local part is cut on runes", + email: "مستخدم@example.sa", + expected: "مس****م@example.sa", + }, + { + name: "two rune non-ascii local part is masked whole", + email: "ää@example.de", + expected: "****@example.de", + }, + { + name: "astral plane runes are not split into surrogates", + email: "a🎉bc@example.com", + expected: "a🎉****c@example.com", + }, + { + name: "a non-ascii domain is left alone", + email: "admin@münchen.example", + expected: "ad****n@münchen.example", + }, + { + name: "only the first separator splits, so a second stays in the domain", + email: "a@b@example.com", + expected: "****@b@example.com", + }, + { + name: "empty email has nothing to mask", + email: "", + expected: "", + }, + { + name: "value without a separator is not an address", + email: "not-an-email", + expected: "", + }, + { + name: "missing local part is not an address", + email: "@example.com", + expected: "", + }, + { + name: "missing domain is not an address", + email: "admin@", + expected: "", + }, + { + name: "a bare separator is not an address", + email: "@", + expected: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, MaskEmail(tc.email)) + }) + } + +} diff --git a/management/server/user.go b/management/server/user.go index 823c1b2e4..3510a624b 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -1448,6 +1448,25 @@ func (am *DefaultAccountManager) deleteRegularUser(ctx context.Context, accountI return updateAccountPeers, nil } +// pendingApprovalError refuses a user awaiting approval, naming the owner who +// can approve them when their address resolves. Failing to resolve one is not a +// reason to withhold the refusal, so the lookup is best effort. +func (am *DefaultAccountManager) pendingApprovalError(ctx context.Context, accountID string) error { + owner, err := am.GetOwnerInfo(ctx, accountID) + if err != nil { + log.WithContext(ctx).Debugf("pending approval refusal: owner of account %s did not resolve: %v", accountID, err) + return status.NewUserPendingApprovalError() + } + + masked := types.MaskEmail(owner.Email) + if masked == "" { + log.WithContext(ctx).Debugf("pending approval refusal: no address found for the owner of account %s", accountID) + return status.NewUserPendingApprovalError() + } + + return status.NewUserPendingApprovalByOwnerError(masked) +} + // GetOwnerInfo retrieves the owner information for a given account ID. func (am *DefaultAccountManager) GetOwnerInfo(ctx context.Context, accountID string) (*types.UserInfo, error) { owner, err := am.Store.GetAccountOwner(ctx, store.LockingStrengthNone, accountID) @@ -1505,6 +1524,14 @@ func (am *DefaultAccountManager) GetCurrentUserInfo(ctx context.Context, userAut return nil, err } + // A user pending approval is blocked too, and the dashboard needs to tell + // the two apart: one is a dead end, the other resolves by itself once the + // owner acts. Naming that owner needs the address the IdP holds, which is + // why this is answered here rather than in the permission gate. + if user.IsBlocked() && user.PendingApproval { + return nil, am.pendingApprovalError(ctx, user.AccountID) + } + if user.IsBlocked() { return nil, status.NewUserBlockedError() } diff --git a/management/server/user_test.go b/management/server/user_test.go index ec0bbc54e..2d1a5f1e9 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -1779,6 +1779,42 @@ func TestDefaultAccountManager_GetCurrentUserInfo(t *testing.T) { } require.NoError(t, store.SaveAccount(context.Background(), account2)) + account3 := newAccountWithId(context.Background(), "account3", "account3Owner", "", "owner@example.com", "", false) + account3.Users["pending-user"] = &types.User{ + Id: "pending-user", + AccountID: account3.Id, + Role: types.UserRoleUser, + Blocked: true, + PendingApproval: true, + } + require.NoError(t, store.SaveAccount(context.Background(), account3)) + + // The owner has no address to name, so the refusal falls back to the generic one. + account4 := newAccountWithId(context.Background(), "account4", "account4Owner", "", "", "", false) + account4.Users["pending-user-without-owner-email"] = &types.User{ + Id: "pending-user-without-owner-email", + AccountID: account4.Id, + Role: types.UserRoleUser, + Blocked: true, + PendingApproval: true, + } + require.NoError(t, store.SaveAccount(context.Background(), account4)) + + // No user holds the owner role, so the owner lookup itself fails. + account5 := newAccountWithId(context.Background(), "account5", "account5Admin", "", "", "", false) + account5.Users["account5Admin"].Role = types.UserRoleAdmin + account5.Users["pending-user-without-owner"] = &types.User{ + Id: "pending-user-without-owner", + AccountID: account5.Id, + Role: types.UserRoleUser, + Blocked: true, + PendingApproval: true, + } + require.NoError(t, store.SaveAccount(context.Background(), account5)) + + account6 := newAccountWithId(context.Background(), "account6", "account6Owner", "", "stranger@example.com", "", false) + require.NoError(t, store.SaveAccount(context.Background(), account6)) + permissionsManager := permissions.NewManager(store) am := DefaultAccountManager{ Store: store, @@ -1812,6 +1848,34 @@ func TestDefaultAccountManager_GetCurrentUserInfo(t *testing.T) { userAuth: auth.UserAuth{AccountId: account1.Id, UserId: "service-user"}, expectedErr: status.NewPermissionDeniedError(), }, + { + name: "pending approval names the owner", + userAuth: auth.UserAuth{AccountId: account3.Id, UserId: "pending-user"}, + expectedErr: status.NewUserPendingApprovalByOwnerError("ow****r@example.com"), + }, + { + name: "pending approval without an owner address", + userAuth: auth.UserAuth{AccountId: account4.Id, UserId: "pending-user-without-owner-email"}, + expectedErr: status.NewUserPendingApprovalError(), + }, + { + name: "pending approval without an owner", + userAuth: auth.UserAuth{AccountId: account5.Id, UserId: "pending-user-without-owner"}, + expectedErr: status.NewUserPendingApprovalError(), + }, + { + // The account claim points at an account the caller is not in. The + // owner named has to be the one of the account holding the caller's + // own record, never the one the claim asks for. + name: "pending approval ignores a mismatched account claim", + userAuth: auth.UserAuth{AccountId: account6.Id, UserId: "pending-user"}, + expectedErr: status.NewUserPendingApprovalByOwnerError("ow****r@example.com"), + }, + { + name: "blocked user answers before the account claim is validated", + userAuth: auth.UserAuth{AccountId: account6.Id, UserId: "blocked-user"}, + expectedErr: status.NewUserBlockedError(), + }, { name: "owner user", userAuth: auth.UserAuth{AccountId: account1.Id, UserId: "account1Owner"}, diff --git a/shared/management/status/error.go b/shared/management/status/error.go index e31663450..4249f27de 100644 --- a/shared/management/status/error.go +++ b/shared/management/status/error.go @@ -135,6 +135,11 @@ func NewUserPendingApprovalError() error { return Errorf(PermissionDenied, "user is pending approval") } +// NewUserPendingApprovalByOwnerError creates a new Error with PermissionDenied type for a blocked user pending approval, naming the masked address of the owner who can approve them +func NewUserPendingApprovalByOwnerError(ownerEmail string) error { + return Errorf(PermissionDenied, "user is pending approval by owner %s", ownerEmail) +} + // NewPeerNotRegisteredError creates a new Error with Unauthenticated type unregistered peer func NewPeerNotRegisteredError() error { return Errorf(Unauthenticated, "peer is not registered")