diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 4fa62ec14..ad0e8e532 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -173,24 +173,11 @@ func WithScopes(scopes []string) Option { } } -// WithBrowserOpen overrides the default "open browser" functionality with a custom callback. If not specified, -// an implementation using https://github.com/pkg/browser will be used by default. -// -// Deprecated: this option will be removed in a future version of Pinniped. See the -// WithSkipBrowserOpen() option instead. -func WithBrowserOpen(openURL func(url string) error) Option { - return func(h *handlerState) error { - h.openURL = openURL - return nil - } -} - // WithSkipBrowserOpen causes the login to only print the authorize URL, but skips attempting to // open the user's default web browser. func WithSkipBrowserOpen() Option { return func(h *handlerState) error { h.skipBrowser = true - h.openURL = func(_ string) error { return nil } return nil } } @@ -629,14 +616,13 @@ func (h *handlerState) webBrowserBasedAuth(authorizeOptions *[]oauth2.AuthCodeOp // Open the authorize URL in the users browser, logging but otherwise ignoring any error. // Keep track of whether the browser was opened. - openedBrowser := true - if err := h.openURL(authorizeURL); err != nil { - openedBrowser = false - h.logger.V(plog.KlogLevelDebug).Error(err, "could not open browser") - } - if h.skipBrowser { - // We didn't actually try to open the browser in the above call to openURL(). - openedBrowser = false + openedBrowser := false + if !h.skipBrowser { + if err := h.openURL(authorizeURL); err != nil { + h.logger.V(plog.KlogLevelDebug).Error(err, "could not open browser") + } else { + openedBrowser = true + } } // Allow optionally skipping printing the login URL, for example because printing it may confuse @@ -669,9 +655,10 @@ func (h *handlerState) webBrowserBasedAuth(authorizeOptions *[]oauth2.AuthCodeOp // prompt to the screen and wait for user input, if needed. It can be cancelled by the context provided. // It returns a function which should be invoked by the caller to perform some cleanup. func (h *handlerState) promptForWebLogin(ctx context.Context, authorizeURL string, printAuthorizeURL bool) func() { - if printAuthorizeURL { - _, _ = fmt.Fprintf(h.out, "Log in by visiting this link:\n\n %s\n\n", authorizeURL) + if !printAuthorizeURL { + return func() {} } + _, _ = fmt.Fprintf(h.out, "Log in by visiting this link:\n\n %s\n\n", authorizeURL) // If stdin is not a TTY, don't prompt for the manual paste, since we have no way of reading it. if !h.stdinIsTTY() { diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index 7d71a8899..949419e9e 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -712,6 +712,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.stdinIsTTY = func() bool { return true } require.NoError(t, WithClient(buildHTTPClientForPEM(formPostSuccessServerCA))(h)) require.NoError(t, WithSkipListen()(h)) + h.skipBrowser = false // don't skip calling the following openURL func h.openURL = func(authorizeURL string) error { parsed, err := url.Parse(authorizeURL) require.NoError(t, err) @@ -751,6 +752,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.stdinIsTTY = func() bool { return true } require.NoError(t, WithClient(buildHTTPClientForPEM(formPostSuccessServerCA))(h)) h.listen = func(string, string) (net.Listener, error) { return nil, fmt.Errorf("some listen error") } + h.skipBrowser = false // don't skip calling the following openURL func h.openURL = func(authorizeURL string) error { parsed, err := url.Parse(authorizeURL) require.NoError(t, err) @@ -794,6 +796,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo ctx, cancel := context.WithCancel(h.ctx) h.ctx = ctx + h.skipBrowser = false // don't skip calling the following openURL func h.openURL = func(_ string) error { cancel() return nil @@ -824,6 +827,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } require.NoError(t, WithClient(buildHTTPClientForPEM(successServerCA))(h)) + h.skipBrowser = false // don't skip calling the following openURL func h.openURL = func(_ string) error { go func() { h.callbacks <- callbackResult{err: fmt.Errorf("some callback error")} @@ -876,6 +880,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo client.Timeout = 10 * time.Second require.NoError(t, WithClient(client)(h)) + h.skipBrowser = false // don't skip calling the following openURL func h.openURL = func(actualURL string) error { parsedActualURL, err := url.Parse(actualURL) require.NoError(t, err) @@ -950,7 +955,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo client.Timeout = 10 * time.Second require.NoError(t, WithClient(client)(h)) - h.skipBrowser = false + h.skipBrowser = false // don't skip calling the following openURL func h.openURL = func(actualURL string) error { parsedActualURL, err := url.Parse(actualURL) require.NoError(t, err) @@ -1016,7 +1021,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo client.Timeout = 10 * time.Second require.NoError(t, WithClient(client)(h)) - h.skipBrowser = false + h.skipBrowser = false // don't skip calling the following openURL func h.openURL = func(actualURL string) error { parsedActualURL, err := url.Parse(actualURL) require.NoError(t, err) @@ -1095,33 +1100,15 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.NoError(t, WithClient(client)(h)) h.skipBrowser = true - h.openURL = func(actualURL string) error { - parsedActualURL, err := url.Parse(actualURL) - require.NoError(t, err) - actualParams := parsedActualURL.Query() - require.Contains(t, actualParams.Get("redirect_uri"), "http://127.0.0.1:") - actualParams.Del("redirect_uri") + // Allow the login to finish so this test does not hang waiting for the callback, + // and so we can check if the authorize URL was shown on stderr. + // The openURL function will be skipped, so we can't put this code inside the + // mock version of that function as we do for other tests in this file. + go func() { + h.callbacks <- callbackResult{token: &testToken} + }() - require.Equal(t, url.Values{ - "code_challenge": []string{testCodeChallenge}, - "code_challenge_method": []string{"S256"}, - "response_type": []string{"code"}, - "scope": []string{"test-scope"}, - "nonce": []string{"test-nonce"}, - "state": []string{"test-state"}, - "access_type": []string{"offline"}, - "client_id": []string{"test-client-id"}, - }, actualParams) - - parsedActualURL.RawQuery = "" - require.Equal(t, successServer.URL+"/authorize", parsedActualURL.String()) - - go func() { - h.callbacks <- callbackResult{token: &testToken} - }() - return nil - } return nil } }, @@ -1185,6 +1172,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo client.Timeout = 10 * time.Second require.NoError(t, WithClient(client)(h)) + h.skipBrowser = false // don't skip calling the following openURL func h.openURL = func(actualURL string) error { parsedActualURL, err := url.Parse(actualURL) require.NoError(t, err) @@ -1261,6 +1249,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo client.Timeout = 10 * time.Second require.NoError(t, WithClient(client)(h)) + h.skipBrowser = false // don't skip calling the following openURL func h.openURL = func(actualURL string) error { parsedActualURL, err := url.Parse(actualURL) require.NoError(t, err) @@ -2415,7 +2404,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo WithContext(context.Background()), WithListenPort(0), WithScopes([]string{"test-scope"}), - WithSkipBrowserOpen(), + WithSkipBrowserOpen(), // Skip by default so we don't really open a browser. Each test can override this. tt.opt(t), WithLogger(testLogger.Logger), withOutWriter(t, &buffer), @@ -2591,14 +2580,12 @@ func TestHandlePasteCallback(t *testing.T) { }, }, { - name: "success, without printing auth url", + name: "skipping printing auth url (also skips prompting for authcode)", opt: func(t *testing.T) Option { return func(h *handlerState) error { h.stdinIsTTY = func() bool { return true } h.useFormPost = true - h.promptForValue = func(_ context.Context, promptLabel string, _ io.Writer) (string, error) { - return "valid", nil - } + h.promptForValue = nil // shouldn't get called, so can be nil h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) @@ -2611,11 +2598,8 @@ func TestHandlePasteCallback(t *testing.T) { } }, authorizeURL: testAuthURL, - printAuthorizeURL: false, // do not want to print auth URL - wantStderr: newlineAfterEveryAuthcodePromptOutput, // auth URL was not printed to stdout - wantCallback: &callbackResult{ - token: &oidctypes.Token{IDToken: &oidctypes.IDToken{Token: "test-id-token"}}, - }, + printAuthorizeURL: false, // do not want to print auth URL + wantStderr: "", // auth URL was not printed, and prompt for pasting authcode was also not printed }, } for _, tt := range tests {