Fix panic on OIDC implicit login flows other than auth0

This fixes a regression introduced in dcos#1502. This issue only happens on non-desktop environments. https://jira.mesosphere.com/browse/DCOS-60349
bamarni · Oct 23, 2019 · f4c9109 · f4c9109
1 parent 253a400
commit f4c9109
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 57 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,12 @@
 
 ## Next
 
+## 1.1.1
+
+* Fixes
+
+  * Fix panic on OIDC implicit login flows other than auth0
+
 ## 1.1.0
 
 * Features

diff --git a/pkg/login/flow.go b/pkg/login/flow.go
@@ -32,6 +32,7 @@ type Flow struct {
 	prompt      *prompt.Prompt
 	logger      *logrus.Logger
 	opener      open.Opener
+	loginServer *loginserver.LoginServer
 	flags       *Flags
 	interactive bool
 }
@@ -157,46 +158,20 @@ func (f *Flow) triggerMethod(provider *Provider) (acsToken string, err error) {
 		// will be intercepted automatically through a temporary login server running on localhost.
 		case methodBrowserAuthToken, methodBrowserOIDCToken:
 			var token string
-			var loginServer *loginserver.LoginServer
 
 			if attempt == 1 {
-				startFlowURL := provider.Config.StartFlowURL
-
-				// For the "dcos-oidc-auth0" login provider ID, we start a local web server
-				// in order to avoid copy-pasting the token from the browser to the terminal.
-				if provider.ID == DCOSOIDCAuth0 {
-					loginServer, err = loginserver.New(startFlowURL)
-					if err != nil {
-						f.logger.Error(err)
-					} else {
-						startFlowURL = loginServer.StartFlowURL()
-						go loginServer.Start()
-						defer loginServer.Close()
-					}
-				}
-
-				if err := f.openBrowser(startFlowURL); err != nil {
-					// Being unable to open the browser is not critical, a message was prompted
-					// to the user with the URL they should go to in order to start the login flow.
-					f.logger.Info(err)
-
-					// However we close and don't use the web server in this scenario, as the CLI might
-					// be running on a non-desktop environment so the we server would be stuck waiting.
-					// Falling back to reading from STDIN is safer here.
-					//
-					// See https://jira.mesosphere.com/browse/DCOS_OSS-5591
-					loginServer.Close()
-				} else if loginServer != nil {
-					token = <-loginServer.Token()
-
-					// When the token comes from the local webserver there is no need to retry. In case of an
-					// error, retrying isn't needed (it can't be a typo, no copy-pasting was involved).
-					// Thus we raise the attempt counter to 3.
-					attempt = 3
-				}
+				f.initBrowserFlow(provider)
 			}
 
-			if token == "" {
+			if f.loginServer != nil {
+				token = <-f.loginServer.Token()
+				f.loginServer.Close()
+
+				// When the token comes from the local webserver there is no need to retry. In case of an
+				// error, retrying isn't needed (it can't be a typo, no copy-pasting was involved).
+				// Thus we raise the attempt counter to 3.
+				attempt = 3
+			} else {
 				token = f.prompt.Input("Enter token from the browser: ")
 			}
 
@@ -236,6 +211,43 @@ func (f *Flow) triggerMethod(provider *Provider) (acsToken string, err error) {
 	}
 }
 
+// initBrowserFlow initiates a browser based flow.
+//
+// It opens the browser and starts a local login server when applicable.
+func (f *Flow) initBrowserFlow(provider *Provider) {
+	startFlowURL := provider.Config.StartFlowURL
+
+	// For the "dcos-oidc-auth0" login provider ID, we start a local web server
+	// in order to avoid copy-pasting the token from the browser to the terminal.
+	if provider.ID == DCOSOIDCAuth0 {
+		var err error
+		f.loginServer, err = loginserver.New(startFlowURL)
+		if err != nil {
+			f.logger.Error(err)
+		} else {
+			startFlowURL = f.loginServer.StartFlowURL()
+			go f.loginServer.Start()
+		}
+	}
+
+	err := f.openBrowser(startFlowURL)
+	if err != nil {
+		// Being unable to open the browser is not critical, a message was prompted
+		// to the user with the URL they should go to in order to start the login flow.
+		f.logger.Info(err)
+
+		// However we close and don't use the web server in this scenario, as the CLI might
+		// be running on a non-desktop environment so the we server would be stuck waiting.
+		// Falling back to reading from STDIN is safer here.
+		//
+		// See https://jira.mesosphere.com/browse/DCOS_OSS-5591
+		if f.loginServer != nil {
+			f.loginServer.Close()
+			f.loginServer = nil
+		}
+	}
+}
+
 // uid returns the UID from the flag or prompts the user for it.
 func (f *Flow) uid() string {
 	if f.flags.username != "" {

diff --git a/pkg/login/flow_test.go b/pkg/login/flow_test.go
@@ -97,34 +97,49 @@ func TestTriggerMethodOIDCWithLoginServer(t *testing.T) {
 
 func TestTriggerMethodOIDCWithoutLoginServer(t *testing.T) {
 
-	// We're simulating a container environment where the browser can't be opened.
-	// See https://jira.mesosphere.com/browse/DCOS_OSS-5591
-	opener := open.OpenerFunc(func(_ string) error {
-		return errors.New("couldn't open browser, I'm just a container")
-	})
+	fixtures := []struct {
+		provider *Provider
+	}{
+		{
+			// We're simulating a container environment where the browser can't be opened.
+			// See https://jira.mesosphere.com/browse/DCOS_OSS-5591
+			defaultOIDCImplicitFlowProvider(),
+		},
+		{
+			// Non-regression test for a panic on browser login flows other than auth0.
+			// https://jira.mesosphere.com/browse/DCOS-60349
+			shibbolethLoginProvider(),
+		},
+	}
 
-	expectedLoginToken := "dummy_login_token"
-	expectedACSToken := "dummy_acs_token"
+	for _, fixture := range fixtures {
+		expectedLoginToken := "dummy_login_token"
+		expectedACSToken := "dummy_acs_token"
 
-	ts := httptest.NewServer(mockLoginEndpoint(t, expectedLoginToken, expectedACSToken))
-	defer ts.Close()
+		ts := httptest.NewServer(mockLoginEndpoint(t, expectedLoginToken, expectedACSToken))
+		defer ts.Close()
 
-	logger := &logrus.Logger{Out: ioutil.Discard}
+		logger := &logrus.Logger{Out: ioutil.Discard}
 
-	var in bytes.Buffer
-	in.WriteString(expectedLoginToken)
+		var in bytes.Buffer
+		in.WriteString(expectedLoginToken)
 
-	flow := NewFlow(FlowOpts{
-		Prompt: prompt.New(&in, ioutil.Discard),
-		Logger: logger,
-		Opener: opener,
-	})
+		opener := open.OpenerFunc(func(_ string) error {
+			return errors.New("couldn't open browser, I'm just a container")
+		})
 
-	flow.client = NewClient(httpclient.New(ts.URL), logger)
+		flow := NewFlow(FlowOpts{
+			Prompt: prompt.New(&in, ioutil.Discard),
+			Logger: logger,
+			Opener: opener,
+		})
 
-	acsToken, err := flow.triggerMethod(defaultOIDCImplicitFlowProvider())
-	require.NoError(t, err)
-	require.Equal(t, expectedACSToken, acsToken)
+		flow.client = NewClient(httpclient.New(ts.URL), logger)
+
+		acsToken, err := flow.triggerMethod(fixture.provider)
+		require.NoError(t, err)
+		require.Equal(t, expectedACSToken, acsToken)
+	}
 }
 
 func mockLoginEndpoint(t *testing.T, expectedLoginToken, acsToken string) http.Handler {
@@ -148,3 +163,14 @@ func mockLoginEndpoint(t *testing.T, expectedLoginToken, acsToken string) http.H
 	})
 	return mux
 }
+
+func shibbolethLoginProvider() *Provider {
+	return &Provider{
+		ID:           "shib-integration-test",
+		Type:         OIDCImplicitFlow,
+		ClientMethod: methodBrowserOIDCToken,
+		Config: ProviderConfig{
+			StartFlowURL: "/login?redirect_uri=urn:ietf:wg:oauth:2.0:oob",
+		},
+	}
+}