keystone: fetching groups only if requested, refactoring.

.travis.yml

@@ -13,14 +13,18 @@ services:
   - docker
 
 env:
-  - DEX_POSTGRES_DATABASE=postgres DEX_POSTGRES_USER=postgres DEX_POSTGRES_HOST="localhost" DEX_ETCD_ENDPOINTS=http://localhost:2379 DEX_LDAP_TESTS=1 DEBIAN_FRONTEND=noninteractive DEX_KEYSTONE_URL=http://localhost:5000 DEX_KEYSTONE_ADMIN_URL=http://localhost:35357
+  - DEX_POSTGRES_DATABASE=postgres DEX_POSTGRES_USER=postgres DEX_POSTGRES_HOST="localhost" DEX_ETCD_ENDPOINTS=http://localhost:2379 DEX_LDAP_TESTS=1 DEBIAN_FRONTEND=noninteractive DEX_KEYSTONE_URL=http://localhost:5000 DEX_KEYSTONE_ADMIN_URL=http://localhost:35357 DEX_KEYSTONE_ADMIN_USER=demo DEX_KEYSTONE_ADMIN_PASS=DEMO_PASS
 
 install:
   - sudo -E apt-get install -y --force-yes slapd time ldap-utils
   - sudo /etc/init.d/slapd stop
   - docker run -d --net=host gcr.io/etcd-development/etcd:v3.2.9
-  - docker run -d -p 0.0.0.0:5000:5000 -p 0.0.0.0:35357:35357 openio/openstack-keystone
-  - sleep 60s
+  - docker run -d -p 0.0.0.0:5000:5000 -p 0.0.0.0:35357:35357 openio/openstack-keystone:pike
+  - |
+    until curl --fail http://localhost:5000/v3; do
+      echo 'Waiting for keystone...'
+      sleep 1;
+    done;
 
 script:
   - make testall

connector/keystone/keystone.go

@@ -14,65 +14,148 @@ import (
 	"github.com/dexidp/dex/connector"
 )
 
+type conn struct {
+	Domain        string
+	Host          string
+	AdminUsername string
+	AdminPassword string
+	Logger        logrus.FieldLogger
+}
+
+type userKeystone struct {
+	Domain domainKeystone `json:"domain"`
+	ID     string         `json:"id"`
+	Name   string         `json:"name"`
+}
+
+type domainKeystone struct {
+	ID   string `json:"id"`
+	Name string `json:"name"`
+}
+
+// Config holds the configuration parameters for Keystone connector.
+// Keystone should expose API v3
+// An example config:
+//	connectors:
+//		type: keystone
+//		id: keystone
+//		name: Keystone
+//		config:
+//			keystoneHost: http://example:5000
+//			domain: default
+//      keystoneUsername: demo
+//      keystonePassword: DEMO_PASS
+type Config struct {
+	Domain        string `json:"domain"`
+	Host          string `json:"keystoneHost"`
+	AdminUsername string `json:"keystoneUsername"`
+	AdminPassword string `json:"keystonePassword"`
+}
+
+type loginRequestData struct {
+	auth `json:"auth"`
+}
+
+type auth struct {
+	Identity identity `json:"identity"`
+}
+
+type identity struct {
+	Methods  []string `json:"methods"`
+	Password password `json:"password"`
+}
+
+type password struct {
+	User user `json:"user"`
+}
+
+type user struct {
+	Name     string `json:"name"`
+	Domain   domain `json:"domain"`
+	Password string `json:"password"`
+}
+
+type domain struct {
+	ID string `json:"id"`
+}
+
+type token struct {
+	User userKeystone `json:"user"`
+}
+
+type tokenResponse struct {
+	Token token `json:"token"`
+}
+
+type group struct {
+	ID   string `json:"id"`
+	Name string `json:"name"`
+}
+
+type groupsResponse struct {
+	Groups []group `json:"groups"`
+}
+
 var (
-	_ connector.PasswordConnector = &keystoneConnector{}
-	_ connector.RefreshConnector  = &keystoneConnector{}
+	_ connector.PasswordConnector = &conn{}
+	_ connector.RefreshConnector  = &conn{}
 )
 
 // Open returns an authentication strategy using Keystone.
 func (c *Config) Open(id string, logger logrus.FieldLogger) (connector.Connector, error) {
-	return &keystoneConnector{c.Domain, c.KeystoneHost,
-		c.KeystoneUsername, c.KeystonePassword, logger}, nil
+	return &conn{
+		c.Domain,
+		c.Host,
+		c.AdminUsername,
+		c.AdminPassword,
+		logger}, nil
 }
 
-func (p *keystoneConnector) Close() error { return nil }
+func (p *conn) Close() error { return nil }
 
-func (p *keystoneConnector) Login(ctx context.Context, s connector.Scopes, username, password string) (
-	identity connector.Identity, validPassword bool, err error) {
+func (p *conn) Login(ctx context.Context, scopes connector.Scopes, username, password string) (identity connector.Identity, validPassword bool, err error) {
 	resp, err := p.getTokenResponse(ctx, username, password)
 	if err != nil {
 		return identity, false, fmt.Errorf("keystone: error %v", err)
 	}
-
-	// Providing wrong password or wrong keystone URI throws error
-	if resp.StatusCode == 201 {
-		token := resp.Header.Get("X-Subject-Token")
-		data, err := ioutil.ReadAll(resp.Body)
-		if err != nil {
-			return identity, false, err
-		}
-		defer resp.Body.Close()
-
-		var tokenResp = new(tokenResponse)
-		err = json.Unmarshal(data, &tokenResp)
-		if err != nil {
-			return identity, false, fmt.Errorf("keystone: invalid token response: %v", err)
-		}
+	if resp.StatusCode/100 != 2 {
+		return identity, false, fmt.Errorf("keystone login: error %v", resp.StatusCode)
+	}
+	if resp.StatusCode != 201 {
+		return identity, false, nil
+	}
+	token := resp.Header.Get("X-Subject-Token")
+	data, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return identity, false, err
+	}
+	defer resp.Body.Close()
+	var tokenResp = new(tokenResponse)
+	err = json.Unmarshal(data, &tokenResp)
+	if err != nil {
+		return identity, false, fmt.Errorf("keystone: invalid token response: %v", err)
+	}
+	if scopes.Groups {
 		groups, err := p.getUserGroups(ctx, tokenResp.Token.User.ID, token)
 		if err != nil {
 			return identity, false, err
 		}
-
-		identity.Username = username
-		identity.UserID = tokenResp.Token.User.ID
 		identity.Groups = groups
-		return identity, true, nil
-
 	}
-
-	return identity, false, nil
+	identity.Username = username
+	identity.UserID = tokenResp.Token.User.ID
+	return identity, true, nil
 }
 
-func (p *keystoneConnector) Prompt() string { return "username" }
+func (p *conn) Prompt() string { return "username" }
 
-func (p *keystoneConnector) Refresh(
-	ctx context.Context, s connector.Scopes, identity connector.Identity) (connector.Identity, error) {
+func (p *conn) Refresh(
+	ctx context.Context, scopes connector.Scopes, identity connector.Identity) (connector.Identity, error) {
 
 	token, err := p.getAdminToken(ctx)
 	if err != nil {
 		return identity, fmt.Errorf("keystone: failed to obtain admin token: %v", err)
 	}
-
 	ok, err := p.checkIfUserExists(ctx, identity.UserID, token)
 	if err != nil {
 		return identity, err
@@ -80,17 +163,17 @@ func (p *keystoneConnector) Refresh(
 	if !ok {
 		return identity, fmt.Errorf("keystone: user %q does not exist", identity.UserID)
 	}
-
-	groups, err := p.getUserGroups(ctx, identity.UserID, token)
-	if err != nil {
-		return identity, err
+	if scopes.Groups {
+		groups, err := p.getUserGroups(ctx, identity.UserID, token)
+		if err != nil {
+			return identity, err
+		}
+		identity.Groups = groups
 	}
-
-	identity.Groups = groups
 	return identity, nil
 }
 
-func (p *keystoneConnector) getTokenResponse(ctx context.Context, username, pass string) (response *http.Response, err error) {
+func (p *conn) getTokenResponse(ctx context.Context, username, pass string) (response *http.Response, err error) {
 	client := &http.Client{}
 	jsonData := loginRequestData{
 		auth: auth{
@@ -110,8 +193,8 @@ func (p *keystoneConnector) getTokenResponse(ctx context.Context, username, pass
 	if err != nil {
 		return nil, err
 	}
-
-	authTokenURL := p.KeystoneHost + "/v3/auth/tokens/"
+	// https://developer.openstack.org/api-ref/identity/v3/#password-authentication-with-unscoped-authorization
+	authTokenURL := p.Host + "/v3/auth/tokens/"
 	req, err := http.NewRequest("POST", authTokenURL, bytes.NewBuffer(jsonValue))
 	if err != nil {
 		return nil, err
@@ -123,8 +206,8 @@ func (p *keystoneConnector) getTokenResponse(ctx context.Context, username, pass
 	return client.Do(req)
 }
 
-func (p *keystoneConnector) getAdminToken(ctx context.Context) (string, error) {
-	resp, err := p.getTokenResponse(ctx, p.KeystoneUsername, p.KeystonePassword)
+func (p *conn) getAdminToken(ctx context.Context) (string, error) {
+	resp, err := p.getTokenResponse(ctx, p.AdminUsername, p.AdminPassword)
 	if err != nil {
 		return "", err
 	}
@@ -132,8 +215,9 @@ func (p *keystoneConnector) getAdminToken(ctx context.Context) (string, error) {
 	return token, nil
 }
 
-func (p *keystoneConnector) checkIfUserExists(ctx context.Context, userID string, token string) (bool, error) {
-	userURL := p.KeystoneHost + "/v3/users/" + userID
+func (p *conn) checkIfUserExists(ctx context.Context, userID string, token string) (bool, error) {
+	// https://developer.openstack.org/api-ref/identity/v3/#show-user-details
+	userURL := p.Host + "/v3/users/" + userID
 	client := &http.Client{}
 	req, err := http.NewRequest("GET", userURL, nil)
 	if err != nil {
@@ -153,10 +237,10 @@ func (p *keystoneConnector) checkIfUserExists(ctx context.Context, userID string
 	return false, err
 }
 
-func (p *keystoneConnector) getUserGroups(ctx context.Context, userID string, token string) ([]string, error) {
+func (p *conn) getUserGroups(ctx context.Context, userID string, token string) ([]string, error) {
 	client := &http.Client{}
-	groupsURL := p.KeystoneHost + "/v3/users/" + userID + "/groups"
-
+	// https://developer.openstack.org/api-ref/identity/v3/#list-groups-to-which-a-user-belongs
+	groupsURL := p.Host + "/v3/users/" + userID + "/groups"
 	req, err := http.NewRequest("GET", groupsURL, nil)
 	req.Header.Set("X-Auth-Token", token)
 	req = req.WithContext(ctx)

connector/keystone/keystone_test.go

@@ -16,8 +16,6 @@ import (
 )
 
 const (
-	adminUser   = "demo"
-	adminPass   = "DEMO_PASS"
 	invalidPass = "WRONG_PASS"
 
 	testUser   = "test_user"
@@ -30,6 +28,8 @@ const (
 var (
 	keystoneURL      = ""
 	keystoneAdminURL = ""
+	adminUser        = ""
+	adminPass        = ""
 	authTokenURL     = ""
 	usersURL         = ""
 	groupsURL        = ""
@@ -213,24 +213,31 @@ func addUserToGroup(t *testing.T, token, groupID, userID string) error {
 }
 
 func TestIncorrectCredentialsLogin(t *testing.T) {
-	c := keystoneConnector{KeystoneHost: keystoneURL, Domain: testDomain,
-		KeystoneUsername: adminUser, KeystonePassword: adminPass}
+	setupVariables(t)
+	c := conn{Host: keystoneURL, Domain: testDomain,
+		AdminUsername: adminUser, AdminPassword: adminPass}
 	s := connector.Scopes{OfflineAccess: true, Groups: true}
 	_, validPW, err := c.Login(context.Background(), s, adminUser, invalidPass)
-	if err != nil {
-		t.Fatal(err.Error())
-	}
 
 	if validPW {
-		t.Fail()
+		t.Fatal("Incorrect password check")
+	}
+
+	if err == nil {
+		t.Fatal("Error should be returned when invalid password is provided")
+	}
+
+	if !strings.Contains(err.Error(), "401") {
+		t.Fatal("Unrecognized error, expecting 401")
 	}
 }
 
 func TestValidUserLogin(t *testing.T) {
+	setupVariables(t)
 	token, _ := getAdminToken(t, adminUser, adminPass)
 	userID := createUser(t, token, testUser, testEmail, testPass)
-	c := keystoneConnector{KeystoneHost: keystoneURL, Domain: testDomain,
-		KeystoneUsername: adminUser, KeystonePassword: adminPass}
+	c := conn{Host: keystoneURL, Domain: testDomain,
+		AdminUsername: adminUser, AdminPassword: adminPass}
 	s := connector.Scopes{OfflineAccess: true, Groups: true}
 	identity, validPW, err := c.Login(context.Background(), s, testUser, testPass)
 	if err != nil {
@@ -239,18 +246,19 @@ func TestValidUserLogin(t *testing.T) {
 	t.Log(identity)
 
 	if !validPW {
-		t.Fail()
+		t.Fatal("Valid password was not accepted")
 	}
 	delete(t, token, userID, usersURL)
 }
 
 func TestUseRefreshToken(t *testing.T) {
+	setupVariables(t)
 	token, adminID := getAdminToken(t, adminUser, adminPass)
 	groupID := createGroup(t, token, "Test group description", testGroup)
 	addUserToGroup(t, token, groupID, adminID)
 
-	c := keystoneConnector{KeystoneHost: keystoneURL, Domain: testDomain,
-		KeystoneUsername: adminUser, KeystonePassword: adminPass}
+	c := conn{Host: keystoneURL, Domain: testDomain,
+		AdminUsername: adminUser, AdminPassword: adminPass}
 	s := connector.Scopes{OfflineAccess: true, Groups: true}
 
 	identityLogin, _, err := c.Login(context.Background(), s, adminUser, adminPass)
@@ -270,11 +278,12 @@ func TestUseRefreshToken(t *testing.T) {
 }
 
 func TestUseRefreshTokenUserDeleted(t *testing.T) {
+	setupVariables(t)
 	token, _ := getAdminToken(t, adminUser, adminPass)
 	userID := createUser(t, token, testUser, testEmail, testPass)
 
-	c := keystoneConnector{KeystoneHost: keystoneURL, Domain: testDomain,
-		KeystoneUsername: adminUser, KeystonePassword: adminPass}
+	c := conn{Host: keystoneURL, Domain: testDomain,
+		AdminUsername: adminUser, AdminPassword: adminPass}
 	s := connector.Scopes{OfflineAccess: true, Groups: true}
 
 	identityLogin, _, err := c.Login(context.Background(), s, testUser, testPass)
@@ -296,11 +305,12 @@ func TestUseRefreshTokenUserDeleted(t *testing.T) {
 }
 
 func TestUseRefreshTokenGroupsChanged(t *testing.T) {
+	setupVariables(t)
 	token, _ := getAdminToken(t, adminUser, adminPass)
 	userID := createUser(t, token, testUser, testEmail, testPass)
 
-	c := keystoneConnector{KeystoneHost: keystoneURL, Domain: testDomain,
-		KeystoneUsername: adminUser, KeystonePassword: adminPass}
+	c := conn{Host: keystoneURL, Domain: testDomain,
+		AdminUsername: adminUser, AdminPassword: adminPass}
 	s := connector.Scopes{OfflineAccess: true, Groups: true}
 
 	identityLogin, _, err := c.Login(context.Background(), s, testUser, testPass)
@@ -315,7 +325,7 @@ func TestUseRefreshTokenGroupsChanged(t *testing.T) {
 
 	expectEquals(t, 0, len(identityRefresh.Groups))
 
-	groupID := createGroup(t, token, "Test group description", testGroup)
+	groupID := createGroup(t, token, "Test group", testGroup)
 	addUserToGroup(t, token, groupID, userID)
 
 	identityRefresh, err = c.Refresh(context.Background(), s, identityLogin)
@@ -329,26 +339,62 @@ func TestUseRefreshTokenGroupsChanged(t *testing.T) {
 	expectEquals(t, 1, len(identityRefresh.Groups))
 }
 
-func TestMain(m *testing.M) {
+func TestNoGroupsInScope(t *testing.T) {
+	setupVariables(t)
+	token, _ := getAdminToken(t, adminUser, adminPass)
+	userID := createUser(t, token, testUser, testEmail, testPass)
+
+	c := conn{Host: keystoneURL, Domain: testDomain,
+		AdminUsername: adminUser, AdminPassword: adminPass}
+	s := connector.Scopes{OfflineAccess: true, Groups: false}
+
+	groupID := createGroup(t, token, "Test group", testGroup)
+	addUserToGroup(t, token, groupID, userID)
+
+	identityLogin, _, err := c.Login(context.Background(), s, testUser, testPass)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+	expectEquals(t, 0, len(identityLogin.Groups))
+
+	identityRefresh, err := c.Refresh(context.Background(), s, identityLogin)
+	if err != nil {
+		t.Fatal(err.Error())
+	}
+	expectEquals(t, 0, len(identityRefresh.Groups))
+
+	delete(t, token, groupID, groupsURL)
+	delete(t, token, userID, usersURL)
+}
+
+func setupVariables(t *testing.T) {
 	keystoneURLEnv := "DEX_KEYSTONE_URL"
 	keystoneAdminURLEnv := "DEX_KEYSTONE_ADMIN_URL"
+	keystoneAdminUserEnv := "DEX_KEYSTONE_ADMIN_USER"
+	keystoneAdminPassEnv := "DEX_KEYSTONE_ADMIN_PASS"
 	keystoneURL = os.Getenv(keystoneURLEnv)
 	if keystoneURL == "" {
-		fmt.Printf("variable %q not set, skipping keystone connector tests\n", keystoneURLEnv)
+		t.Skip(fmt.Sprintf("variable %q not set, skipping keystone connector tests\n", keystoneURLEnv))
 		return
 	}
-	keystoneAdminURL := os.Getenv(keystoneAdminURLEnv)
+	keystoneAdminURL = os.Getenv(keystoneAdminURLEnv)
 	if keystoneAdminURL == "" {
-		fmt.Printf("variable %q not set, skipping keystone connector tests\n", keystoneAdminURLEnv)
+		t.Skip(fmt.Sprintf("variable %q not set, skipping keystone connector tests\n", keystoneAdminURLEnv))
+		return
+	}
+	adminUser = os.Getenv(keystoneAdminUserEnv)
+	if adminUser == "" {
+		t.Skip(fmt.Sprintf("variable %q not set, skipping keystone connector tests\n", keystoneAdminUserEnv))
+		return
+	}
+	adminPass = os.Getenv(keystoneAdminPassEnv)
+	if adminPass == "" {
+		t.Skip(fmt.Sprintf("variable %q not set, skipping keystone connector tests\n", keystoneAdminPassEnv))
 		return
 	}
 	authTokenURL = keystoneURL + "/v3/auth/tokens/"
-	fmt.Printf("Auth token url %q\n", authTokenURL)
-	fmt.Printf("Keystone URL %q\n", keystoneURL)
 	usersURL = keystoneAdminURL + "/v3/users/"
 	groupsURL = keystoneAdminURL + "/v3/groups/"
-	// run all tests
-	m.Run()
 }
 
 func expectEquals(t *testing.T, a interface{}, b interface{}) {

connector/keystone/types.go

-package keystone
-
-import (
-	"github.com/sirupsen/logrus"
-)
-
-type keystoneConnector struct {
-	Domain           string
-	KeystoneHost     string
-	KeystoneUsername string
-	KeystonePassword string
-	Logger           logrus.FieldLogger
-}
-
-type userKeystone struct {
-	Domain domainKeystone `json:"domain"`
-	ID     string         `json:"id"`
-	Name   string         `json:"name"`
-}
-
-type domainKeystone struct {
-	ID   string `json:"id"`
-	Name string `json:"name"`
-}
-
-// Config holds the configuration parameters for Keystone connector.
-// Keystone should expose API v3
-// An example config:
-//	connectors:
-//		type: keystone
-//		id: keystone
-//		name: Keystone
-//		config:
-//			keystoneHost: http://example:5000
-//			domain: default
-//      keystoneUsername: demo
-//      keystonePassword: DEMO_PASS
-type Config struct {
-	Domain           string `json:"domain"`
-	KeystoneHost     string `json:"keystoneHost"`
-	KeystoneUsername string `json:"keystoneUsername"`
-	KeystonePassword string `json:"keystonePassword"`
-}
-
-type loginRequestData struct {
-	auth `json:"auth"`
-}
-
-type auth struct {
-	Identity identity `json:"identity"`
-}
-
-type identity struct {
-	Methods  []string `json:"methods"`
-	Password password `json:"password"`
-}
-
-type password struct {
-	User user `json:"user"`
-}
-
-type user struct {
-	Name     string `json:"name"`
-	Domain   domain `json:"domain"`
-	Password string `json:"password"`
-}
-
-type domain struct {
-	ID string `json:"id"`
-}
-
-type token struct {
-	User userKeystone `json:"user"`
-}
-
-type tokenResponse struct {
-	Token token `json:"token"`
-}
-
-type group struct {
-	ID   string `json:"id"`
-	Name string `json:"name"`
-}
-
-type groupsResponse struct {
-	Groups []group `json:"groups"`
-}