mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-26 06:19:08 +01:00
[client] Do not write the profile config while only reading it to decide
The update-settings gate needs the stored config to decide whether a request changes anything, so the previous commit moved that read ahead of the refusal. The read is not side-effect free: profilemanager.GetConfig writes the config back whenever apply() has to fill in a default the file was missing. A request that the gate then refuses had therefore already rewritten the profile file. PeekConfig is GetConfig without that write-back. The returned config is still normalized in memory, which is what the decision needs; the file is left exactly as it was found. Every caller of storedConfigAtPath feeds a gate that can refuse, so they all peek. Note for reviewers: the daemon still normalizes the file on startup and on every real update, so nothing depends on a read performing that write.
This commit is contained in:
@@ -1161,6 +1161,11 @@ func (s *Server) storedLoginConfig(activeProf *profilemanager.ActiveProfileState
|
||||
|
||||
// storedConfigAtPath reads a profile config file, yielding nil when it does not
|
||||
// exist yet.
|
||||
//
|
||||
// It peeks rather than reads: every caller here feeds a gate that may refuse
|
||||
// the request, and profilemanager.GetConfig writes the config back whenever it
|
||||
// has to fill in a default the file was missing. A refused request must leave
|
||||
// the profile file exactly as it found it.
|
||||
func (s *Server) storedConfigAtPath(path string) (*profilemanager.Config, error) {
|
||||
if _, err := os.Stat(path); err != nil {
|
||||
if os.IsNotExist(err) {
|
||||
@@ -1169,7 +1174,7 @@ func (s *Server) storedConfigAtPath(path string) (*profilemanager.Config, error)
|
||||
return nil, fmt.Errorf("stat profile config: %w", err)
|
||||
}
|
||||
|
||||
cfg, err := profilemanager.GetConfig(path)
|
||||
cfg, err := profilemanager.PeekConfig(path)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("read profile config: %w", err)
|
||||
}
|
||||
|
||||
@@ -2,6 +2,7 @@ package server
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
|
||||
@@ -218,3 +219,28 @@ func TestGateDecisionFailsClosedOnAnInvalidRequest(t *testing.T) {
|
||||
require.True(t, configChangeRequested(nil, profilemanager.ConfigInput{ManagementURL: "not-a-url"}),
|
||||
"an unevaluable request must count as a change")
|
||||
}
|
||||
|
||||
// The gate reads the stored config to decide, and reading it must not write it:
|
||||
// a refused request has to leave the profile file byte-for-byte as it was.
|
||||
// A config file missing a field the config layer fills in (MTU, here) is what
|
||||
// makes the normalization write fire.
|
||||
func TestSetConfig_RefusedRequestLeavesTheConfigFileUntouched(t *testing.T) {
|
||||
s, ctx, profName, username, cfgPath := setupServerWithProfile(t)
|
||||
s.updateSettingsDisabled = true
|
||||
|
||||
require.NoError(t, os.WriteFile(cfgPath, []byte(`{"WgIface":"wt0"}`), 0o600))
|
||||
before, err := os.ReadFile(cfgPath)
|
||||
require.NoError(t, err)
|
||||
|
||||
_, err = s.SetConfig(ctx, &proto.SetConfigRequest{
|
||||
ProfileName: profName,
|
||||
Username: username,
|
||||
ManagementUrl: "https://mgmt.elsewhere.example:443",
|
||||
})
|
||||
require.Error(t, err)
|
||||
require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err)
|
||||
|
||||
after, err := os.ReadFile(cfgPath)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, string(before), string(after), "the refused request rewrote the profile config")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user