Fix gitops access when using --dry-run with fleetctl apply (#13178)

changes/issue-13177-fix-gitops-fleetctl-apply

@@ -0,0 +1 @@
+* Fixed an issue when a user with `gitops` role was used to validate a configuration with `fleetctl apply --dry-run`.

cmd/fleetctl/apply_test.go

@@ -887,6 +887,11 @@ spec:
         pack_delimiter: /
     overrides: {}
 `)
+
+	// test applying with dry-run flag
+	assert.Equal(t, "[+] would've applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"}))
+
+	// test applying for real
 	assert.Equal(t, "[+] applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name}))
 	assert.True(t, currentAppConfig.Features.EnableHostUsers)
 
@@ -914,6 +919,11 @@ spec:
       macos_setup_assistant: %s
     windows_enabled_and_configured: true
 `, mobileConfigPath, emptySetupAsst))
+
+	// first apply with dry-run
+	assert.Equal(t, "[+] would've applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"}))
+
+	// then apply for real
 	assert.Equal(t, "[+] applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name}))
 	// features left untouched, not provided
 	assert.True(t, currentAppConfig.Features.EnableHostUsers)
@@ -945,6 +955,11 @@ spec:
     macos_setup:
       bootstrap_package: %s
 `, bootstrapURL))
+
+	// first apply with dry-run
+	assert.Equal(t, "[+] would've applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"}))
+
+	// then apply for real
 	assert.Equal(t, "[+] applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name}))
 	// features left untouched, not provided
 	assert.True(t, currentAppConfig.Features.EnableHostUsers)
@@ -988,6 +1003,10 @@ spec:
       - secret: BBB
 `, mobileConfigPath))
 
+	// first apply with dry-run
+	require.Equal(t, "[+] would've applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"}))
+
+	// then apply for real
 	require.Equal(t, "[+] applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name}))
 	assert.JSONEq(t, string(json.RawMessage(`{"config":{"views":{"foo":"qux"}}}`)), string(*savedTeam.Config.AgentOptions))
 	assert.Equal(t, fleet.TeamMDM{
@@ -1015,6 +1034,11 @@ spec:
       macos_setup:
         macos_setup_assistant: %s
 `, emptySetupAsst))
+
+	// first apply with dry-run
+	require.Equal(t, "[+] would've applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"}))
+
+	// then apply for real
 	require.Equal(t, "[+] applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name}))
 	require.True(t, ds.GetMDMAppleSetupAssistantFuncInvoked)
 	require.True(t, ds.SetOrUpdateMDMAppleSetupAssistantFuncInvoked)
@@ -1045,6 +1069,11 @@ spec:
       macos_setup:
         bootstrap_package: %s
 `, bootstrapURL))
+
+	// first apply with dry-run
+	require.Equal(t, "[+] would've applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name, "--dry-run"}))
+
+	// then apply for real
 	require.Equal(t, "[+] applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", name}))
 	// all left untouched, only bootstrap package added
 	assert.Equal(t, fleet.TeamMDM{

ee/server/service/mdm.go

@@ -219,7 +219,10 @@ func (svc *Service) UpdateMDMAppleSetup(ctx context.Context, payload fleet.MDMAp
 }
 
 func (svc *Service) updateAppConfigMDMAppleSetup(ctx context.Context, payload fleet.MDMAppleSetupPayload) error {
-	ac, err := svc.AppConfigObfuscated(ctx)
+	// appconfig is only used internally, it's fine to read it unobfuscated
+	// (svc.AppConfigObfuscated must not be used because the write-only users
+	// such as gitops will fail to access it).
+	ac, err := svc.ds.AppConfig(ctx)
 	if err != nil {
 		return err
 	}
@@ -264,7 +267,10 @@ func (svc *Service) updateMacOSSetupEnableEndUserAuth(ctx context.Context, enabl
 }
 
 func (svc *Service) validateMDMAppleSetupPayload(ctx context.Context, payload fleet.MDMAppleSetupPayload) error {
-	ac, err := svc.AppConfigObfuscated(ctx)
+	// appconfig is only used internally, it's fine to read it unobfuscated
+	// (svc.AppConfigObfuscated must not be used because the write-only users
+	// such as gitops will fail to access it).
+	ac, err := svc.ds.AppConfig(ctx)
 	if err != nil {
 		return err
 	}

server/service/appconfig.go

@@ -363,8 +363,14 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle
 		if legacyUsedWarning != nil {
 			return nil, legacyUsedWarning
 		}
-		// must reload to get the unchanged app config
+
-		return svc.AppConfigObfuscated(ctx)
+		// must reload to get the unchanged app config (retrieve with obfuscated secrets)
+		obfuscatedAppConfig, err := svc.ds.AppConfig(ctx)
+		if err != nil {
+			return nil, err
+		}
+		obfuscatedAppConfig.Obfuscate()
+		return obfuscatedAppConfig, nil
 	}
 
 	// Perform validation of the applied SMTP settings.

server/service/apple_mdm.go

@@ -1681,12 +1681,8 @@ func (svc *Service) UpdateMDMAppleSettings(ctx context.Context, payload fleet.MD
 	// for now, assume all settings require premium (this is true for the first
 	// supported setting, enable_disk_encryption. Adjust as needed in the future
 	// if this is not always the case).
-	license, err := svc.License(ctx)
+	lic, _ := license.FromContext(ctx)
-	if err != nil {
+	if lic == nil || !lic.IsPremium() {
-		svc.authz.SkipAuthorization(ctx) // so that the error message is not replaced by "forbidden"
-		return err
-	}
-	if !license.IsPremium() {
 		svc.authz.SkipAuthorization(ctx) // so that the error message is not replaced by "forbidden"
 		return ErrMissingLicense
 	}
@@ -1706,7 +1702,10 @@ func (svc *Service) UpdateMDMAppleSettings(ctx context.Context, payload fleet.MD
 }
 
 func (svc *Service) updateAppConfigMDMAppleSettings(ctx context.Context, payload fleet.MDMAppleSettingsPayload) error {
-	ac, err := svc.AppConfigObfuscated(ctx)
+	// appconfig is only used internally, it's fine to read it unobfuscated
+	// (svc.AppConfigObfuscated must not be used because the write-only users
+	// such as gitops will fail to access it).
+	ac, err := svc.ds.AppConfig(ctx)
 	if err != nil {
 		return err
 	}

server/service/integration_mdm_test.go

@@ -4882,6 +4882,17 @@ func (s *integrationMDMTestSuite) TestGitOpsUserActions() {
   }`), http.StatusOK, &acResp)
 	assert.True(t, acResp.MDM.MacOSSettings.EnableDiskEncryption)
 
+	// Attempt to setup Apple MDM, will fail but the important thing is that it
+	// fails with 422 (cannot enable end user auth because no IdP is configured)
+	// and not 403 forbidden.
+	s.Do("PATCH", "/api/latest/fleet/mdm/apple/setup",
+		fleet.MDMAppleSetupPayload{TeamID: ptr.Uint(0), EnableEndUserAuthentication: ptr.Bool(true)}, http.StatusUnprocessableEntity)
+
+	// Attempt to update the Apple MDM settings but with no change, just to
+	// validate the access.
+	s.Do("PATCH", "/api/latest/fleet/mdm/apple/settings",
+		fleet.MDMAppleSettingsPayload{}, http.StatusNoContent)
+
 	// Attempt to set profile batch globally, should allow.
 	globalProfiles := [][]byte{
 		mobileconfigForTest("N1", "I1"),