Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CLI also skips authcode prompt when PINNIPED_SKIP_PRINT_LOGIN_URL=true #1938

Merged
merged 1 commit into from
May 7, 2024
Merged

CLI also skips authcode prompt when PINNIPED_SKIP_PRINT_LOGIN_URL=true #1938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 10 additions & 23 deletions pkg/oidcclient/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down
58 changes: 21 additions & 37 deletions pkg/oidcclient/login_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
joshuatcasey marked this conversation as resolved.
Show resolved Hide resolved
h.openURL = func(authorizeURL string) error {
parsed, err := url.Parse(authorizeURL)
require.NoError(t, err)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
},
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
Loading