fix(certificate): give owner_role_name a declarative clear path

owner_role_name only ever supported setting a new owner: an explicit
owner_role_name = "" fell into the same code path as a real role name,
sending {"NewRoleName":""} to PUT /Certificates/{id}/Owner. Per the
verified live Command behavior (Swagger: "If removing the owner, leave
both empty"; confirmed against a v25.5 lab -- HTTP 204, subsequent GET
shows OwnerRoleId/OwnerRoleName null), the correct clear payload omits
both NewRoleId and NewRoleName entirely.

- ownerChangeRequestForPlan builds {} (both fields nil) for an explicit
  "" plan value, relying on api.OwnerRequest's existing `omitempty`
  pointer fields -- no go-client change needed.
- keepStringSentinel (attribute_contract.go) gives owner_role_name
  sentinel stability on Read: when the server reports no owner and
  prior state held the "" clear sentinel, state keeps "" instead of
  collapsing to null, so config-vs-state stays settled instead of
  producing a permanent "" -> null -> "" diff. A real owner appearing
  out-of-band still surfaces as drift over a stale sentinel.
- Documented the omit/clear contract on the schema description and
  regenerated docs.

certificateOwnerRoleChanged already refused to fire on an undeclared
(Null/Unknown) plan value, and Update's post-apply state already echoed
the plan value verbatim via knownStringFromPlan, so both continue to
behave correctly for the clear case with no change needed there.

Author: spbsoluble

docs/resources/certificate.md

@@ -324,6 +324,9 @@ Note:  This parameter is considered deprecated as for Keyfactor Command v25.1.0
 - `organizational_unit` (String) Subject organizational unit (OU) of the certificate
 - `owner_role_name` (String) A string containing the name of the security role assigned as the certificate owner. This name must match the existing name of the security role.
 
+> [!NOTE]
+> **Attribute contract**: omitting `owner_role_name` from config leaves ownership unmanaged -- Terraform never sends a clearing value, and drift from an out-of-band owner change is still surfaced on plan/refresh. Declaring an explicit empty string (`owner_role_name = ""`) is a declarative "clear the owner" sentinel: Terraform sends a PUT with no role identifier, which Keyfactor Command interprets as removing the certificate's owner.
+
 Expanded Change Owner Permission: A user who holds the Certificates > Expanded Change Owner permission can set the certificate owner to any role within the permission sets they are a member of. This permission setting overrides the Certificates > Collections > Change Owner permission (both Global and Collection-level) if both are set.
 
 Collections > Change Owner Permission:

keyfactor/attribute_contract.go

@@ -7,6 +7,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/diag"
 	"github.com/hashicorp/terraform-plugin-framework/path"
 	"github.com/hashicorp/terraform-plugin-framework/tfsdk"
+	"github.com/hashicorp/terraform-plugin-framework/types"
 	"github.com/hashicorp/terraform-plugin-go/tftypes"
 )
 
@@ -153,6 +154,33 @@ func getConfigAttributeValue(ctx context.Context, config tfsdk.Config, p path.Pa
 	return v, diags
 }
 
+// keepStringSentinel implements sentinel stability (attribute contract item
+// 4) for a single Optional+Computed string attribute whose empty string ""
+// is a declarative "clear" sentinel (as opposed to Null, which means
+// "undeclared/unmanaged") -- e.g. certificate's owner_role_name. The wire
+// protocol for these attributes typically cannot distinguish "the server
+// genuinely reports nothing" from "the practitioner declared an explicit
+// clear": both surface as serverValue == "".
+//
+// When serverValue is empty AND prior (the previous state value on Read, or
+// the plan value when constructing post-apply state) was itself the ""
+// sentinel, this returns the "" sentinel (Known, non-null) rather than
+// collapsing to Null. Without this, an Optional+Computed attribute plans the
+// declared "" again on every subsequent plan (config still says ""), while
+// state kept flipping to Null -- a permanent spurious `"" -> null -> ""`
+// diff. When serverValue is non-empty, that is a genuine value (freshly set,
+// or changed out-of-band) and must always be surfaced, never masked by a
+// stale sentinel.
+func keepStringSentinel(serverValue string, prior types.String) types.String {
+	if serverValue != "" {
+		return types.String{Value: serverValue}
+	}
+	if !prior.Null && !prior.Unknown && prior.Value == "" {
+		return types.String{Value: ""}
+	}
+	return types.String{Null: true}
+}
+
 // nullValueOfType builds a null attr.Value sharing the same underlying type
 // as an existing (possibly Unknown) attr.Value, generically -- works for any
 // attr.Type via the ValueFromTerraform/TerraformType round-trip, so this

keyfactor/resource_keyfactor_certificate.go

@@ -410,10 +410,15 @@ func (r resourceCommandCertificateType) GetSchema(_ context.Context) (tfsdk.Sche
 				Computed: true,
 				Description: "Optional owner role name. " +
 					"This is required if the certificate template being used requires an owner role to be set during" +
-					" enrollment. Only compatible with Keyfactor Command versions v12.3.0 and later.",
+					" enrollment. Only compatible with Keyfactor Command versions v12.3.0 and later. " +
+					"Omitting this attribute leaves ownership unmanaged/preserved (server-side changes are not corrected); " +
+					"an explicit empty string (\"\") declaratively clears the certificate's owner.",
 				MarkdownDescription: `
 A string containing the name of the security role assigned as the certificate owner. This name must match the existing name of the security role.
 
+> [!NOTE]
+> **Attribute contract**: omitting ` + "`owner_role_name`" + ` from config leaves ownership unmanaged -- Terraform never sends a clearing value, and drift from an out-of-band owner change is still surfaced on plan/refresh. Declaring an explicit empty string (` + "`owner_role_name = \"\"`" + `) is a declarative "clear the owner" sentinel: Terraform sends a PUT with no role identifier, which Keyfactor Command interprets as removing the certificate's owner.
+
 Expanded Change Owner Permission: A user who holds the Certificates > Expanded Change Owner permission can set the certificate owner to any role within the permission sets they are a member of. This permission setting overrides the Certificates > Collections > Change Owner permission (both Global and Collection-level) if both are set.
 
 Collections > Change Owner Permission:
@@ -1210,9 +1215,9 @@ func (r resourceCommandCertificate) Read(
 		enrollmentPatternName = state.EnrollmentPattern.Value
 	}
 
-	var ownerRoleName string
-	if certGetResp != nil && certGetResp.OwnerRoleName != "" {
-		ownerRoleName = certGetResp.OwnerRoleName
+	var serverOwnerRoleName string
+	if certGetResp != nil {
+		serverOwnerRoleName = certGetResp.OwnerRoleName
 	}
 	tflog.Debug(ctx, "Creating state object for certificate.")
 	result := CommandCertificate{
@@ -1262,10 +1267,10 @@ func (r resourceCommandCertificate) Read(
 			Value: certGetResp != nil && strings.Contains(strings.ToLower(certGetResp.CertStateString), "pending"),
 		},
 		RenewalConfig: renewalConfig,
-		OwnerRoleName: types.String{
-			Value: ownerRoleName,
-			Null:  isNullString(ownerRoleName),
-		},
+		// Sentinel stability (attribute contract item 4): keep the "" clear
+		// sentinel if the server reports no owner AND prior state itself held
+		// the sentinel; otherwise write server truth (drift-visible).
+		OwnerRoleName:       keepStringSentinel(serverOwnerRoleName, state.OwnerRoleName),
 		EnrollmentPattern:   state.EnrollmentPattern,   // This may be mutated below
 		CertificateTemplate: state.CertificateTemplate, // This may be mutated below
 		NotBefore: types.String{
@@ -1430,6 +1435,34 @@ func certificateOwnerRoleChanged(plan, state CommandCertificate) bool {
 	return plan.OwnerRoleName.Value != state.OwnerRoleName.Value
 }
 
+// ownerChangeRequestForPlan builds the PUT /Certificates/{id}/Owner payload
+// for a declared owner_role_name plan value. Only called when
+// certificateOwnerRoleChanged reports a change, so ownerRoleName is always a
+// Known plan value here (never Null/Unknown).
+//
+// An explicit empty string ("") is the declarative "clear ownership"
+// sentinel (attribute contract item 3): per the Keyfactor Command API (PUT
+// /Certificates/{id}/Owner Swagger doc: "If removing the owner, leave both
+// empty"), a request with both NewRoleId and NewRoleName unset clears the
+// certificate's owner -- verified live against a v25.5 lab: HTTP 204,
+// subsequent GET shows OwnerRoleId/OwnerRoleName null. api.OwnerRequest's
+// fields are `omitempty` pointers, so &api.OwnerRequest{} serializes to
+// exactly `{}` (a non-nil pointer to an empty string, by contrast, is NOT
+// omitted by encoding/json -- omitempty only checks for a nil pointer -- so
+// this must be a genuinely nil field, not `NewRoleName: &""`).
+//
+// A non-empty value may be either a numeric role ID or a role name; try
+// parsing as an int first and fall back to treating it as a name.
+func ownerChangeRequestForPlan(ownerRoleName string) *api.OwnerRequest {
+	if ownerRoleName == "" {
+		return &api.OwnerRequest{}
+	}
+	if ownerInt, convErr := strconv.Atoi(ownerRoleName); convErr == nil {
+		return &api.OwnerRequest{NewRoleId: &ownerInt}
+	}
+	return &api.OwnerRequest{NewRoleName: &ownerRoleName}
+}
+
 func (r resourceCommandCertificate) Update(
 	ctx context.Context,
 	request tfsdk.UpdateResourceRequest,
@@ -1567,14 +1600,7 @@ func (r resourceCommandCertificate) Update(
 	// Check if ownerrolename has changed
 	if certificateOwnerRoleChanged(plan, state) {
 		tflog.Debug(ctx, "OwnerRoleName has changed, updating certificate owner role.")
-		// Check if rolename is an integer ID or string name
-		ownerInt, convErr := strconv.Atoi(plan.OwnerRoleName.Value)
-		ownerRequest := &api.OwnerRequest{}
-		if convErr != nil {
-			ownerRequest.NewRoleName = &plan.OwnerRoleName.Value
-		} else {
-			ownerRequest.NewRoleId = &ownerInt
-		}
+		ownerRequest := ownerChangeRequestForPlan(plan.OwnerRoleName.Value)
 
 		oErr := r.p.client.ChangeCertificateOwnerRole(certificateID, ownerRequest)
 		if oErr != nil {

keyfactor/resource_keyfactor_certificate_unit_test.go

@@ -2,8 +2,11 @@ package keyfactor
 
 import (
 	"context"
+	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"github.com/Keyfactor/keyfactor-go-client/v3/api"
@@ -212,3 +215,101 @@ func TestUnitKeyfactorCertificateResource_OwnerRoleNameIsComputed(t *testing.T)
 		t.Errorf("owner_role_name: expected UseStateForUnknown plan modifier, but none was found (modifiers: %+v)", attr.PlanModifiers)
 	}
 }
+
+// TestUnitCertificateOwnerRoleClearSendsEmptyPayload is a regression test for
+// declaratively clearing owner_role_name (G1). Per the verified live Command
+// v25.5 behavior (Swagger doc: "If removing the owner, leave both empty" --
+// confirmed with a real PUT: HTTP 204, subsequent GET shows
+// OwnerRoleId/OwnerRoleName null), the wire payload for clearing ownership
+// must omit BOTH NewRoleId and NewRoleName -- i.e. `{}`.
+//
+// Before this fix, ownerChangeRequestForPlan("") reproduced the historical
+// inline Update() logic: strconv.Atoi("") fails, so it fell into the
+// "treat as name" branch and sent {"NewRoleName":""} -- a non-empty pointer
+// to an empty string is NOT omitted by Go's `omitempty` (only a nil pointer
+// is), so this is NOT the verified clear payload and is an unverified,
+// possibly-rejected wire shape.
+//
+// This drives the real HTTP call through api.Client.ChangeCertificateOwnerRole
+// so the assertion exercises actual JSON-over-the-wire shaping, not just the
+// in-memory struct.
+func TestUnitCertificateOwnerRoleClearSendsEmptyPayload(t *testing.T) {
+	const certID = 845070
+
+	var capturedBody string
+	var sawRequest bool
+	mux := http.NewServeMux()
+	mux.HandleFunc(fmt.Sprintf("/KeyfactorAPI/Certificates/%d/Owner", certID), func(w http.ResponseWriter, r *http.Request) {
+		sawRequest = true
+		body, _ := io.ReadAll(r.Body)
+		capturedBody = strings.TrimSpace(string(body))
+		w.WriteHeader(http.StatusNoContent)
+	})
+	server := httptest.NewTLSServer(mux)
+	defer server.Close()
+
+	client := newCertUpdateMockClient(server)
+
+	state := CommandCertificate{OwnerRoleName: types.String{Value: "Administrator"}}
+	plan := CommandCertificate{OwnerRoleName: types.String{Value: ""}}
+
+	if !certificateOwnerRoleChanged(plan, state) {
+		t.Fatalf("expected certificateOwnerRoleChanged to report a change when explicitly clearing a declared owner")
+	}
+
+	ownerRequest := ownerChangeRequestForPlan(plan.OwnerRoleName.Value)
+	if err := client.ChangeCertificateOwnerRole(certID, ownerRequest); err != nil {
+		t.Fatalf("ChangeCertificateOwnerRole returned error: %v", err)
+	}
+
+	if !sawRequest {
+		t.Fatalf("expected a PUT to /Certificates/%d/Owner, got none", certID)
+	}
+	if capturedBody != "{}" {
+		t.Fatalf("expected the verified clear payload {} (both NewRoleId/NewRoleName omitted), got %q", capturedBody)
+	}
+}
+
+// TestUnitCertificateOwnerRoleClearNoopWhenAlreadyEmpty verifies that
+// declaring owner_role_name = "" against a certificate that already has no
+// owner (prior state Null, server-value "") is a no-op: certificateOwnerRoleChanged
+// must not report a change, so Update never calls the owner endpoint at all.
+func TestUnitCertificateOwnerRoleClearNoopWhenAlreadyEmpty(t *testing.T) {
+	state := CommandCertificate{OwnerRoleName: types.String{Null: true}}
+	plan := CommandCertificate{OwnerRoleName: types.String{Value: ""}}
+
+	if certificateOwnerRoleChanged(plan, state) {
+		t.Fatalf("expected no owner role change when clearing an already-empty owner, but certificateOwnerRoleChanged returned true")
+	}
+}
+
+// TestUnitCertificateReadKeepsOwnerClearSentinel is a regression test for
+// sentinel stability (attribute contract item 4) on owner_role_name Read.
+// When the server reports no owner (serverValue == "") AND the prior state
+// itself declared the "" clear sentinel, Read must keep "" (Known,
+// non-null) rather than collapsing to Null -- otherwise the very next plan
+// would show a spurious `"" -> null -> ""` diff forever, since config still
+// declares owner_role_name = "".
+//
+// On the unfixed keepStringSentinel (which unconditionally applies
+// isNullString), an empty serverValue always collapses to Null regardless of
+// the prior sentinel, so this is red before the fix.
+func TestUnitCertificateReadKeepsOwnerClearSentinel(t *testing.T) {
+	prior := types.String{Value: ""}
+	got := keepStringSentinel("", prior)
+	if got.Null || got.Value != "" {
+		t.Fatalf("expected the \"\" clear sentinel to be preserved (Value=\"\", Null=false), got %+v", got)
+	}
+}
+
+// TestUnitCertificateReadSurfacesOwnerDriftOverSentinel verifies the other
+// half of sentinel stability: if a REAL owner appears out-of-band (server
+// reports a non-empty value) while prior state held the "" clear sentinel,
+// Read must surface that as drift -- never mask it with the stale sentinel.
+func TestUnitCertificateReadSurfacesOwnerDriftOverSentinel(t *testing.T) {
+	prior := types.String{Value: ""}
+	got := keepStringSentinel("SomeRole", prior)
+	if got.Null || got.Value != "SomeRole" {
+		t.Fatalf("expected server-side drift to be surfaced (Value=\"SomeRole\", Null=false), got %+v", got)
+	}
+}