fix(adt): deploy session ordering + correct MODIFICATION_SUPPORT handling

pkg/adt/config.go

@@ -244,18 +244,38 @@ func (c *Config) NewHTTPClient() *http.Client {
 		Timeout:   c.Timeout,
 	}
 
-	// Preserve Authorization header across redirects.
-	// Go's default strips it per RFC 7235 §4.2, but SAP BTP/Cloud
-	// authentication flows require it to survive redirects.
-	// Without this, BTP users get 401 even though curl works (issue #90).
+	// Preserve ADT-critical headers across redirects.
+	//
+	// Go's default strips Authorization / WWW-Authenticate / Cookie / Cookie2
+	// on cross-origin redirects per RFC 7235 §4.2 — SAP BTP/Cloud SAML flows
+	// need Authorization back, otherwise the IdP dance drops it and the user
+	// gets 401 even though curl works (issue #90).
+	//
+	// Custom headers like X-CSRF-Token and X-sap-adt-sessiontype are *not*
+	// in Go's sensitive-headers list, so Go technically forwards them by
+	// default. We re-set them explicitly anyway for two reasons:
+	//   - defensive: guards against any Go version or middleware tweak that
+	//     decides to strip custom headers on its own;
+	//   - intent: makes it obvious in the code that these two headers are
+	//     load-bearing for the lock→write→unlock ADT sequence. If either
+	//     goes missing across a redirect, the second hop hits SAP with a
+	//     fresh (stateless) session-type or a missing CSRF token, and the
+	//     lock handle / mutation is rejected.
 	client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
 		if len(via) >= 10 {
 			return fmt.Errorf("too many redirects")
 		}
 		if len(via) > 0 {
-			if auth := via[0].Header.Get("Authorization"); auth != "" {
+			first := via[0]
+			if auth := first.Header.Get("Authorization"); auth != "" {
 				req.Header.Set("Authorization", auth)
 			}
+			if csrf := first.Header.Get("X-CSRF-Token"); csrf != "" {
+				req.Header.Set("X-CSRF-Token", csrf)
+			}
+			if st := first.Header.Get("X-sap-adt-sessiontype"); st != "" {
+				req.Header.Set("X-sap-adt-sessiontype", st)
+			}
 		}
 		return nil
 	}

pkg/adt/config_test.go

@@ -202,3 +202,81 @@ func TestSessionTypes(t *testing.T) {
 		t.Errorf("SessionKeep = %v, want keep", SessionKeep)
 	}
 }
+
+// TestNewHTTPClient_CheckRedirectPreservesADTHeaders verifies that the
+// HTTP client's CheckRedirect callback re-sets headers that are
+// load-bearing for the ADT lock→write→unlock sequence across redirects:
+//   - Authorization: Go strips this by default on cross-origin redirects
+//     (sensitive header per RFC 7235). Without restoration, SAML flows get
+//     401 even when curl works (issue #90).
+//   - X-CSRF-Token: mutation requests are rejected as CSRF-violating if
+//     this disappears mid-sequence.
+//   - X-sap-adt-sessiontype: the lock handle is bound to a stateful
+//     session; if a redirect hop lands stateless, the subsequent PUT
+//     can't find the lock and gets HTTP 423.
+//
+// Go's default forwards custom headers on same-origin redirects, but this
+// test pins the behaviour explicitly so future refactors can't silently
+// drop them.
+func TestNewHTTPClient_CheckRedirectPreservesADTHeaders(t *testing.T) {
+	cfg := NewConfig("https://sap.example.com:44300", "user", "pass")
+	client := cfg.NewHTTPClient()
+
+	if client.CheckRedirect == nil {
+		t.Fatal("CheckRedirect must be set")
+	}
+
+	// Build the "via" chain: the original request that carries the ADT
+	// headers we expect to be re-set on the redirect target.
+	orig, err := http.NewRequest(http.MethodPost,
+		"https://sap.example.com:44300/sap/bc/adt/oo/classes/ZFOO?_action=LOCK", nil)
+	if err != nil {
+		t.Fatalf("NewRequest(orig): %v", err)
+	}
+	orig.Header.Set("Authorization", "Basic dXNlcjpwYXNz")
+	orig.Header.Set("X-CSRF-Token", "ABC123==")
+	orig.Header.Set("X-sap-adt-sessiontype", "stateful")
+
+	// The redirect target that Go would follow — initially without any
+	// of the ADT headers (simulating Go having stripped them cross-origin).
+	next, err := http.NewRequest(http.MethodPost,
+		"https://sap.example.com:44300/sap/bc/adt/follow", nil)
+	if err != nil {
+		t.Fatalf("NewRequest(next): %v", err)
+	}
+
+	if err := client.CheckRedirect(next, []*http.Request{orig}); err != nil {
+		t.Fatalf("CheckRedirect returned error: %v", err)
+	}
+
+	if got := next.Header.Get("Authorization"); got != "Basic dXNlcjpwYXNz" {
+		t.Errorf("Authorization = %q, want preserved from initial request (issue #90)", got)
+	}
+	if got := next.Header.Get("X-CSRF-Token"); got != "ABC123==" {
+		t.Errorf("X-CSRF-Token = %q, want preserved — mutation requests need it to survive redirect hops", got)
+	}
+	if got := next.Header.Get("X-sap-adt-sessiontype"); got != "stateful" {
+		t.Errorf("X-sap-adt-sessiontype = %q, want preserved — lock handles are bound to a stateful session (issue #88)", got)
+	}
+}
+
+// TestNewHTTPClient_CheckRedirectHonoursLimit guards the 10-redirect cap
+// that CheckRedirect enforces to prevent infinite loops.
+func TestNewHTTPClient_CheckRedirectHonoursLimit(t *testing.T) {
+	cfg := NewConfig("https://sap.example.com:44300", "user", "pass")
+	client := cfg.NewHTTPClient()
+
+	if client.CheckRedirect == nil {
+		t.Fatal("CheckRedirect must be set")
+	}
+
+	next, _ := http.NewRequest(http.MethodGet, "https://sap.example.com:44300/", nil)
+	via := make([]*http.Request, 10)
+	for i := range via {
+		via[i], _ = http.NewRequest(http.MethodGet, "https://sap.example.com:44300/", nil)
+	}
+
+	if err := client.CheckRedirect(next, via); err == nil {
+		t.Error("CheckRedirect must return an error on the 10th redirect")
+	}
+}

pkg/adt/crud.go

@@ -57,23 +57,27 @@ func (c *Client) LockObject(ctx context.Context, objectURL string, accessMode st
 		return nil, err
 	}
 
-	// BTP / ABAP Cloud systems sometimes return a successful lock with
-	// MODIFICATION_SUPPORT="NoModification" — the lock acquired but the
-	// object is read-only via ADT (typical for SAP-delivered objects in
-	// hyperfocused mode, or systems where the user lacks the edit role).
-	// Without this guard the caller proceeds to PUT/POST and gets a
-	// confusing 423 InvalidLockHandle several seconds later. Surface it
-	// upfront so the user sees a clear, actionable error (issue #91).
-	if accessMode == "MODIFY" && strings.EqualFold(result.ModificationSupport, "NoModification") {
-		return nil, fmt.Errorf(
-			"object %s is not modifiable via ADT on this system "+
-				"(SAP returned modificationSupport=%q during LOCK). "+
-				"Common causes: read-only system class, missing developer/edit role, "+
-				"BTP ABAP Environment object outside the customer namespace, "+
-				"or hyperfocused mode locking the object as read-only",
-			objectURL, result.ModificationSupport)
-	}
-
+	// MODIFICATION_SUPPORT carries SAP-side policy metadata about how the
+	// object's changes are tracked, not whether the caller may modify it.
+	// IF_ADT_LOCK_RESULT in SAP's standard ADT defines four values:
+	//
+	//   "ModifcationAssistant"  (CO_MOD_SUPPORT_MODASS)         — SAP/partner
+	//       object with Modification Assistant; changes are tracked.
+	//   "ModificationsLoggedOnly" (CO_MOD_SUPPORT_LOGGED_ONLY) — SAP/partner
+	//       object; changes are only logged (no Modification Assistant).
+	//   "NoModification" (CO_MOD_SUPPORT_NOT_NEEDED) — customer-namespace
+	//       object; tracking is not needed. This is the normal response
+	//       for Z*/Y* objects that customers edit freely.
+	//   "" (CO_MOD_SUPPORT_NOT_SPECIFIED) — not specified.
+	//
+	// A valid LOCK_HANDLE plus any of these values means the caller got the
+	// lock. The previous guard that rejected "NoModification" at LOCK time
+	// (issue #91 attempt) was a misreading of the string — the constant is
+	// *NOT_NEEDED*, not *NOT_PERMITTED*. Genuine read-only rejections
+	// surface at other layers (HTTP 403 on the LOCK itself, or 423 on the
+	// subsequent write if session affinity is broken); the correct place
+	// to handle those is where they originate. Here we just return the
+	// parsed result verbatim.
 	return result, nil
 }
 

pkg/adt/crud_reconcile_test.go

@@ -550,15 +550,41 @@ func TestCreateTestInclude_UsesStatefulSession(t *testing.T) {
 	}
 }
 
-// TestLockObject_RejectsNoModification covers the BTP / ABAP Cloud
-// case from issue #91: a successful LOCK can return
-// MODIFICATION_SUPPORT=NoModification to signal that the object is
-// read-only via ADT for this user/system. Before the fix the caller
-// proceeded to PUT and got a confusing 423 InvalidLockHandle several
-// seconds later. The expected behaviour is to fail at the LOCK call
-// with a clear, actionable error message.
-func TestLockObject_RejectsNoModification(t *testing.T) {
-	const noModLockXML = `<?xml version="1.0" encoding="UTF-8"?>
+// TestLockObject_PassesThroughModificationSupport pins the correct
+// semantics of the MODIFICATION_SUPPORT field on the ADT lock response.
+//
+// The SAP standard interface IF_ADT_LOCK_RESULT defines four values:
+//
+//   - "ModifcationAssistant"    (CO_MOD_SUPPORT_MODASS)       — SAP/partner
+//     object, Modification Assistant on.
+//   - "ModificationsLoggedOnly" (CO_MOD_SUPPORT_LOGGED_ONLY)  — SAP/partner
+//     object, no Modification Assistant (changes only logged).
+//   - "NoModification"          (CO_MOD_SUPPORT_NOT_NEEDED)   — customer
+//     namespace object; modification tracking is NOT NEEDED. This is the
+//     usual response when a developer edits their own Z*/Y* code.
+//   - ""                        (CO_MOD_SUPPORT_NOT_SPECIFIED) — not set.
+//
+// The string "NoModification" therefore means "no tracking needed", not
+// "no edit allowed". An earlier commit (22517d4) hard-failed on this value
+// as if it meant read-only, which broke every normal customer-code edit
+// on destinations that populate the field (e.g. SAP Business Application
+// Studio via Cloud Connector + Principal Propagation). This test locks
+// the corrected behaviour: LockObject returns the parsed result verbatim
+// for any value of MODIFICATION_SUPPORT, and callers treat it as
+// advisory metadata only.
+func TestLockObject_PassesThroughModificationSupport(t *testing.T) {
+	cases := []struct {
+		name    string
+		support string
+	}{
+		{"customer-namespace-not-needed", "NoModification"},
+		{"sap-modification-assistant", "ModifcationAssistant"},
+		{"sap-logged-only", "ModificationsLoggedOnly"},
+		{"not-specified", ""},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			lockXML := `<?xml version="1.0" encoding="UTF-8"?>
 <asx:abap xmlns:asx="http://www.sap.com/abapxml" version="1.0">
   <asx:values>
     <DATA>
@@ -568,40 +594,45 @@ func TestLockObject_RejectsNoModification(t *testing.T) {
       <CORRTEXT></CORRTEXT>
       <IS_LOCAL></IS_LOCAL>
       <IS_LINK_UP></IS_LINK_UP>
-      <MODIFICATION_SUPPORT>NoModification</MODIFICATION_SUPPORT>
+      <MODIFICATION_SUPPORT>` + tc.support + `</MODIFICATION_SUPPORT>
     </DATA>
   </asx:values>
 </asx:abap>`
-	mock := &methodPathMock{
-		routes: []routedResponse{
-			resp("", "discovery", 200, "ok"),
-			resp(http.MethodPost, "/oo/classes/ZREADONLY", 200, noModLockXML),
-		},
-	}
-	cfg := NewConfig("https://sap.example.com:44300", "user", "pass")
-	transport := NewTransportWithClient(cfg, mock)
-	client := NewClientWithTransport(cfg, transport)
-
-	_, err := client.LockObject(
-		context.Background(),
-		"/sap/bc/adt/oo/classes/ZREADONLY",
-		"MODIFY",
-	)
-	if err == nil {
-		t.Fatal("LockObject should have returned an error for NoModification, got nil")
-	}
-	if !strings.Contains(err.Error(), "not modifiable") {
-		t.Errorf("error = %q, want to contain \"not modifiable\"", err.Error())
-	}
-	if !strings.Contains(err.Error(), "NoModification") {
-		t.Errorf("error = %q, want to surface the raw modificationSupport value", err.Error())
+			mock := &methodPathMock{
+				routes: []routedResponse{
+					resp("", "discovery", 200, "ok"),
+					resp(http.MethodPost, "/oo/classes/ZOBJ", 200, lockXML),
+				},
+			}
+			cfg := NewConfig("https://sap.example.com:44300", "user", "pass")
+			transport := NewTransportWithClient(cfg, mock)
+			client := NewClientWithTransport(cfg, transport)
+
+			result, err := client.LockObject(
+				context.Background(),
+				"/sap/bc/adt/oo/classes/ZOBJ",
+				"MODIFY",
+			)
+			if err != nil {
+				t.Fatalf("LockObject(MODIFY) must not fail on MODIFICATION_SUPPORT=%q, got error: %v", tc.support, err)
+			}
+			if result == nil {
+				t.Fatal("LockObject returned nil result")
+			}
+			if result.LockHandle != "HANDLE-X" {
+				t.Errorf("LockHandle = %q, want HANDLE-X", result.LockHandle)
+			}
+			if result.ModificationSupport != tc.support {
+				t.Errorf("ModificationSupport = %q, want %q (must be passed through verbatim)", result.ModificationSupport, tc.support)
+			}
+		})
 	}
 }
 
-// TestLockObject_AllowsNoModificationOnReadLock proves the guard is
-// scoped to MODIFY locks — read-only locks (accessMode != MODIFY)
-// must still succeed even if the system flags the object as not
-// modifiable, because there is no write to fail downstream.
+// TestLockObject_AllowsNoModificationOnReadLock exercises the READ access
+// mode path, which skips the OpLock safety check. Kept as a companion to
+// TestLockObject_PassesThroughModificationSupport so the READ branch
+// keeps explicit coverage for the same metadata-only behaviour.
 func TestLockObject_AllowsNoModificationOnReadLock(t *testing.T) {
 	const noModLockXML = `<?xml version="1.0" encoding="UTF-8"?>
 <asx:abap xmlns:asx="http://www.sap.com/abapxml" version="1.0">
@@ -639,3 +670,4 @@ func TestLockObject_AllowsNoModificationOnReadLock(t *testing.T) {
 		t.Errorf("LockHandle = %q, want HANDLE-X", result.LockHandle)
 	}
 }
+