Repository: markbates/goth
Pull request #294 (2019-10-09 01:08:35)

Feature - Slack Provider - Progressive Profile Fetching
# Scope

This PR addresses some usability issues with the current Slack provider, some security concerns, some HTTP client response closing issues, code formatting and linting issues, and greatly improves unit-test code coverage **for the Slack provider only**.

This was developed in a TDD fashion and the code has already been tested in an integration via our downstream fork.

### Most of this PR (in terms of lines of code) is in fact tests.
Don't let the size scare you. 😅

## Motivations

While working on a project that uses the Goth library, and more specifically the Slack provider, I ran into an issue where this library was essentially _forcing_ the use of [the `users:read` scope](https://api.slack.com/scopes/users:read), due to the way that it fetches the user's information at [the `users.info` endpoint](https://api.slack.com/methods/users.info).

While the Slack provider allows for a custom set of "scopes" to be defined via the last parameter of `slack.New`, the provider's `FetchUser` method **always** hits [the `users.info` endpoint](https://api.slack.com/methods/users.info) that **requires** [the `users:read` scope](https://api.slack.com/scopes/users:read). This unfortunately causes a "silent error" of sorts, as the response code is never even checked and the data won't be returned without that scope in place.

Ideally, and as this PR implements, the `FetchUser` method would return the "basic" profile (identity) of the user that it can get with [the `auth.test` endpoint](https://api.slack.com/methods/auth.test) that it must hit anyway, and then only attempt to grab the additional information if the required scope is present.

## Specifics

 - Adds comments and fixing formatting, to appease golint
 - Defines the `users:read` Slack provider scope as a constant
 - Greatly increases test coverage for the Slack provider
 - Refactors `providers/slack.Provider.FetchUser` to use the initially fetched data as a "basic" profile and only fetch the other, more "expansive", user-data if the proper scope has been included in the provider
     - This improves the Slack provider to only perform an operation when its able to (has the scope) and allows for users of the provider to choose whether they want to use the `users:read` scope, which is a scope that Slack considers to be advanced enough to "warn" about in Slack apps.
     - Also, this enables the Slack provider to be used with the newer "Sign in with Slack" flow and potentially ask for more scopes/permission "progressively".
 - Adds a missing HTTP client response body closure and removing redundant and unnecessary closures
 - Adds proper handling and testing for bad Slack user-info HTTP responses


Patch:
diff --git a/providers/slack/slack.go b/providers/slack/slack.go
index 3f0079a..9083637 100644
--- a/providers/slack/slack.go
+++ b/providers/slack/slack.go
@@ -11,10 +11,17 @@ import (
 	"net/url"
 
 	"fmt"
+
 	"github.com/markbates/goth"
 	"golang.org/x/oauth2"
 )
 
+// Scopes
+const (
+	ScopeUserRead string = "users:read"
+)
+
+// URLs and endpoints
 const (
 	authURL         string = "https://slack.com/oauth/authorize"
 	tokenURL        string = "https://slack.com/api/oauth.access"
@@ -56,6 +63,7 @@ func (p *Provider) SetName(name string) {
 	p.providerName = name
 }
 
+// Client returns the http.Client used in the provider.
 func (p *Provider) Client() *http.Client {
 	return goth.HTTPClientWithFallBack(p.HTTPClient)
 }
@@ -88,11 +96,9 @@ func (p *Provider) FetchUser(session goth.Session) (goth.User, error) {
 	// Get the userID, slack needs userID in order to get user profile info
 	response, err := p.Client().Get(endpointUser + "?token=" + url.QueryEscape(sess.AccessToken))
 	if err != nil {
-		if response != nil {
-			response.Body.Close()
-		}
 		return user, err
 	}
+	defer response.Body.Close()
 
 	if response.StatusCode != http.StatusOK {
 		return user, fmt.Errorf("%s responded with a %d trying to fetch user information", p.providerName, response.StatusCode)
@@ -103,36 +109,54 @@ func (p *Provider) FetchUser(session goth.Session) (goth.User, error) {
 		return user, err
 	}
 
-	u := struct {
-		UserID string `json:"user_id"`
-	}{}
-
-	err = json.NewDecoder(bytes.NewReader(bits)).Decode(&u)
-
-	// Get user profile info
-	response, err = p.Client().Get(endpointProfile + "?token=" + url.QueryEscape(sess.AccessToken) + "&user=" + u.UserID)
+	err = json.NewDecoder(bytes.NewReader(bits)).Decode(&user.RawData)
 	if err != nil {
-		if response != nil {
-			response.Body.Close()
-		}
 		return user, err
 	}
-	defer response.Body.Close()
 
-	bits, err = ioutil.ReadAll(response.Body)
-	if err != nil {
-		return user, err
-	}
+	err = simpleUserFromReader(bytes.NewReader(bits), &user)
 
-	err = json.NewDecoder(bytes.NewReader(bits)).Decode(&user.RawData)
-	if err != nil {
-		return user, err
+	if p.hasScope(ScopeUserRead) {
+		// Get user profile info
+		response, err = p.Client().Get(endpointProfile + "?token=" + url.QueryEscape(sess.AccessToken) + "&user=" + user.UserID)
+		if err != nil {
+			return user, err
+		}
+		defer response.Body.Close()
+
+		if response.StatusCode != http.StatusOK {
+			return user, fmt.Errorf("%s responded with a %d trying to fetch user information", p.providerName, response.StatusCode)
+		}
+
+		bits, err = ioutil.ReadAll(response.Body)
+		if err != nil {
+			return user, err
+		}
+
+		err = json.NewDecoder(bytes.NewReader(bits)).Decode(&user.RawData)
+		if err != nil {
+			return user, err
+		}
+
+		err = userFromReader(bytes.NewReader(bits), &user)
 	}
 
-	err = userFromReader(bytes.NewReader(bits), &user)
 	return user, err
 }
 
+func (p *Provider) hasScope(scope string) bool {
+	hasScope := false
+
+	for i := range p.config.Scopes {
+		if p.config.Scopes[i] == scope {
+			hasScope = true
+			break
+		}
+	}
+
+	return hasScope
+}
+
 func newConfig(provider *Provider, scopes []string) *oauth2.Config {
 	c := &oauth2.Config{
 		ClientID:     provider.ClientKey,
@@ -150,11 +174,28 @@ func newConfig(provider *Provider, scopes []string) *oauth2.Config {
 			c.Scopes = append(c.Scopes, scope)
 		}
 	} else {
-		c.Scopes = append(c.Scopes, "users:read")
+		c.Scopes = append(c.Scopes, ScopeUserRead)
 	}
 	return c
 }
 
+func simpleUserFromReader(r io.Reader, user *goth.User) error {
+	u := struct {
+		UserID string `json:"user_id"`
+		Name   string `json:"user"`
+	}{}
+
+	err := json.NewDecoder(r).Decode(&u)
+	if err != nil {
+		return err
+	}
+
+	user.UserID = u.UserID
+	user.NickName = u.Name
+
+	return nil
+}
+
 func userFromReader(r io.Reader, user *goth.User) error {
 	u := struct {
 		User struct {
@@ -165,7 +206,7 @@ func userFromReader(r io.Reader, user *goth.User) error {
 				Name      string `json:"real_name"`
 				AvatarURL string `json:"image_32"`
 				FirstName string `json:"first_name"`
-				LastName string `json:"last_name"`
+				LastName  string `json:"last_name"`
 			} `json:"profile"`
 		} `json:"user"`
 	}{}


Test patch:
diff --git a/providers/slack/slack_test.go b/providers/slack/slack_test.go
index eda5c75..dd6d595 100644
--- a/providers/slack/slack_test.go
+++ b/providers/slack/slack_test.go
@@ -1,11 +1,39 @@
 package slack_test
 
 import (
+	"context"
+	"crypto/tls"
+	"encoding/json"
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"testing"
+
 	"github.com/markbates/goth"
 	"github.com/markbates/goth/providers/slack"
 	"github.com/stretchr/testify/assert"
-	"os"
-	"testing"
+)
+
+var (
+	testAuthTestResponseData = map[string]interface{}{
+		"user":    "testuser",
+		"user_id": "user1234",
+	}
+
+	testUserInfoResponseData = map[string]interface{}{
+		"user": map[string]interface{}{
+			"id":   testAuthTestResponseData["user_id"],
+			"name": testAuthTestResponseData["user"],
+			"profile": map[string]interface{}{
+				"real_name":  "Test User",
+				"first_name": "Test",
+				"last_name":  "User",
+				"image_32":   "http://example.org/avatar.png",
+				"email":      "test@example.org",
+			},
+		},
+	}
 )
 
 func Test_New(t *testing.T) {
@@ -34,6 +62,142 @@ func Test_BeginAuth(t *testing.T) {
 	a.Contains(s.AuthURL, "slack.com/oauth/authorize")
 }
 
+func Test_FetchUser(t *testing.T) {
+	t.Parallel()
+
+	for _, testData := range []struct {
+		name         string
+		provider     *slack.Provider
+		session      goth.Session
+		handler      http.Handler
+		expectedUser goth.User
+		expectErr    bool
+	}{
+		{
+			name:     "FetchesFullProfile",
+			provider: provider(),
+			session:  &slack.Session{AccessToken: "TOKEN"},
+			handler: http.HandlerFunc(
+				func(res http.ResponseWriter, req *http.Request) {
+					switch req.URL.Path {
+					case "/api/auth.test":
+						res.WriteHeader(http.StatusOK)
+						json.NewEncoder(res).Encode(testAuthTestResponseData)
+					case "/api/users.info":
+						res.WriteHeader(http.StatusOK)
+						json.NewEncoder(res).Encode(testUserInfoResponseData)
+					default:
+						res.WriteHeader(http.StatusNotFound)
+					}
+				},
+			),
+			expectedUser: goth.User{
+				UserID:      "user1234",
+				NickName:    "testuser",
+				Name:        "Test User",
+				FirstName:   "Test",
+				LastName:    "User",
+				AvatarURL:   "http://example.org/avatar.png",
+				Email:       "test@example.org",
+				AccessToken: "TOKEN",
+			},
+			expectErr: false,
+		},
+		{
+			name:     "FetchesBasicProfileWhenLackingUserReadScope",
+			provider: slack.New(os.Getenv("SLACK_KEY"), os.Getenv("SLACK_SECRET"), "/foo", "commands"),
+			session:  &slack.Session{AccessToken: "TOKEN"},
+			handler: http.HandlerFunc(
+				func(res http.ResponseWriter, req *http.Request) {
+					switch req.URL.Path {
+					case "/api/auth.test":
+						res.WriteHeader(http.StatusOK)
+						json.NewEncoder(res).Encode(testAuthTestResponseData)
+					default:
+						res.WriteHeader(http.StatusNotFound)
+					}
+				},
+			),
+			expectedUser: goth.User{
+				UserID:      "user1234",
+				NickName:    "testuser",
+				AccessToken: "TOKEN",
+			},
+			expectErr: false,
+		},
+		{
+			name:      "FailsWithNoAccessToken",
+			provider:  provider(),
+			session:   &slack.Session{AccessToken: ""},
+			handler:   nil,
+			expectErr: true,
+		},
+		{
+			name:     "FailsWithBadAuthTestResponse",
+			provider: provider(),
+			session:  &slack.Session{AccessToken: "TOKEN"},
+			handler: http.HandlerFunc(
+				func(res http.ResponseWriter, req *http.Request) {
+					switch req.URL.Path {
+					case "/api/auth.test":
+						res.WriteHeader(http.StatusForbidden)
+					}
+				},
+			),
+			expectedUser: goth.User{
+				AccessToken: "TOKEN",
+			},
+			expectErr: true,
+		},
+		{
+			name:     "FailsWithBadUserInfoResponse",
+			provider: provider(),
+			session:  &slack.Session{AccessToken: "TOKEN"},
+			handler: http.HandlerFunc(
+				func(res http.ResponseWriter, req *http.Request) {
+					switch req.URL.Path {
+					case "/api/auth.test":
+						res.WriteHeader(http.StatusOK)
+						json.NewEncoder(res).Encode(testAuthTestResponseData)
+					case "/api/users.info":
+						res.WriteHeader(http.StatusForbidden)
+					}
+				},
+			),
+			expectedUser: goth.User{
+				UserID:      "user1234",
+				NickName:    "testuser",
+				AccessToken: "TOKEN",
+			},
+			expectErr: true,
+		},
+	} {
+		t.Run(testData.name, func(t *testing.T) {
+			a := assert.New(t)
+
+			withMockServer(testData.provider, testData.handler, func(p *slack.Provider) {
+				user, err := p.FetchUser(testData.session)
+				a.NotZero(user)
+
+				if testData.expectErr {
+					a.Error(err)
+				} else {
+					a.NoError(err)
+				}
+
+				a.Equal(testData.expectedUser.UserID, user.UserID)
+				a.Equal(testData.expectedUser.NickName, user.NickName)
+				a.Equal(testData.expectedUser.Name, user.Name)
+				a.Equal(testData.expectedUser.FirstName, user.FirstName)
+				a.Equal(testData.expectedUser.LastName, user.LastName)
+				a.Equal(testData.expectedUser.AvatarURL, user.AvatarURL)
+				a.Equal(testData.expectedUser.Email, user.Email)
+				a.Equal(testData.expectedUser.AccessToken, user.AccessToken)
+			})
+		})
+	}
+}
+
 func Test_SessionFromJSON(t *testing.T) {
 	t.Parallel()
 	a := assert.New(t)
@@ -50,3 +214,23 @@ func Test_SessionFromJSON(t *testing.T) {
 func provider() *slack.Provider {
 	return slack.New(os.Getenv("SLACK_KEY"), os.Getenv("SLACK_SECRET"), "/foo")
 }
+
+func withMockServer(p *slack.Provider, handler http.Handler, fn func(p *slack.Provider)) {
+	server := httptest.NewTLSServer(handler)
+	defer server.Close()
+
+	httpClient := &http.Client{
+		Transport: &http.Transport{
+			DialContext: func(ctx context.Context, network, _ string) (net.Conn, error) {
+				return net.Dial(network, server.Listener.Addr().String())
+			},
+			TLSClientConfig: &tls.Config{
+				InsecureSkipVerify: true,
+			},
+		},
+	}
+
+	p.HTTPClient = httpClient
+
+	fn(p)
+}