From 294fa0bcfd95b479ed242732776958d3f4c1c8ce Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 2 Sep 2026 15:31:52 +0200 Subject: [PATCH] [client] Address the remaining bot findings on PR #7398 - Login logged the active-profile-state error and returned the same cause; the repo's guidelines call for one or the other, and the wrapped error is the one that carries context. (CodeRabbit) - `netbird up` reported a codes.Unavailable SetConfig failure as "the daemon refused the settings update", but that code also covers a daemon that became unreachable. It now reports what the daemon said without asserting why. (cubic-dev-ai) - TestLogin_ChangingTheManagementURLIsRefused asserted the error and nothing else, while "refused before it can touch daemon state" is the contract. It now checks the stored management URL, the in-progress login and the active profile, matching its SetConfig counterpart. (cubic-dev-ai) --- client/cmd/up.go | 9 +++++---- client/server/server.go | 1 - client/server/update_settings_gate_test.go | 16 +++++++++++++++- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/client/cmd/up.go b/client/cmd/up.go index ab2d82acf..6812cf88b 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -353,10 +353,11 @@ func runInDaemonMode(ctx context.Context, cmd *cobra.Command, pm *profilemanager req := setupSetConfigReq(customDNSAddressConverted, cmd, activeProf.ID.String(), username.Username) if _, err := client.SetConfig(ctx, req); err != nil { if st, ok := gstatus.FromError(err); ok && st.Code() == codes.Unavailable { - // The daemon refused the settings update, it did not lack the - // method: reporting the latter sent people looking for a version - // mismatch that was not there. - log.Warnf("the daemon refused the settings update: %s", st.Message()) + // Report what the daemon said rather than asserting why: this code + // covers both a refused update and a daemon that became + // unreachable. Claiming the method was missing, as this used to, + // sent people looking for a version mismatch that was not there. + log.Warnf("the daemon did not apply the settings update: %s", st.Message()) } else { return daemonCallError("call service setConfig method", err) } diff --git a/client/server/server.go b/client/server/server.go index d50bb9663..9d9bcce28 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -621,7 +621,6 @@ func (s *Server) setConfigInputFromRequest(msg *proto.SetConfigRequest) (profile func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*proto.LoginResponse, error) { activeProf, err := s.profileManager.GetActiveProfileState() if err != nil { - log.Errorf("failed to get active profile state: %v", err) return nil, fmt.Errorf("failed to get active profile state: %w", err) } diff --git a/client/server/update_settings_gate_test.go b/client/server/update_settings_gate_test.go index 2808318d8..91ca8a898 100644 --- a/client/server/update_settings_gate_test.go +++ b/client/server/update_settings_gate_test.go @@ -107,16 +107,30 @@ func TestSetConfig_ChangeAllowedWhenTheSwitchIsOff(t *testing.T) { // way: a login that would move a protected setting is refused before it can // touch daemon state. func TestLogin_ChangingTheManagementURLIsRefused(t *testing.T) { - s, _, _, username, _ := setupServerWithProfile(t) + s, _, profName, username, cfgPath := setupServerWithProfile(t) s.updateSettingsDisabled = true s.rootCtx = internal.CtxInitState(context.Background()) + cancelled := false + s.actCancel = func() { cancelled = true } + _, err := s.Login(userCtx(), &proto.LoginRequest{ Username: &username, ManagementUrl: "https://mgmt.elsewhere.example:443", }) require.Error(t, err, "moving the management URL through Login is a settings change") require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) + + // "Refused before it can touch daemon state" is the contract, so check the + // state as well as the error. + cfg, err := profilemanager.GetExistingConfig(cfgPath) + require.NoError(t, err) + require.Equal(t, storedManagementURL, cfg.ManagementURL.String(), "the refused login moved the management URL") + require.False(t, cancelled, "the refused login cancelled the login already in progress") + + active, err := s.profileManager.GetActiveProfileState() + require.NoError(t, err) + require.Equal(t, profilemanager.ID(profName), active.ID, "the refused login switched the active profile") } // seedProfileConfig writes a profile config carrying the given management URL