Fix setting Activity Log enable flag through the API (hashicorp#10594)

* fix setting enable, update tests

* improve wording

* fix typo - left the testing enabled set in originally

* improve warning handling

* move from nested if to switch - TIL

vault/activity_log_test.go

@@ -694,13 +694,15 @@ func TestActivityLog_API_ConfigCRUD(t *testing.T) {
 		req.Data["enabled"] = "default"
 		req.Data["retention_months"] = 24
 		req.Data["default_report_months"] = 12
+
+		originalEnabled := core.activityLog.GetEnabled()
+		newEnabled := activityLogEnabledDefault
+
 		resp, err := b.HandleRequest(namespace.RootContext(nil), req)
 		if err != nil {
 			t.Fatalf("err: %v", err)
 		}
-		if resp != nil {
-			t.Fatalf("bad: %#v", resp)
-		}
+		checkAPIWarnings(t, originalEnabled, newEnabled, resp)
 
 		req = logical.TestRequest(t, logical.ReadOperation, "internal/counters/config")
 		req.Storage = view
@@ -1640,6 +1642,23 @@ func TestActivityLog_DeleteWorker(t *testing.T) {
 	expectMissingSegment(t, core, ActivityLogPrefix+"directtokens/1111/1")
 }
 
+// checkAPIWarnings ensures there is a warning if switching from enabled -> disabled,
+// and no response otherwise
+func checkAPIWarnings(t *testing.T, originalEnabled, newEnabled bool, resp *logical.Response) {
+	t.Helper()
+
+	expectWarning := originalEnabled == true && newEnabled == false
+
+	switch {
+	case !expectWarning && resp != nil:
+		t.Fatalf("got unexpected response: %#v", resp)
+	case expectWarning && resp == nil:
+		t.Fatal("expected response (containing warning) when switching from enabled to disabled")
+	case expectWarning && len(resp.Warnings) == 0:
+		t.Fatal("expected warning when switching from enabled to disabled")
+	}
+}
+
 func TestActivityLog_EnableDisable(t *testing.T) {
 	timeutil.SkipAtEndOfMonth(t)
 
@@ -1650,29 +1669,30 @@ func TestActivityLog_EnableDisable(t *testing.T) {
 
 	enableRequest := func() {
 		t.Helper()
+		originalEnabled := a.GetEnabled()
+
 		req := logical.TestRequest(t, logical.UpdateOperation, "internal/counters/config")
 		req.Storage = view
 		req.Data["enabled"] = "enable"
 		resp, err := b.HandleRequest(ctx, req)
 		if err != nil {
 			t.Fatalf("err: %v", err)
 		}
-		if resp != nil {
-			t.Fatalf("bad: %#v", resp)
-		}
+		// don't really need originalEnabled, but might as well be correct
+		checkAPIWarnings(t, originalEnabled, true, resp)
 	}
 	disableRequest := func() {
 		t.Helper()
+		originalEnabled := a.GetEnabled()
+
 		req := logical.TestRequest(t, logical.UpdateOperation, "internal/counters/config")
 		req.Storage = view
 		req.Data["enabled"] = "disable"
 		resp, err := b.HandleRequest(ctx, req)
 		if err != nil {
 			t.Fatalf("err: %v", err)
 		}
-		if resp != nil {
-			t.Fatalf("bad: %#v", resp)
-		}
+		checkAPIWarnings(t, originalEnabled, false, resp)
 	}
 
 	// enable (if not already) and write a segment

vault/activity_log_testing_util.go

@@ -164,3 +164,10 @@ func (a *ActivityLog) SetEnable(enabled bool) {
 	defer a.fragmentLock.Unlock()
 	a.enabled = enabled
 }
+
+// GetEnabled returns the enabled flag on an activity log
+func (a *ActivityLog) GetEnabled() bool {
+	a.fragmentLock.RLock()
+	defer a.fragmentLock.RUnlock()
+	return a.enabled
+}

vault/logical_system_activity.go

@@ -190,19 +190,20 @@ func (b *SystemBackend) handleActivityConfigUpdate(ctx context.Context, req *log
 		if enabledRaw, ok := d.GetOk("enabled"); ok {
 			enabledStr := enabledRaw.(string)
 
-			// If currently enabled with the intent to disable or intent to revert to
-			// default in a OSS context, then we return a warning to the client.
+			// If we switch from enabled to disabled, then we return a warning to the client.
+			// We have to keep the default state of activity log enabled in mind
 			if config.Enabled == "enable" && enabledStr == "disable" ||
 				!activityLogEnabledDefault && config.Enabled == "enable" && enabledStr == "default" ||
 				activityLogEnabledDefault && config.Enabled == "default" && enabledStr == "disable" {
 				warnings = append(warnings, "the current monthly segment will be deleted because the activity log was disabled")
 			}
-		}
 
-		switch config.Enabled {
-		case "default", "enable", "disable":
-		default:
-			return logical.ErrorResponse("enabled must be one of \"default\", \"enable\", \"disable\""), logical.ErrInvalidRequest
+			switch enabledStr {
+			case "default", "enable", "disable":
+				config.Enabled = enabledStr
+			default:
+				return logical.ErrorResponse("enabled must be one of \"default\", \"enable\", \"disable\""), logical.ErrInvalidRequest
+			}
 		}
 	}