Restore trailing slash on Sentry API requests
Commitf5703d390replaced the fmt.Sprintf URL construction with url.JoinPath, which calls path.Join and therefore strips a trailing slash unless the final element carries one. Sentry's API only routes slashed paths and answers 404 without redirecting, so every ListAccounts call failed on its first request and no access-review campaign targeting Sentry could fetch a single account. The failure was invisible for two reasons. queryMembers maps 404 to errSentryOrgNotAccessible, so a routing bug surfaced to users as "reconnect the connector with the correct organization" -- advice that could never help, because the slug was never wrong. And commit74ce2bc5dedited the recorded request URL in testdata/sentry.yaml to match the new construction instead of re-recording the cassette, which kept CI green; that cassette still carries Sentry's own Link header with the trailing slash, contradicting its own request line. Pass the slash on the final JoinPath element in both the members endpoint and the organization name resolver, revert the cassette to the URL Sentry actually served, and add a regression test that drives the driver against a server which 404s unslashed paths, so the URL shape is pinned independently of the cassette matcher. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
This commit is contained in:
@@ -708,7 +708,8 @@ func (r *sentryNameResolver) ResolveInstanceName(ctx context.Context) (string, e
|
||||
return "", nil
|
||||
}
|
||||
|
||||
endpoint, err := url.JoinPath("https://sentry.io", "api", "0", "organizations", url.PathEscape(r.orgSlug))
|
||||
// Trailing slash required; see SentryDriver.ListAccounts.
|
||||
endpoint, err := url.JoinPath("https://sentry.io", "api", "0", "organizations", url.PathEscape(r.orgSlug)+"/")
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("cannot build sentry organization URL: %w", err)
|
||||
}
|
||||
|
||||
@@ -170,7 +170,7 @@ func TestSentryNameResolver(t *testing.T) {
|
||||
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
assert.Equal(t, http.MethodGet, r.Method)
|
||||
assert.Equal(t, "/api/0/organizations/acme", r.URL.Path)
|
||||
assert.Equal(t, "/api/0/organizations/acme/", r.URL.Path)
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(tc.status)
|
||||
_, _ = w.Write([]byte(tc.body))
|
||||
|
||||
@@ -101,7 +101,10 @@ func (d *SentryDriver) ListAccounts(ctx context.Context) ([]AccountRecord, error
|
||||
|
||||
var records []AccountRecord
|
||||
|
||||
nextURL, err := url.JoinPath("https://sentry.io", "api", "0", "organizations", url.PathEscape(orgSlug), "members")
|
||||
// The trailing slash is required: Sentry's API only routes slashed
|
||||
// paths and answers 404 (without redirecting) otherwise. url.JoinPath
|
||||
// keeps it only when the last element carries it.
|
||||
nextURL, err := url.JoinPath("https://sentry.io", "api", "0", "organizations", url.PathEscape(orgSlug), "members/")
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("cannot build sentry members URL: %w", err)
|
||||
}
|
||||
|
||||
@@ -25,6 +25,7 @@ import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
@@ -54,12 +55,48 @@ func TestSentryDriver(t *testing.T) {
|
||||
assert.NotEmpty(t, r.Roles)
|
||||
}
|
||||
|
||||
// TestSentryDriverRequestsTrailingSlashPaths pins the exact request paths
|
||||
// against Sentry's real routing: its API is Django-based and only matches
|
||||
// paths ending in a slash, answering 404 (no redirect) otherwise. A 404 is
|
||||
// indistinguishable from a revoked membership here, so dropping the slash
|
||||
// silently turns every campaign fetch into a bogus "reconnect" error.
|
||||
func TestSentryDriverRequestsTrailingSlashPaths(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
var gotPaths []string
|
||||
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
gotPaths = append(gotPaths, r.URL.Path)
|
||||
|
||||
// Mimic Sentry: unslashed paths do not route.
|
||||
if !strings.HasSuffix(r.URL.Path, "/") {
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
_, _ = w.Write([]byte(`{"detail":"The requested resource does not exist"}`))
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(http.StatusOK)
|
||||
_, _ = w.Write([]byte(`[{"id":"42","email":"alice@example.com","name":"Alice","orgRole":"member"}]`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
client := &http.Client{Transport: &hostRewriter{target: srv.URL}}
|
||||
|
||||
records, err := NewSentryDriver(client, "acme-corp").ListAccounts(context.Background())
|
||||
require.NoError(t, err)
|
||||
require.Len(t, records, 1)
|
||||
assert.Equal(t, "alice@example.com", records[0].Email)
|
||||
assert.Equal(t, []string{"/api/0/organizations/acme-corp/members/"}, gotPaths)
|
||||
}
|
||||
|
||||
func TestSentryDriverListAccountsStaleSlug(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
assert.Equal(t, http.MethodGet, r.Method)
|
||||
assert.Equal(t, "/api/0/organizations/acme-old/members", r.URL.Path)
|
||||
assert.Equal(t, "/api/0/organizations/acme-old/members/", r.URL.Path)
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
_, _ = w.Write([]byte(`{"detail":"The requested resource does not exist"}`))
|
||||
}))
|
||||
@@ -88,7 +125,7 @@ func TestSentryDriverListAccountsAutoDiscoversSlug(t *testing.T) {
|
||||
assert.Equal(t, "true", r.URL.Query().Get("member"))
|
||||
w.WriteHeader(http.StatusOK)
|
||||
_, _ = w.Write([]byte(`[{"slug":"` + discoveredSlug + `","name":"Discovered Org"}]`))
|
||||
case "/api/0/organizations/" + discoveredSlug + "/members":
|
||||
case "/api/0/organizations/" + discoveredSlug + "/members/":
|
||||
w.WriteHeader(http.StatusOK)
|
||||
_, _ = w.Write([]byte(`[{"id":"42","email":"alice@example.com","name":"Alice","orgRole":"member"}]`))
|
||||
default:
|
||||
|
||||
@@ -8,7 +8,7 @@ interactions:
|
||||
proto_minor: 1
|
||||
content_length: 0
|
||||
host: sentry.io
|
||||
url: https://sentry.io/api/0/organizations/acme-corp/members
|
||||
url: https://sentry.io/api/0/organizations/acme-corp/members/
|
||||
method: GET
|
||||
response:
|
||||
proto: HTTP/2.0
|
||||
|
||||
Reference in New Issue
Block a user