diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 67f8349c..bd8bb24c 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -41,7 +41,9 @@ jobs: go run github.com/mxschmitt/playwright-go/cmd/playwright install chromium --with-deps - run: make lint - - run: go test -race ./... + # cmd/cloudx tests run against live staging, which go test cannot track, + # so a cached pass would hide a break on the staging side. + - run: go test -race -count=1 ./... env: ORY_RATE_LIMIT_HEADER: ${{ secrets.ORY_RATE_LIMIT_HEADER }} ORY_CONSOLE_URL: https://console.staging.ory.dev diff --git a/cmd/cloudx/client/auth.go b/cmd/cloudx/client/auth.go index c51d8507..4b38756a 100644 --- a/cmd/cloudx/client/auth.go +++ b/cmd/cloudx/client/auth.go @@ -273,32 +273,69 @@ func redirectErr(w http.ResponseWriter, r *http.Request, err, desc string) { http.Redirect(w, r, location.String(), http.StatusFound) } +// SignOut revokes the stored OAuth2 token and removes it from the local +// configuration. It reads the configuration file directly instead of going +// through GetAuthenticatedConfig, because signing out must not require a valid +// session: an expired or invalid one would otherwise start a new login, and +// RFC 7009 servers accept revocation requests for tokens that are already +// invalid. func (h *CommandHelper) SignOut(ctx context.Context) error { - config, err := h.GetAuthenticatedConfig(ctx) - if err != nil { + config, err := readConfig(h.configLocation) + if stderrors.Is(err, ErrNoConfig) { + return nil + } else if err != nil { return err } - if config.AccessToken == nil { - return h.ClearConfig() + if config.AccessToken != nil { + if err := revokeToken(ctx, config.AccessToken); err != nil { + _, _ = fmt.Fprintf(h.VerboseErrWriter, "failed to revoke access token, signing out locally only: %v\n", err) + } } + return h.ClearConfig() +} + +// revokeToken revokes the refresh token, which also revokes the access tokens +// issued with it. Tokens obtained without the offline_access scope have no +// refresh token, so the access token is revoked instead. +func revokeToken(ctx context.Context, token *oauth2.Token) error { + value, hint := token.RefreshToken, "refresh_token" + if value == "" { + value, hint = token.AccessToken, "access_token" + } + if value == "" { + return nil + } + client := oauth2ClientConfig() revoke, err := url.Parse(client.Endpoint.AuthURL) if err != nil { return err } revoke.Path = "/oauth2/revoke" - res, err := http.PostForm(revoke.String(), url.Values{ - "client_id": []string{client.ClientID}, - "token": []string{config.AccessToken.RefreshToken}, // this also revokes the associated access token - }) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, revoke.String(), strings.NewReader(url.Values{ + "client_id": {client.ClientID}, + "token": {value}, + "token_type_hint": {hint}, + }.Encode())) if err != nil { - _, _ = fmt.Fprintf(h.VerboseErrWriter, "failed to revoke access token: %v\n", err) - } else { - defer res.Body.Close() - if res.StatusCode < 200 || res.StatusCode > 299 { - body, _ := io.ReadAll(res.Body) - _, _ = fmt.Fprintf(h.VerboseErrWriter, "failed to revoke access token: %v\n", string(body)) - } + return err } - return h.ClearConfig() + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + // Honor the same context HTTP client that the oauth2 package uses for the + // token exchange and refresh requests against this server. + httpClient := http.DefaultClient + if c, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok && c != nil { + httpClient = c + } + res, err := httpClient.Do(req) + if err != nil { + return err + } + defer res.Body.Close() + if res.StatusCode < 200 || res.StatusCode > 299 { + body, _ := io.ReadAll(io.LimitReader(res.Body, 4096)) + return fmt.Errorf("unexpected status %d: %s", res.StatusCode, body) + } + return nil } diff --git a/cmd/cloudx/client/auth_test.go b/cmd/cloudx/client/auth_test.go new file mode 100644 index 00000000..7dcbaaa5 --- /dev/null +++ b/cmd/cloudx/client/auth_test.go @@ -0,0 +1,148 @@ +// Copyright © 2026 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package client_test + +import ( + "bytes" + "context" + "encoding/json" + "io" + "net/http" + "net/url" + "os" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + + "github.com/ory/cli/cmd/cloudx/client" + "github.com/ory/cli/cmd/cloudx/testhelpers" +) + +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +// revokeRecorder answers OAuth2 token revocation requests with the given +// status and fails the test on any other request, such as a token refresh. +func revokeRecorder(t *testing.T, status int) (*http.Client, *[]url.Values) { + var revoked []url.Values + return &http.Client{Transport: roundTripFunc(func(r *http.Request) (*http.Response, error) { + res := &http.Response{ + StatusCode: status, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader("")), + Request: r, + } + if r.Method != http.MethodPost || r.URL.Path != "/oauth2/revoke" { + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + res.StatusCode = http.StatusBadRequest + return res, nil + } + if assert.NoError(t, r.ParseForm()) { + revoked = append(revoked, r.PostForm) + } + return res, nil + })}, &revoked +} + +func TestSignOut(t *testing.T) { + expired := time.Now().Add(-time.Hour) + + writeConfig := func(t *testing.T, config any) string { + location := testhelpers.NewConfigFile(t) + raw, err := json.Marshal(config) + require.NoError(t, err) + require.NoError(t, os.WriteFile(location, raw, 0600)) + return location + } + + signOut := func(t *testing.T, location string, status int, opts ...client.CommandHelperOption) (revoked []url.Values, stderr string, err error) { + httpClient, rec := revokeRecorder(t, status) + ctx := context.WithValue(context.Background(), oauth2.HTTPClient, httpClient) + out := bytes.Buffer{} + h, err := client.NewCommandHelper(ctx, append([]client.CommandHelperOption{ + client.WithConfigLocation(location), + client.WithVerboseErrWriter(&out), + client.WithOpenBrowserHook(func(string) error { return testhelpers.ErrAuthFlowTriggered }), + }, opts...)...) + require.NoError(t, err) + err = h.SignOut(ctx) + return *rec, out.String(), err + } + + t.Run("revokes the refresh token of an expired session without signing in", func(t *testing.T) { + location := writeConfig(t, client.Config{ + Version: client.ConfigVersion, + AccessToken: &oauth2.Token{AccessToken: "access", RefreshToken: "refresh", Expiry: expired}, + }) + + revoked, _, err := signOut(t, location, http.StatusOK) + require.NoError(t, err) + + assert.Equal(t, []url.Values{{ + "client_id": {"ory-cli"}, + "token": {"refresh"}, + "token_type_hint": {"refresh_token"}, + }}, revoked) + assert.Nil(t, testhelpers.ReadConfig(t, location).AccessToken) + }) + + t.Run("revokes the access token if there is no refresh token", func(t *testing.T) { + location := writeConfig(t, client.Config{ + Version: client.ConfigVersion, + AccessToken: &oauth2.Token{AccessToken: "access", Expiry: expired}, + }) + + revoked, _, err := signOut(t, location, http.StatusOK) + require.NoError(t, err) + + assert.Equal(t, []url.Values{{ + "client_id": {"ory-cli"}, + "token": {"access"}, + "token_type_hint": {"access_token"}, + }}, revoked) + assert.Nil(t, testhelpers.ReadConfig(t, location).AccessToken) + }) + + t.Run("signs out locally if revocation fails", func(t *testing.T) { + location := writeConfig(t, client.Config{ + Version: client.ConfigVersion, + AccessToken: &oauth2.Token{AccessToken: "access", RefreshToken: "refresh", Expiry: time.Now().Add(time.Hour)}, + }) + + revoked, stderr, err := signOut(t, location, http.StatusServiceUnavailable) + require.NoError(t, err) + + assert.Len(t, revoked, 1) + assert.Contains(t, stderr, "failed to revoke") + assert.Nil(t, testhelpers.ReadConfig(t, location).AccessToken) + }) + + t.Run("does nothing without a config file", func(t *testing.T) { + location := testhelpers.NewConfigFile(t) + + revoked, _, err := signOut(t, location, http.StatusOK) + require.NoError(t, err) + + assert.Empty(t, revoked) + assert.NoFileExists(t, location) + }) + + t.Run("clears a legacy config", func(t *testing.T) { + location := writeConfig(t, map[string]string{"version": "v0alpha0", "session_token": "legacy"}) + + revoked, _, err := signOut(t, location, http.StatusOK, client.WithNoConfirm(true)) + require.NoError(t, err) + + assert.Empty(t, revoked) + raw, err := os.ReadFile(location) + require.NoError(t, err) + assert.NotContains(t, string(raw), "legacy") + assert.Equal(t, client.ConfigVersion, testhelpers.ReadConfig(t, location).Version) + }) +} diff --git a/cmd/cloudx/testhelpers/testhelpers.go b/cmd/cloudx/testhelpers/testhelpers.go index bc3619a0..bb0cd5f6 100644 --- a/cmd/cloudx/testhelpers/testhelpers.go +++ b/cmd/cloudx/testhelpers/testhelpers.go @@ -468,15 +468,15 @@ func redactInZip(path, secret string) error { return os.Rename(tmp.Name(), path) } -// submitPasswordForm submits the filled-in login form and fails immediately if -// Ory Network did not accept it. +// submitLoginStep submits the login form with the given method and fails +// immediately if Ory Network did not accept it. // -// It exists because the consent screen is the next thing the caller waits for. -// On a refusal the page never leaves the form, so an unchecked failure surfaces -// 30 seconds later as a missing `Allow` button, pointing at the consent screen -// instead of at the reason it never rendered: rejected credentials (400), 429 -// when a CI run drives more logins from one IP than Ory Network allows, or a 5xx. -func submitPasswordForm(t testing.TB, page playwright.Page) { +// It exists because every step is followed by a wait for the next screen. On a +// refusal the page never leaves the form, so an unchecked failure surfaces 30 +// seconds later as a missing field or `Allow` button, pointing at that screen +// instead of at the reason it never rendered: rejected credentials, 429 when a +// CI run drives more logins from one IP than Ory Network allows, or a 5xx. +func submitLoginStep(t testing.TB, page playwright.Page, method string) { // The login UI submits the flow with fetch, so this POST is the only request // to this path; the flow itself was created before the page rendered. isLoginSubmission := func(r playwright.Response) bool { @@ -484,22 +484,58 @@ func submitPasswordForm(t testing.TB, page playwright.Page) { } resp, err := page.ExpectResponse(isLoginSubmission, func() error { - return page.Locator(`[type="submit"][name="method"][value="password"]`).Click() + return page.Locator(fmt.Sprintf(`[type="submit"][name="method"][value=%q]`, method)).Click() }) - require.NoError(t, err, "the login form was never submitted") - - // A login that continues at the OAuth2 consent screen is answered with 422 - // browser_location_change_required, the only use of 422 in self-service - // flows. The login UI navigates away on it, which can discard the body - // before it is read, so the status alone has to decide. - if resp.Status() < http.StatusBadRequest || resp.Status() == http.StatusUnprocessableEntity { + require.NoError(t, err, "the %s login step was never submitted", method) + + switch resp.Status() { + case http.StatusUnprocessableEntity: + // A login that continues at the OAuth2 consent screen is answered with + // 422 browser_location_change_required, the only use of 422 in + // self-service flows. The login UI navigates away on it, which can + // discard the body before it is read, so the status alone has to decide. return + case http.StatusBadRequest: + // Kratos answers 400 both when it refuses a step and when the flow + // continues at its next step, as after the identifier of an + // identifier-first login. The UI renders that step in place, so the + // body is still there to tell the two apart. + if body, err := resp.Body(); err == nil && isNextLoginStep(body) { + return + } + default: + if resp.Status() < http.StatusBadRequest { + return + } } body, _ := resp.Text() require.FailNowf(t, "the login form was refused", - "POST %s\n%d %s\n%s\n\nThe consent screen never renders after this, so waiting for it would only time out.", - resp.URL(), resp.Status(), resp.StatusText(), body) + "POST %s (method %s)\n%d %s\n%s\n\nThe next login screen never renders after this, so waiting for it would only time out.", + resp.URL(), method, resp.Status(), resp.StatusText(), body) +} + +// isNextLoginStep reports whether a 400 answer to a login submission is the +// flow asking for its next step. Kratos answers a refusal with the flow too, +// but adds messages of type error to it; other errors are not a flow at all. +func isNextLoginStep(body []byte) bool { + var flow cloud.LoginFlow + if err := json.Unmarshal(body, &flow); err != nil { + return false + } + for _, m := range flow.Ui.Messages { + if m.Type == "error" { + return false + } + } + for _, n := range flow.Ui.Nodes { + for _, m := range n.Messages { + if m.Type == "error" { + return false + } + } + } + return true } func PlaywrightAcceptConsentBrowserHook(t testing.TB, page playwright.Page, email, password string) func(uri string) error { @@ -521,19 +557,23 @@ func PlaywrightAcceptConsentBrowserHook(t testing.TB, page playwright.Page, emai _, err := page.Goto(uri) require.NoError(t, err) - // The password field renders on a first login and when reconfirming an - // existing session, but only the first login asks for the identifier. - passwordInput := page.Locator(`[data-testid="ory/form/node/input/password"]`) - require.NoError(t, passwordInput.WaitFor()) - identifierInput := page.Locator(`[data-testid="ory/form/node/input/identifier"]`) + // Console login is identifier-first: a first login asks for the email on + // its own screen and only then for the password, while reconfirming an + // existing session starts at the password. The email stays in the form + // as a hidden field after its step, so only visible fields count. + identifierInput := page.Locator(`[data-testid="ory/form/node/input/identifier"]:visible`) + passwordInput := page.Locator(`[data-testid="ory/form/node/input/password"]:visible`) + require.NoError(t, identifierInput.Or(passwordInput).First().WaitFor()) if visible, err := identifierInput.IsVisible(); err == nil && visible { t.Logf("logging in") require.NoError(t, identifierInput.Fill(email)) + submitLoginStep(t, page, "identifier_first") + require.NoError(t, passwordInput.WaitFor()) } else { t.Logf("reconfirming password") } require.NoError(t, passwordInput.Fill(password)) - submitPasswordForm(t, page) + submitLoginStep(t, page, "password") // we wait here for the button +1s because there is some console bug that can lead to form submissions before the form action is correctly set if err := page.Locator(`button:has-text("Allow")`).WaitFor(); err != nil {