Repository: markbates/goth
Pull request #286 (2019-08-11 21:16:22)

Fix provider deduction from existing sessions so that an argument is not needed on the authentication landing page.
This has been broken since https://github.com/markbates/goth/commit/78433fec1a8fed110af91e79c94be2d835afe637.

Also moving the deduction code to be a fallback: if a provider is explicitly
specified it should not be overridden by the existing session. I believe
this is why the Logout() problems with multiple providers existed in the
first place. If deduction wins over explicit parameters, then once you
wrongly click one provider, the cookie will force you to use that
authentication mechanism even if you've navigated to a URL that
explicitly states another provider's name.

Fixed the test harness to check session key names (this was a bug in the
test harness) and added a test to verify that this change works.


Patch:
diff --git a/gothic/gothic.go b/gothic/gothic.go
index bea87d9..ea4e1e0 100644
--- a/gothic/gothic.go
+++ b/gothic/gothic.go
@@ -245,19 +245,6 @@ var GetProviderName = getProviderName
 
 func getProviderName(req *http.Request) (string, error) {
 
-	// get all the used providers
-	providers := goth.GetProviders()
-
-	// loop over the used providers, if we already have a valid session for any provider (ie. user is already logged-in with a provider), then return that provider name
-	for _, provider := range providers {
-		p := provider.Name()
-		session, _ := Store.Get(req, p+SessionName)
-		value := session.Values[p]
-		if _, ok := value.(string); ok {
-			return p, nil
-		}
-	}
-
 	// try to get it from the url param "provider"
 	if p := req.URL.Query().Get("provider"); p != "" {
 		return p, nil
@@ -278,6 +265,17 @@ func getProviderName(req *http.Request) (string, error) {
 		return p, nil
 	}
 
+	// As a fallback, loop over the used providers, if we already have a valid session for any provider (ie. user has already begun authentication with a provider), then return that provider name
+	providers := goth.GetProviders()
+	session, _ := Store.Get(req, SessionName)
+	for _, provider := range providers {
+		p := provider.Name()
+		value := session.Values[p]
+		if _, ok := value.(string); ok {
+			return p, nil
+		}
+	}
+
 	// if not found then return an empty string with the corresponding error
 	return "", errors.New("you must select a provider")
 }


Test patch:
diff --git a/gothic/gothic_test.go b/gothic/gothic_test.go
index 49dd8a9..fecde6c 100644
--- a/gothic/gothic_test.go
+++ b/gothic/gothic_test.go
@@ -19,16 +19,21 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+type mapKey struct {
+	r *http.Request
+	n string
+}
+
 type ProviderStore struct {
-	Store map[*http.Request]*sessions.Session
+	Store map[mapKey]*sessions.Session
 }
 
 func NewProviderStore() *ProviderStore {
-	return &ProviderStore{map[*http.Request]*sessions.Session{}}
+	return &ProviderStore{map[mapKey]*sessions.Session{}}
 }
 
 func (p ProviderStore) Get(r *http.Request, name string) (*sessions.Session, error) {
-	s := p.Store[r]
+	s := p.Store[mapKey{r, name}]
 	if s == nil {
 		s, err := p.New(r, name)
 		return s, err
@@ -42,12 +47,12 @@ func (p ProviderStore) New(r *http.Request, name string) (*sessions.Session, err
 		Path:   "/",
 		MaxAge: 86400 * 30,
 	}
-	p.Store[r] = s
+	p.Store[mapKey{r, name}] = s
 	return s, nil
 }
 
 func (p ProviderStore) Save(r *http.Request, w http.ResponseWriter, s *sessions.Session) error {
-	p.Store[r] = s
+	p.Store[mapKey{r, s.Name()}] = s
 	return nil
 }
 
@@ -68,7 +73,7 @@ func Test_BeginAuthHandler(t *testing.T) {
 
 	BeginAuthHandler(res, req)
 
-	sess, err := Store.Get(req, "faux"+SessionName)
+	sess, err := Store.Get(req, SessionName)
 	if err != nil {
 		t.Fatalf("error getting faux Gothic session: %v", err)
 	}
@@ -128,7 +133,28 @@ func Test_CompleteUserAuth(t *testing.T) {
 	a.NoError(err)
 
 	sess := faux.Session{Name: "Homer Simpson", Email: "homer@example.com"}
-	session, _ := Store.Get(req, "faux"+SessionName)
+	session, _ := Store.Get(req, SessionName)
+	session.Values["faux"] = gzipString(sess.Marshal())
+	err = session.Save(req, res)
+	a.NoError(err)
+
+	user, err := CompleteUserAuth(res, req)
+	a.NoError(err)
+
+	a.Equal(user.Name, "Homer Simpson")
+	a.Equal(user.Email, "homer@example.com")
+}
+
+func Test_CompleteUserAuthWithSessionDeducedProvider(t *testing.T) {
+	a := assert.New(t)
+
+	res := httptest.NewRecorder()
+	// Inteintionally omit a provider argument, force looking in session.
+	req, err := http.NewRequest("GET", "/auth/callback", nil)
+	a.NoError(err)
+
+	sess := faux.Session{Name: "Homer Simpson", Email: "homer@example.com"}
+	session, _ := Store.Get(req, SessionName)
 	session.Values["faux"] = gzipString(sess.Marshal())
 	err = session.Save(req, res)
 	a.NoError(err)
@@ -148,7 +174,7 @@ func Test_Logout(t *testing.T) {
 	a.NoError(err)
 
 	sess := faux.Session{Name: "Homer Simpson", Email: "homer@example.com"}
-	session, _ := Store.Get(req, "faux"+SessionName)
+	session, _ := Store.Get(req, SessionName)
 	session.Values["faux"] = gzipString(sess.Marshal())
 	err = session.Save(req, res)
 	a.NoError(err)
@@ -160,7 +186,7 @@ func Test_Logout(t *testing.T) {
 	a.Equal(user.Email, "homer@example.com")
 	err = Logout(res, req)
 	a.NoError(err)
-	session, _ = Store.Get(req, "faux"+SessionName)
+	session, _ = Store.Get(req, SessionName)
 	a.Equal(session.Values, make(map[interface{}]interface{}))
 	a.Equal(session.Options.MaxAge, -1)
 }
@@ -188,7 +214,7 @@ func Test_StateValidation(t *testing.T) {
 	a.NoError(err)
 
 	BeginAuthHandler(res, req)
-	session, _ := Store.Get(req, "faux"+SessionName)
+	session, _ := Store.Get(req, SessionName)
 
 	// Assert that matching states will return a nil error
 	req, err = http.NewRequest("GET", "/auth/callback?provider=faux&state=state_REAL", nil)