From 627ee71fa82d6e909eb91bdda5085d79a612af56 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Thu, 21 May 2026 11:45:11 +0200 Subject: [PATCH] fix(review): coderabbit follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - openapi.yml: declare default: false on ServiceTargetOptions.direct_upstream so generated clients/validators reflect the documented default. - proto/proxy_service.proto: ValidateTunnelPeer doc + denied_reason list said "distribution_groups" (bearer-auth field) but the actual gate is service.access_groups. Replaced both occurrences to match the code path in checkPeerGroupAccess. - peers/manager.go (GetPeerWithGroups) + users/manager.go (GetUserWithGroups): on store error after a successful first lookup, both now return (nil, nil, err) so callers can't get a valid entity alongside a non-nil error. Findings skipped with reasons: - embedded.go merged CLI/Dashboard redirect URIs: pre-existing on origin/main, not introduced by this PR. - account_mock.go MarkPeerDisconnected zero-time UnixNano: same — pre-existing. - openapi Service schema if/then conditionals: Go-side Validate() already enforces these invariants (Private + non-empty AccessGroups, mode=http, mutually-exclusive with bearer), and oapi-codegen on OpenAPI 3.1.x doesn't honour allOf/if/then anyway. - *.patch / *.diff / b-n-p.sh: untracked personal artifacts, not part of any commit. --- management/internals/modules/peers/manager.go | 6 ++++-- management/server/users/manager.go | 3 ++- shared/management/http/api/openapi.yml | 3 ++- shared/management/http/api/types.gen.go | 2 +- shared/management/proto/proxy_service.pb.go | 2 +- shared/management/proto/proxy_service.proto | 10 +++++----- shared/management/proto/proxy_service_grpc.pb.go | 16 ++++++++-------- 7 files changed, 23 insertions(+), 19 deletions(-) diff --git a/management/internals/modules/peers/manager.go b/management/internals/modules/peers/manager.go index 3a2dd39e2..75ae8de91 100644 --- a/management/internals/modules/peers/manager.go +++ b/management/internals/modules/peers/manager.go @@ -113,7 +113,9 @@ func (m *managerImpl) GetPeerByTunnelIP(ctx context.Context, accountID string, i return m.store.GetPeerByIP(ctx, store.LockingStrengthNone, accountID, ip) } -// GetPeerWithGroups returns the peer plus its group memberships. +// GetPeerWithGroups returns the peer plus its group memberships. Any store +// error returns (nil, nil, err) so callers never receive a valid peer +// alongside a non-nil error. func (m *managerImpl) GetPeerWithGroups(ctx context.Context, accountID, peerID string) (*peer.Peer, []*types.Group, error) { p, err := m.store.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peerID) if err != nil { @@ -121,7 +123,7 @@ func (m *managerImpl) GetPeerWithGroups(ctx context.Context, accountID, peerID s } groups, err := m.store.GetPeerGroups(ctx, store.LockingStrengthNone, accountID, peerID) if err != nil { - return p, nil, err + return nil, nil, err } return p, groups, nil } diff --git a/management/server/users/manager.go b/management/server/users/manager.go index 634b2d006..1a05b1a7c 100644 --- a/management/server/users/manager.go +++ b/management/server/users/manager.go @@ -33,6 +33,7 @@ func (m *managerImpl) GetUser(ctx context.Context, userID string) (*types.User, // GetUserWithGroups returns the user and the *types.Group records for the user's AutoGroups, in the same order as // AutoGroups. Group ids that don't resolve to a stored group are skipped from the returned slice (the parallel id list is // derivable from the returned User). Wraps two store calls today; can be optimised to a single JOIN later if needed. +// Any store error returns (nil, nil, err) so callers never receive a valid user alongside a non-nil error. func (m *managerImpl) GetUserWithGroups(ctx context.Context, userID string) (*types.User, []*types.Group, error) { user, err := m.store.GetUserByUserID(ctx, store.LockingStrengthNone, userID) if err != nil { @@ -43,7 +44,7 @@ func (m *managerImpl) GetUserWithGroups(ctx context.Context, userID string) (*ty } groupsMap, err := m.store.GetGroupsByIDs(ctx, store.LockingStrengthNone, user.AccountID, user.AutoGroups) if err != nil { - return user, nil, err + return nil, nil, err } groups := make([]*types.Group, 0, len(user.AutoGroups)) for _, id := range user.AutoGroups { diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index cd412ef93..6b8939598 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -3213,7 +3213,8 @@ components: When true, the proxy dials this target via the host's network stack instead of through its embedded NetBird client. Use for upstreams reachable without WireGuard (public APIs, LAN services, localhost - sidecars). Default false. + sidecars). + default: false example: false ServiceTarget: type: object diff --git a/shared/management/http/api/types.gen.go b/shared/management/http/api/types.gen.go index fbcce9bdd..d7945e448 100644 --- a/shared/management/http/api/types.gen.go +++ b/shared/management/http/api/types.gen.go @@ -4248,7 +4248,7 @@ type ServiceTargetOptions struct { // DirectUpstream When true, the proxy dials this target via the host's network stack // instead of through its embedded NetBird client. Use for upstreams // reachable without WireGuard (public APIs, LAN services, localhost - // sidecars). Default false. + // sidecars). DirectUpstream *bool `json:"direct_upstream,omitempty"` // PathRewrite Controls how the request path is rewritten before forwarding to the backend. Default strips the matched prefix. "preserve" keeps the full original request path. diff --git a/shared/management/proto/proxy_service.pb.go b/shared/management/proto/proxy_service.pb.go index 67f9a10fb..96527584f 100644 --- a/shared/management/proto/proxy_service.pb.go +++ b/shared/management/proto/proxy_service.pb.go @@ -2185,7 +2185,7 @@ func (x *ValidateTunnelPeerRequest) GetDomain() string { // "no_user" — peer exists but is not bound to a user // "service_not_found" // "account_mismatch" -// "not_in_group" — user resolved but not in service.distribution_groups +// "not_in_group" — peer resolved but not in service.access_groups type ValidateTunnelPeerResponse struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache diff --git a/shared/management/proto/proxy_service.proto b/shared/management/proto/proxy_service.proto index eb5057fc5..71e18c721 100644 --- a/shared/management/proto/proxy_service.proto +++ b/shared/management/proto/proxy_service.proto @@ -36,12 +36,12 @@ service ProxyService { rpc ValidateSession(ValidateSessionRequest) returns (ValidateSessionResponse); // ValidateTunnelPeer resolves an inbound peer by its WireGuard tunnel IP and - // checks the resolved user's access against the service's required groups. + // checks the resolved user's access against the service's access_groups. // Acts as a fast-path equivalent of OIDC for requests originating on the // netbird mesh: when the source IP maps to a known peer in the calling - // account and that peer's user is in the service's distribution_groups, - // the proxy can issue a session cookie without redirecting through the - // OIDC flow. Mirrors ValidateSession's response shape. + // account and that peer is in the service's access_groups, the proxy can + // issue a session cookie without redirecting through the OIDC flow. + // Mirrors ValidateSession's response shape. rpc ValidateTunnelPeer(ValidateTunnelPeerRequest) returns (ValidateTunnelPeerResponse); } @@ -323,7 +323,7 @@ message ValidateTunnelPeerRequest { // "no_user" — peer exists but is not bound to a user // "service_not_found" // "account_mismatch" -// "not_in_group" — user resolved but not in service.distribution_groups +// "not_in_group" — peer resolved but not in service.access_groups message ValidateTunnelPeerResponse { bool valid = 1; string user_id = 2; diff --git a/shared/management/proto/proxy_service_grpc.pb.go b/shared/management/proto/proxy_service_grpc.pb.go index 30eabc296..40064fe61 100644 --- a/shared/management/proto/proxy_service_grpc.pb.go +++ b/shared/management/proto/proxy_service_grpc.pb.go @@ -36,12 +36,12 @@ type ProxyServiceClient interface { // Called by the proxy after receiving a session token from OIDC callback. ValidateSession(ctx context.Context, in *ValidateSessionRequest, opts ...grpc.CallOption) (*ValidateSessionResponse, error) // ValidateTunnelPeer resolves an inbound peer by its WireGuard tunnel IP and - // checks the resolved user's access against the service's required groups. + // checks the resolved user's access against the service's access_groups. // Acts as a fast-path equivalent of OIDC for requests originating on the // netbird mesh: when the source IP maps to a known peer in the calling - // account and that peer's user is in the service's distribution_groups, - // the proxy can issue a session cookie without redirecting through the - // OIDC flow. Mirrors ValidateSession's response shape. + // account and that peer is in the service's access_groups, the proxy can + // issue a session cookie without redirecting through the OIDC flow. + // Mirrors ValidateSession's response shape. ValidateTunnelPeer(ctx context.Context, in *ValidateTunnelPeerRequest, opts ...grpc.CallOption) (*ValidateTunnelPeerResponse, error) } @@ -201,12 +201,12 @@ type ProxyServiceServer interface { // Called by the proxy after receiving a session token from OIDC callback. ValidateSession(context.Context, *ValidateSessionRequest) (*ValidateSessionResponse, error) // ValidateTunnelPeer resolves an inbound peer by its WireGuard tunnel IP and - // checks the resolved user's access against the service's required groups. + // checks the resolved user's access against the service's access_groups. // Acts as a fast-path equivalent of OIDC for requests originating on the // netbird mesh: when the source IP maps to a known peer in the calling - // account and that peer's user is in the service's distribution_groups, - // the proxy can issue a session cookie without redirecting through the - // OIDC flow. Mirrors ValidateSession's response shape. + // account and that peer is in the service's access_groups, the proxy can + // issue a session cookie without redirecting through the OIDC flow. + // Mirrors ValidateSession's response shape. ValidateTunnelPeer(context.Context, *ValidateTunnelPeerRequest) (*ValidateTunnelPeerResponse, error) mustEmbedUnimplementedProxyServiceServer() }