mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-29 19:11:28 +02:00
[management] Split ValidateSession into named checks
The account status gate pushed ValidateSession to 115 lines of code, past the 100 the quality gate allows. Session token verification and the checks that run against a user of the service's own account move into named helpers, and the denials raised before a user is resolved share one constructor. An account mismatch keeps its bare response: the user belongs to a different account, so none of their identity belongs in an answer to this service's proxy. Tests assert denied reasons through the package constants, with the wire values pinned in one place so a rename has to be deliberate.
This commit is contained in:
@@ -1732,10 +1732,7 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val
|
||||
sessionToken := req.GetSessionToken()
|
||||
|
||||
if domain == "" || sessionToken == "" {
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: false,
|
||||
DeniedReason: "missing domain or session_token",
|
||||
}, nil
|
||||
return deniedSessionResponse("missing domain or session_token"), nil
|
||||
}
|
||||
|
||||
service, err := s.getServiceByDomain(ctx, domain)
|
||||
@@ -1745,40 +1742,16 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val
|
||||
"error": err.Error(),
|
||||
}).Debug("ValidateSession: service not found")
|
||||
//nolint:nilerr
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: false,
|
||||
DeniedReason: "service_not_found",
|
||||
}, nil
|
||||
return deniedSessionResponse("service_not_found"), nil
|
||||
}
|
||||
|
||||
if err := enforceAccountScope(ctx, service.AccountID); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
pubKeyBytes, err := base64.StdEncoding.DecodeString(service.SessionPublicKey)
|
||||
if err != nil {
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
"error": err.Error(),
|
||||
}).Error("ValidateSession: decode public key")
|
||||
//nolint:nilerr
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: false,
|
||||
DeniedReason: "invalid_service_config",
|
||||
}, nil
|
||||
}
|
||||
|
||||
userID, _, _, _, _, err := proxyauth.ValidateSessionJWT(sessionToken, domain, pubKeyBytes)
|
||||
if err != nil {
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
"error": err.Error(),
|
||||
}).Debug("ValidateSession: invalid session token")
|
||||
//nolint:nilerr
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: false,
|
||||
DeniedReason: "invalid_token",
|
||||
}, nil
|
||||
userID, reason := sessionTokenSubject(domain, service, sessionToken)
|
||||
if reason != "" {
|
||||
return deniedSessionResponse(reason), nil
|
||||
}
|
||||
|
||||
user, userGroups, err := s.usersManager.GetUserWithGroups(ctx, userID)
|
||||
@@ -1789,12 +1762,11 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val
|
||||
"error": err,
|
||||
}).Debug("ValidateSession: user not found")
|
||||
//nolint:nilerr
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: false,
|
||||
DeniedReason: deniedReasonUserNotFound,
|
||||
}, nil
|
||||
return deniedSessionResponse(deniedReasonUserNotFound), nil
|
||||
}
|
||||
|
||||
// A user from another account gets a bare response: none of their identity
|
||||
// belongs in an answer to a proxy serving a different account.
|
||||
if user.AccountID != service.AccountID {
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
@@ -1802,20 +1774,12 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val
|
||||
"user_account": user.AccountID,
|
||||
"service_account": service.AccountID,
|
||||
}).Debug("ValidateSession: user account mismatch")
|
||||
//nolint:nilerr
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: false,
|
||||
DeniedReason: "account_mismatch",
|
||||
}, nil
|
||||
return deniedSessionResponse("account_mismatch"), nil
|
||||
}
|
||||
|
||||
if reason := userStatusDeniedReason(user); reason != "" {
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
"user_id": userID,
|
||||
"reason": reason,
|
||||
}).Debug("ValidateSession: user status denies access")
|
||||
groupIDs, groupNames := pairGroupIDsAndNames(userGroups)
|
||||
groupIDs, groupNames := pairGroupIDsAndNames(userGroups)
|
||||
|
||||
if reason := s.accountUserDeniedReason(domain, service, user); reason != "" {
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: false,
|
||||
UserId: user.Id,
|
||||
@@ -1826,31 +1790,12 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val
|
||||
}, nil
|
||||
}
|
||||
|
||||
if err := s.checkGroupAccess(service, user); err != nil {
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
"user_id": userID,
|
||||
"error": err.Error(),
|
||||
}).Debug("ValidateSession: access denied")
|
||||
groupIDs, groupNames := pairGroupIDsAndNames(userGroups)
|
||||
//nolint:nilerr
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: false,
|
||||
UserId: user.Id,
|
||||
UserEmail: user.Email,
|
||||
DeniedReason: "not_in_group",
|
||||
PeerGroupIds: groupIDs,
|
||||
PeerGroupNames: groupNames,
|
||||
}, nil
|
||||
}
|
||||
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
"user_id": userID,
|
||||
"email": user.Email,
|
||||
}).Debug("ValidateSession: access granted")
|
||||
|
||||
groupIDs, groupNames := pairGroupIDsAndNames(userGroups)
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: true,
|
||||
UserId: user.Id,
|
||||
@@ -1860,6 +1805,66 @@ func (s *ProxyServiceServer) ValidateSession(ctx context.Context, req *proto.Val
|
||||
}, nil
|
||||
}
|
||||
|
||||
// deniedSessionResponse builds a denial that carries no identity, for the
|
||||
// checks that run before a user of this service's account is resolved.
|
||||
func deniedSessionResponse(reason string) *proto.ValidateSessionResponse {
|
||||
return &proto.ValidateSessionResponse{
|
||||
Valid: false,
|
||||
DeniedReason: reason,
|
||||
}
|
||||
}
|
||||
|
||||
// sessionTokenSubject verifies the session token against the service's session
|
||||
// key and returns the user it was minted for, or the reason it cannot be
|
||||
// trusted.
|
||||
func sessionTokenSubject(domain string, service *rpservice.Service, sessionToken string) (userID, deniedReason string) {
|
||||
pubKeyBytes, err := base64.StdEncoding.DecodeString(service.SessionPublicKey)
|
||||
if err != nil {
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
"error": err.Error(),
|
||||
}).Error("ValidateSession: decode public key")
|
||||
return "", "invalid_service_config"
|
||||
}
|
||||
|
||||
userID, _, _, _, _, err = proxyauth.ValidateSessionJWT(sessionToken, domain, pubKeyBytes)
|
||||
if err != nil {
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
"error": err.Error(),
|
||||
}).Debug("ValidateSession: invalid session token")
|
||||
return "", "invalid_token"
|
||||
}
|
||||
|
||||
return userID, ""
|
||||
}
|
||||
|
||||
// accountUserDeniedReason gates a user of the service's own account, returning
|
||||
// an empty string when access is granted. Account status comes before group
|
||||
// membership: a user awaiting approval or blocked has no access regardless of
|
||||
// the groups they were auto-assigned.
|
||||
func (s *ProxyServiceServer) accountUserDeniedReason(domain string, service *rpservice.Service, user *types.User) string {
|
||||
if reason := userStatusDeniedReason(user); reason != "" {
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
"user_id": user.Id,
|
||||
"reason": reason,
|
||||
}).Debug("ValidateSession: user status denies access")
|
||||
return reason
|
||||
}
|
||||
|
||||
if err := s.checkGroupAccess(service, user); err != nil {
|
||||
log.WithFields(log.Fields{
|
||||
"domain": domain,
|
||||
"user_id": user.Id,
|
||||
"error": err.Error(),
|
||||
}).Debug("ValidateSession: access denied")
|
||||
return "not_in_group"
|
||||
}
|
||||
|
||||
return ""
|
||||
}
|
||||
|
||||
func (s *ProxyServiceServer) getServiceByDomain(ctx context.Context, domain string) (*rpservice.Service, error) {
|
||||
service, err := s.serviceManager.GetServiceByDomain(ctx, domain)
|
||||
if err == nil {
|
||||
|
||||
@@ -559,7 +559,7 @@ func TestValidateTunnelPeerUserEmailEnrichment(t *testing.T) {
|
||||
expectEmail: "idp@example.com",
|
||||
expectUserID: userID,
|
||||
expectIdPHit: true,
|
||||
expectDeniedReason: "user_not_found",
|
||||
expectDeniedReason: deniedReasonUserNotFound,
|
||||
},
|
||||
{
|
||||
name: "unlinked peer uses peer name and never consults idp",
|
||||
@@ -625,6 +625,15 @@ func TestValidateTunnelPeerUserEmailEnrichment(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestDeniedReasonValues pins the wire values of the account status denied
|
||||
// reasons. The proxy logs them and operators filter access logs on them, so a
|
||||
// rename is a breaking change rather than an internal detail.
|
||||
func TestDeniedReasonValues(t *testing.T) {
|
||||
assert.Equal(t, "pending_approval", deniedReasonPendingApproval)
|
||||
assert.Equal(t, "user_blocked", deniedReasonUserBlocked)
|
||||
assert.Equal(t, "user_not_found", deniedReasonUserNotFound)
|
||||
}
|
||||
|
||||
// TestValidateTunnelPeerOwnerStatus verifies that the mesh fast-path gates on
|
||||
// the account status of the peer's owning user. A peer whose owner was blocked
|
||||
// after the peer registered must lose access, while an unlinked machine peer
|
||||
@@ -653,13 +662,13 @@ func TestValidateTunnelPeerOwnerStatus(t *testing.T) {
|
||||
name: "owner pending approval denied",
|
||||
peerUserID: userID,
|
||||
owner: &types.User{Id: userID, AccountID: accountID, Email: "user@example.com", Blocked: true, PendingApproval: true},
|
||||
expectDeniedReason: "pending_approval",
|
||||
expectDeniedReason: deniedReasonPendingApproval,
|
||||
},
|
||||
{
|
||||
name: "owner blocked after registering the peer denied",
|
||||
peerUserID: userID,
|
||||
owner: &types.User{Id: userID, AccountID: accountID, Email: "user@example.com", Blocked: true},
|
||||
expectDeniedReason: "user_blocked",
|
||||
expectDeniedReason: deniedReasonUserBlocked,
|
||||
},
|
||||
{
|
||||
name: "unlinked machine peer stays allowed",
|
||||
|
||||
@@ -245,7 +245,7 @@ func TestValidateSession_PendingApprovalUserDenied(t *testing.T) {
|
||||
|
||||
require.NoError(t, err)
|
||||
assert.False(t, resp.Valid, "User pending approval should be denied")
|
||||
assert.Equal(t, "pending_approval", resp.DeniedReason)
|
||||
assert.Equal(t, deniedReasonPendingApproval, resp.DeniedReason)
|
||||
assert.Equal(t, pendingUserID, resp.UserId)
|
||||
assert.Equal(t, []string{"allowedGroupId"}, resp.GetPeerGroupIds(), "PeerGroupIds must mirror the resolved user's group memberships on denial")
|
||||
assert.Equal(t, []string{"Allowed Group"}, resp.GetPeerGroupNames(), "PeerGroupNames must pair with PeerGroupIds on denial")
|
||||
@@ -270,7 +270,7 @@ func TestValidateSession_PendingApprovalUserInAllUsersGroupDenied(t *testing.T)
|
||||
|
||||
require.NoError(t, err)
|
||||
assert.False(t, resp.Valid, "User pending approval should be denied even in the All Users group")
|
||||
assert.Equal(t, "pending_approval", resp.DeniedReason)
|
||||
assert.Equal(t, deniedReasonPendingApproval, resp.DeniedReason)
|
||||
assert.Equal(t, pendingAllUsersID, resp.UserId)
|
||||
assert.Equal(t, []string{allUsersGroupID}, resp.GetPeerGroupIds(), "PeerGroupIds must mirror the resolved user's group memberships on denial")
|
||||
}
|
||||
@@ -293,7 +293,7 @@ func TestValidateSession_BlockedUserDenied(t *testing.T) {
|
||||
|
||||
require.NoError(t, err)
|
||||
assert.False(t, resp.Valid, "Blocked user should be denied")
|
||||
assert.Equal(t, "user_blocked", resp.DeniedReason)
|
||||
assert.Equal(t, deniedReasonUserBlocked, resp.DeniedReason)
|
||||
assert.Equal(t, blockedUserID, resp.UserId)
|
||||
}
|
||||
|
||||
@@ -318,7 +318,7 @@ func TestValidateSession_UserAllowedAfterApproval(t *testing.T) {
|
||||
resp, err := setup.proxyService.ValidateSession(ctx, req)
|
||||
require.NoError(t, err)
|
||||
require.False(t, resp.Valid, "User pending approval should be denied before approval")
|
||||
assert.Equal(t, "pending_approval", resp.DeniedReason)
|
||||
assert.Equal(t, deniedReasonPendingApproval, resp.DeniedReason)
|
||||
|
||||
user, err := setup.store.GetUserByUserID(ctx, store.LockingStrengthNone, pendingUserID)
|
||||
require.NoError(t, err)
|
||||
|
||||
@@ -130,14 +130,13 @@ func (h *AuthCallbackHandler) handleCallback(w http.ResponseWriter, r *http.Requ
|
||||
// the user can act on, while everything else stays generic so a lookup or
|
||||
// signing failure does not describe management internals to the browser.
|
||||
func sessionTokenErrorDescription(err error) string {
|
||||
switch {
|
||||
case errors.Is(err, nbgrpc.ErrUserPendingApproval):
|
||||
if errors.Is(err, nbgrpc.ErrUserPendingApproval) {
|
||||
return "Your account is pending approval by an administrator"
|
||||
case errors.Is(err, nbgrpc.ErrUserBlocked):
|
||||
return "Your account is blocked"
|
||||
default:
|
||||
return "Service configuration error"
|
||||
}
|
||||
if errors.Is(err, nbgrpc.ErrUserBlocked) {
|
||||
return "Your account is blocked"
|
||||
}
|
||||
return "Service configuration error"
|
||||
}
|
||||
|
||||
func extractUserIDFromToken(ctx context.Context, provider *oidc.Provider, config nbgrpc.ProxyOIDCConfig, token *oauth2.Token) string {
|
||||
|
||||
Reference in New Issue
Block a user