Fix DCONF policy manager removing userdb on empty policy (#1070)

We decided to go for this approach to support the `purge` command. Since it's not yet fully functional and this change is breaking user sessions that do not have at least one DCONF-related policy applied, we should revert this change for now and consider a better way of removing the leftover files in the future. Closes #1002. UDENG-3732.
ubuntu · Aug 28, 2024 · db591d9 · db591d9
2 parents 258c929 + 8a473d6
commit db591d9
Show file tree

Hide file tree

Showing 64 changed files with 94 additions and 54 deletions.
diff --git a/...en/machine_empty_state/db/machine.d/adsys → ...conf/db/[email protected]/adsys b/...en/machine_empty_state/db/machine.d/adsys → ...conf/db/[email protected]/adsys
diff --git a/...hine_empty_state/db/machine.d/locks/adsys → ...b/[email protected]/locks/adsys b/...hine_empty_state/db/machine.d/locks/adsys → ...b/[email protected]/locks/adsys
diff --git a/...stPolicyUpdate/golden/purge_current_user_policies/dconf/profile/[email protected] b/...stPolicyUpdate/golden/purge_current_user_policies/dconf/profile/[email protected]
@@ -0,0 +1,3 @@
+user-db:user
+system-db:[email protected]
+system-db:machine
diff --git a/...ration_tests/testdata/TestPolicyUpdate/golden/purge_machine_policies/dconf/db/gdm.d/adsys b/...ration_tests/testdata/TestPolicyUpdate/golden/purge_machine_policies/dconf/db/gdm.d/adsys
@@ -0,0 +1 @@
+
diff --git a/..._tests/testdata/TestPolicyUpdate/golden/purge_machine_policies/dconf/db/gdm.d/locks/adsys b/..._tests/testdata/TestPolicyUpdate/golden/purge_machine_policies/dconf/db/gdm.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...ration_tests/testdata/TestPolicyUpdate/golden/purge_machine_policies/dconf/profile/.empty b/...ration_tests/testdata/TestPolicyUpdate/golden/purge_machine_policies/dconf/profile/.empty
diff --git a/...tegration_tests/testdata/TestPolicyUpdate/golden/purge_machine_policies/dconf/profile/gdm b/...tegration_tests/testdata/TestPolicyUpdate/golden/purge_machine_policies/dconf/profile/gdm
@@ -0,0 +1,3 @@
+user-db:user
+system-db:gdm
+system-db:machine
diff --git a/...yUpdate/golden/purge_other_user_policies/dconf/db/[email protected]/adsys b/...yUpdate/golden/purge_other_user_policies/dconf/db/[email protected]/adsys
@@ -0,0 +1 @@
+
diff --git a/...e/golden/purge_other_user_policies/dconf/db/[email protected]/locks/adsys b/...e/golden/purge_other_user_policies/dconf/db/[email protected]/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...licyUpdate/golden/purge_other_user_policies/dconf/profile/[email protected] b/...licyUpdate/golden/purge_other_user_policies/dconf/profile/[email protected]
@@ -0,0 +1,3 @@
+user-db:user
+system-db:[email protected]
+system-db:machine
diff --git a/...e/golden/purge_policies_for_all_cached_objects/dconf/db/[email protected]/adsys b/...e/golden/purge_policies_for_all_cached_objects/dconf/db/[email protected]/adsys
@@ -0,0 +1 @@
+
diff --git a/...en/purge_policies_for_all_cached_objects/dconf/db/[email protected]/locks/adsys b/...en/purge_policies_for_all_cached_objects/dconf/db/[email protected]/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...stdata/TestPolicyUpdate/golden/purge_policies_for_all_cached_objects/dconf/db/gdm.d/adsys b/...stdata/TestPolicyUpdate/golden/purge_policies_for_all_cached_objects/dconf/db/gdm.d/adsys
@@ -0,0 +1 @@
+
diff --git a/.../TestPolicyUpdate/golden/purge_policies_for_all_cached_objects/dconf/db/gdm.d/locks/adsys b/.../TestPolicyUpdate/golden/purge_policies_for_all_cached_objects/dconf/db/gdm.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...en/purge_policies_for_all_cached_objects/dconf/db/[email protected]/adsys b/...en/purge_policies_for_all_cached_objects/dconf/db/[email protected]/adsys
@@ -0,0 +1 @@
+
diff --git a/...ge_policies_for_all_cached_objects/dconf/db/[email protected]/locks/adsys b/...ge_policies_for_all_cached_objects/dconf/db/[email protected]/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...stdata/TestPolicyUpdate/golden/purge_policies_for_all_cached_objects/dconf/profile/.empty b/...stdata/TestPolicyUpdate/golden/purge_policies_for_all_cached_objects/dconf/profile/.empty
diff --git a/...date/golden/purge_policies_for_all_cached_objects/dconf/profile/[email protected] b/...date/golden/purge_policies_for_all_cached_objects/dconf/profile/[email protected]
@@ -0,0 +1,3 @@
+user-db:user
+system-db:[email protected]
+system-db:machine
diff --git a/.../testdata/TestPolicyUpdate/golden/purge_policies_for_all_cached_objects/dconf/profile/gdm b/.../testdata/TestPolicyUpdate/golden/purge_policies_for_all_cached_objects/dconf/profile/gdm
@@ -0,0 +1,3 @@
+user-db:user
+system-db:gdm
+system-db:machine
diff --git a/...olden/purge_policies_for_all_cached_objects/dconf/profile/[email protected] b/...olden/purge_policies_for_all_cached_objects/dconf/profile/[email protected]
@@ -0,0 +1,3 @@
+user-db:user
+system-db:[email protected]
+system-db:machine
diff --git a/internal/policies/dconf/dconf.go b/internal/policies/dconf/dconf.go
@@ -95,27 +95,12 @@ func (m *Manager) ApplyPolicy(ctx context.Context, objectName string, isComputer
 	dbsPath := filepath.Join(dconfDir, "db")
 	dbPath := filepath.Join(dbsPath, objectName+".d")
 
-	if !isComputer && len(entries) > 0 {
+	if !isComputer {
 		if _, err := os.Stat(filepath.Join(dbsPath, "machine.d", "locks", "adsys")); err != nil {
 			return errors.New(gotext.Get("machine dconf database is required before generating a policy for an user. This one returns: %v", err))
 		}
 	}
 
-	// Only clean up user databases/profiles if there are no entries to apply.
-	// We don't clean up the machine database because we don't know if there's any user GPO depending on it.
-	if !isComputer && len(entries) == 0 {
-		if err := os.RemoveAll(dbPath); err != nil {
-			return errors.New(gotext.Get("can't remove user dconf database directory: %v", err))
-		}
-		if er := os.RemoveAll(filepath.Join(dbsPath, objectName)); er != nil {
-			return errors.New(gotext.Get("can't remove user dconf binary database: %v", err))
-		}
-		if err := os.RemoveAll(filepath.Join(profilesPath, objectName)); err != nil {
-			return errors.New(gotext.Get("can't remove user dconf profile: %v", err))
-		}
-		return nil
-	}
-
 	// Create profiles for users only
 	if !isComputer {
 		//nolint:gosec // G301 - Profile must be readable by everyone

diff --git a/internal/policies/dconf/dconf_test.go b/internal/policies/dconf/dconf_test.go
@@ -38,8 +38,6 @@ func TestApplyPolicy(t *testing.T) {
 		"Update user disabled key with value": {entries: []entry.Entry{
 			{Key: "com/ubuntu/category/key-s", Value: "'onekey-s-othervalue'", Meta: "s"}},
 			existingDconfDir: "user-with-disabled-value"},
-		"User empty state, with existing machine policy": {entries: []entry.Entry{}, existingDconfDir: "existing-user"},
-		"User empty state": {entries: []entry.Entry{}, existingDconfDir: "-"},
 
 		// Machine cases
 		"First boot": {entries: []entry.Entry{
@@ -57,7 +55,10 @@ func TestApplyPolicy(t *testing.T) {
 		"Update machine disabled key with value": {entries: []entry.Entry{
 			{Key: "com/ubuntu/category/key-s", Value: "'onekey-s-othervalue'", Meta: "s"}},
 			isComputer: true, existingDconfDir: "machine-with-disabled-value"},
-		"Machine empty state": {entries: []entry.Entry{}, isComputer: true, existingDconfDir: "-"},
+
+		// We still need to create an empty database even if there is no policy, otherwise DCONF will block any writes
+		// due to missing database profile stack file.
+		"No policy still generates a valid db": {entries: nil},
 
 		"Multiple keys same category": {entries: []entry.Entry{
 			{Key: "com/ubuntu/category/key-s", Value: "'onekey-s-othervalue'", Meta: "s"},
@@ -143,19 +144,19 @@ func TestApplyPolicy(t *testing.T) {
 		}},
 
 		// Profiles tests
-		"Update existing correct profile stays unchanged": {
+		"Update existing correct profile stays unchanged": {entries: nil,
 			existingDconfDir: "existing-user"},
-		"Update existing correct profile with trailing spaces are removed": {
+		"Update existing correct profile with trailing spaces are removed": {entries: nil,
 			existingDconfDir: "existing-user-with-trailing-spaces"},
-		"Update existing profile without needed db append them": {
+		"Update existing profile without needed db append them": {entries: nil,
 			existingDconfDir: "existing-user-no-adsysdb"},
-		"Update existing profile without needed db, trailine lines are removed": {
+		"Update existing profile without needed db, trailine lines are removed": {entries: nil,
 			existingDconfDir: "existing-user-no-adsysdb-trailing-newlines"},
-		"Update existing profile with partial db append them without repetition": {
+		"Update existing profile with partial db append them without repetition": {entries: nil,
 			existingDconfDir: "existing-user-one-adsysdb-partial"},
-		"Update existing profile with wrong order appends them in correct order": {
+		"Update existing profile with wrong order appends them in correct order": {entries: nil,
 			existingDconfDir: "existing-user-one-adsysdb-reversed-end"},
-		"Update existing profile eliminates adsys DB repetitions": {
+		"Update existing profile eliminates adsys DB repetitions": {entries: nil,
 			existingDconfDir: "existing-user-adsysdb-repetitions"},
 
 		// non adsys content
@@ -194,10 +195,6 @@ func TestApplyPolicy(t *testing.T) {
 
 			dconfDir := t.TempDir()
 
-			if tc.entries == nil {
-				tc.entries = []entry.Entry{{Key: "com/ubuntu/category/key-s", Value: "'onekey-s-othervalue'", Meta: "s"}}
-			}
-
 			if tc.existingDconfDir == "" {
 				tc.existingDconfDir = "machine-base"
 			}

diff --git a/...nf/testdata/TestApplyPolicy/golden/no_policy_still_generates_a_valid_db/db/ubuntu.d/adsys b/...nf/testdata/TestApplyPolicy/golden/no_policy_still_generates_a_valid_db/db/ubuntu.d/adsys
@@ -0,0 +1 @@
+
diff --git a/...tdata/TestApplyPolicy/golden/no_policy_still_generates_a_valid_db/db/ubuntu.d/locks/adsys b/...tdata/TestApplyPolicy/golden/no_policy_still_generates_a_valid_db/db/ubuntu.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...dconf/testdata/TestApplyPolicy/golden/no_policy_still_generates_a_valid_db/profile/ubuntu b/...dconf/testdata/TestApplyPolicy/golden/no_policy_still_generates_a_valid_db/profile/ubuntu
@@ -0,0 +1,3 @@
+user-db:user
+system-db:ubuntu
+system-db:machine
diff --git a/.../TestApplyPolicy/golden/update_existing_correct_profile_stays_unchanged/db/ubuntu.d/adsys b/.../TestApplyPolicy/golden/update_existing_correct_profile_stays_unchanged/db/ubuntu.d/adsys
@@ -1,2 +1 @@
-[com/ubuntu/category]
-key-s='onekey-s-othervalue'
+
diff --git a/...pplyPolicy/golden/update_existing_correct_profile_stays_unchanged/db/ubuntu.d/locks/adsys b/...pplyPolicy/golden/update_existing_correct_profile_stays_unchanged/db/ubuntu.d/locks/adsys
@@ -1 +1 @@
-/com/ubuntu/category/key-s
+
diff --git a/...golden/update_existing_correct_profile_with_trailing_spaces_are_removed/db/ubuntu.d/adsys b/...golden/update_existing_correct_profile_with_trailing_spaces_are_removed/db/ubuntu.d/adsys
@@ -1,2 +1 @@
-[com/ubuntu/category]
-key-s='onekey-s-othervalue'
+
diff --git a/.../update_existing_correct_profile_with_trailing_spaces_are_removed/db/ubuntu.d/locks/adsys b/.../update_existing_correct_profile_with_trailing_spaces_are_removed/db/ubuntu.d/locks/adsys
@@ -1 +1 @@
-/com/ubuntu/category/key-s
+
diff --git a/...lyPolicy/golden/update_existing_profile_eliminates_adsys_db_repetitions/db/ubuntu.d/adsys b/...lyPolicy/golden/update_existing_profile_eliminates_adsys_db_repetitions/db/ubuntu.d/adsys
@@ -1,2 +1 @@
-[com/ubuntu/category]
-key-s='onekey-s-othervalue'
+
diff --git a/...cy/golden/update_existing_profile_eliminates_adsys_db_repetitions/db/ubuntu.d/locks/adsys b/...cy/golden/update_existing_profile_eliminates_adsys_db_repetitions/db/ubuntu.d/locks/adsys
@@ -1 +1 @@
-/com/ubuntu/category/key-s
+
diff --git a/.../update_existing_profile_with_partial_db_append_them_without_repetition/db/ubuntu.d/adsys b/.../update_existing_profile_with_partial_db_append_them_without_repetition/db/ubuntu.d/adsys
@@ -1,2 +1 @@
-[com/ubuntu/category]
-key-s='onekey-s-othervalue'
+
diff --git a/...e_existing_profile_with_partial_db_append_them_without_repetition/db/ubuntu.d/locks/adsys b/...e_existing_profile_with_partial_db_append_them_without_repetition/db/ubuntu.d/locks/adsys
@@ -1 +1 @@
-/com/ubuntu/category/key-s
+
diff --git a/.../update_existing_profile_with_wrong_order_appends_them_in_correct_order/db/ubuntu.d/adsys b/.../update_existing_profile_with_wrong_order_appends_them_in_correct_order/db/ubuntu.d/adsys
@@ -1,2 +1 @@
-[com/ubuntu/category]
-key-s='onekey-s-othervalue'
+
diff --git a/...e_existing_profile_with_wrong_order_appends_them_in_correct_order/db/ubuntu.d/locks/adsys b/...e_existing_profile_with_wrong_order_appends_them_in_correct_order/db/ubuntu.d/locks/adsys
@@ -1 +1 @@
-/com/ubuntu/category/key-s
+
diff --git a/...n/update_existing_profile_without_needed_db,_trailine_lines_are_removed/db/ubuntu.d/adsys b/...n/update_existing_profile_without_needed_db,_trailine_lines_are_removed/db/ubuntu.d/adsys
@@ -1,2 +1 @@
-[com/ubuntu/category]
-key-s='onekey-s-othervalue'
+
diff --git a/...te_existing_profile_without_needed_db,_trailine_lines_are_removed/db/ubuntu.d/locks/adsys b/...te_existing_profile_without_needed_db,_trailine_lines_are_removed/db/ubuntu.d/locks/adsys
@@ -1 +1 @@
-/com/ubuntu/category/key-s
+
diff --git a/...pplyPolicy/golden/update_existing_profile_without_needed_db_append_them/db/ubuntu.d/adsys b/...pplyPolicy/golden/update_existing_profile_without_needed_db_append_them/db/ubuntu.d/adsys
@@ -1,2 +1 @@
-[com/ubuntu/category]
-key-s='onekey-s-othervalue'
+
diff --git a/...licy/golden/update_existing_profile_without_needed_db_append_them/db/ubuntu.d/locks/adsys b/...licy/golden/update_existing_profile_without_needed_db_append_them/db/ubuntu.d/locks/adsys
@@ -1 +1 @@
-/com/ubuntu/category/key-s
+
diff --git a/.../TestApplyPolicy/golden/user_empty_state,_with_existing_machine_policy/db/machine.d/adsys b/.../TestApplyPolicy/golden/user_empty_state,_with_existing_machine_policy/db/machine.d/adsys
diff --git a/...pplyPolicy/golden/user_empty_state,_with_existing_machine_policy/db/machine.d/locks/adsys b/...pplyPolicy/golden/user_empty_state,_with_existing_machine_policy/db/machine.d/locks/adsys
diff --git a/...data/TestApplyPolicy/golden/user_empty_state,_with_existing_machine_policy/profile/.empty b/...data/TestApplyPolicy/golden/user_empty_state,_with_existing_machine_policy/profile/.empty
diff --git a/internal/policies/dconf/testdata/TestApplyPolicy/golden/user_empty_state/.empty b/internal/policies/dconf/testdata/TestApplyPolicy/golden/user_empty_state/.empty
diff --git a/...a/TestApplyPolicies/golden/no_subscription_is_only_dconf_content/etc/dconf/db/gdm.d/adsys b/...a/TestApplyPolicies/golden/no_subscription_is_only_dconf_content/etc/dconf/db/gdm.d/adsys
@@ -0,0 +1 @@
+
diff --git a/...ApplyPolicies/golden/no_subscription_is_only_dconf_content/etc/dconf/db/gdm.d/locks/adsys b/...ApplyPolicies/golden/no_subscription_is_only_dconf_content/etc/dconf/db/gdm.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...data/TestApplyPolicies/golden/no_subscription_is_only_dconf_content/etc/dconf/profile/gdm b/...data/TestApplyPolicies/golden/no_subscription_is_only_dconf_content/etc/dconf/profile/gdm
@@ -0,0 +1,3 @@
+user-db:user
+system-db:gdm
+system-db:machine
diff --git a/...pplyPolicies/golden/second_call_with_no_rules_deletes_everything/etc/dconf/db/gdm.d/adsys b/...pplyPolicies/golden/second_call_with_no_rules_deletes_everything/etc/dconf/db/gdm.d/adsys
@@ -0,0 +1 @@
+
diff --git a/...licies/golden/second_call_with_no_rules_deletes_everything/etc/dconf/db/gdm.d/locks/adsys b/...licies/golden/second_call_with_no_rules_deletes_everything/etc/dconf/db/gdm.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...stApplyPolicies/golden/second_call_with_no_rules_deletes_everything/etc/dconf/profile/gdm b/...stApplyPolicies/golden/second_call_with_no_rules_deletes_everything/etc/dconf/profile/gdm
@@ -0,0 +1,3 @@
+user-db:user
+system-db:gdm
+system-db:machine
diff --git a/..._call_with_no_rules_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/db/gdm.d/adsys b/..._call_with_no_rules_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/db/gdm.d/adsys
@@ -0,0 +1 @@
+
diff --git a/...with_no_rules_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/db/gdm.d/locks/adsys b/...with_no_rules_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/db/gdm.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...ond_call_with_no_rules_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/profile/gdm b/...ond_call_with_no_rules_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/profile/gdm
@@ -0,0 +1,3 @@
+user-db:user
+system-db:gdm
+system-db:machine
diff --git a/...ith_no_subscription_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/db/gdm.d/adsys b/...ith_no_subscription_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/db/gdm.d/adsys
@@ -0,0 +1 @@
+
diff --git a/..._subscription_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/db/gdm.d/locks/adsys b/..._subscription_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/db/gdm.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...l_with_no_subscription_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/profile/gdm b/...l_with_no_subscription_don't_remove_scripts_if_session_hasn’t_ended/etc/dconf/profile/gdm
@@ -0,0 +1,3 @@
+user-db:user
+system-db:gdm
+system-db:machine
diff --git a/..._with_no_subscription_should_remove_everything_but_dconf_content/etc/dconf/db/gdm.d/adsys b/..._with_no_subscription_should_remove_everything_but_dconf_content/etc/dconf/db/gdm.d/adsys
@@ -0,0 +1 @@
+
diff --git a/...no_subscription_should_remove_everything_but_dconf_content/etc/dconf/db/gdm.d/locks/adsys b/...no_subscription_should_remove_everything_but_dconf_content/etc/dconf/db/gdm.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...all_with_no_subscription_should_remove_everything_but_dconf_content/etc/dconf/profile/gdm b/...all_with_no_subscription_should_remove_everything_but_dconf_content/etc/dconf/profile/gdm
@@ -0,0 +1,3 @@
+user-db:user
+system-db:gdm
+system-db:machine
diff --git a/internal/policies/testdata/TestApplyPolicies/golden/succeed/etc/dconf/db/gdm.d/adsys b/internal/policies/testdata/TestApplyPolicies/golden/succeed/etc/dconf/db/gdm.d/adsys
@@ -0,0 +1 @@
+
diff --git a/internal/policies/testdata/TestApplyPolicies/golden/succeed/etc/dconf/db/gdm.d/locks/adsys b/internal/policies/testdata/TestApplyPolicies/golden/succeed/etc/dconf/db/gdm.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/internal/policies/testdata/TestApplyPolicies/golden/succeed/etc/dconf/profile/gdm b/internal/policies/testdata/TestApplyPolicies/golden/succeed/etc/dconf/profile/gdm
@@ -0,0 +1,3 @@
+user-db:user
+system-db:gdm
+system-db:machine
diff --git a/...n/succeed_if_checking_for_backend_online_status_returns_an_error/etc/dconf/db/gdm.d/adsys b/...n/succeed_if_checking_for_backend_online_status_returns_an_error/etc/dconf/db/gdm.d/adsys
@@ -0,0 +1 @@
+
diff --git a/...eed_if_checking_for_backend_online_status_returns_an_error/etc/dconf/db/gdm.d/locks/adsys b/...eed_if_checking_for_backend_online_status_returns_an_error/etc/dconf/db/gdm.d/locks/adsys
@@ -0,0 +1 @@
+
diff --git a/...lden/succeed_if_checking_for_backend_online_status_returns_an_error/etc/dconf/profile/gdm b/...lden/succeed_if_checking_for_backend_online_status_returns_an_error/etc/dconf/profile/gdm
@@ -0,0 +1,3 @@
+user-db:user
+system-db:gdm
+system-db:machine
Original file line number	Diff line number	Diff line change
		@@ -1,2 +1 @@
		[com/ubuntu/category]
		key-s='onekey-s-othervalue'