integration, *: Improve tests for admin api

* TestCreateClient was missing test coverage on error cases
* Fixed bug where 500s were being reported for bad requests
* changed function signature of NewAdminAPI back to old way of passing
  in lots of repos: passing in a DbMap made it difficult to test
* added swappable ID and Secret generators when creating Clients

admin/api.go

@@ -5,15 +5,17 @@ import (
 	"net/http"
 
 	"github.com/coreos/go-oidc/oidc"
-	"github.com/go-gorp/gorp"
 
 	"github.com/coreos/dex/client"
-	"github.com/coreos/dex/db"
 	"github.com/coreos/dex/schema/adminschema"
 	"github.com/coreos/dex/user"
 	"github.com/coreos/dex/user/manager"
 )
 
+var (
+	ClientIDGenerator = oidc.GenClientID
+)
+
 // AdminAPI provides the logic necessary to implement the Admin API.
 type AdminAPI struct {
 	userManager      *manager.UserManager
@@ -23,17 +25,15 @@ type AdminAPI struct {
 	localConnectorID string
 }
 
-// TODO(ericchiang): Swap the DbMap for a storage interface. See #278
-
-func NewAdminAPI(dbMap *gorp.DbMap, userManager *manager.UserManager, localConnectorID string) *AdminAPI {
+func NewAdminAPI(userRepo user.UserRepo, pwiRepo user.PasswordInfoRepo, clientRepo client.ClientRepo, userManager *manager.UserManager, localConnectorID string) *AdminAPI {
 	if localConnectorID == "" {
 		panic("must specify non-blank localConnectorID")
 	}
 	return &AdminAPI{
 		userManager:      userManager,
-		userRepo:         db.NewUserRepo(dbMap),
-		passwordInfoRepo: db.NewPasswordInfoRepo(dbMap),
-		clientRepo:       db.NewClientRepo(dbMap),
+		userRepo:         userRepo,
+		passwordInfoRepo: pwiRepo,
+		clientRepo:       clientRepo,
 		localConnectorID: localConnectorID,
 	}
 }
@@ -67,10 +67,20 @@ func errorMaker(typ string, desc string, code int) func(internal error) Error {
 }
 
 var (
+	ErrorMissingClient = errorMaker("bad_request", "The 'client' cannot be empty", http.StatusBadRequest)(nil)
+
+	// Called when oidc.ClientMetadata.Valid() fails.
+	ErrorInvalidClientFunc = errorMaker("bad_request", "Your client could not be validated.", http.StatusBadRequest)
+
 	errorMap = map[error]func(error) Error{
 		user.ErrorNotFound:       errorMaker("resource_not_found", "Resource could not be found.", http.StatusNotFound),
 		user.ErrorDuplicateEmail: errorMaker("bad_request", "Email already in use.", http.StatusBadRequest),
 		user.ErrorInvalidEmail:   errorMaker("bad_request", "invalid email.", http.StatusBadRequest),
+
+		adminschema.ErrorInvalidRedirectURI: errorMaker("bad_request", "invalid redirectURI.", http.StatusBadRequest),
+		adminschema.ErrorInvalidLogoURI:     errorMaker("bad_request", "invalid logoURI.", http.StatusBadRequest),
+		adminschema.ErrorInvalidClientURI:   errorMaker("bad_request", "invalid clientURI.", http.StatusBadRequest),
+		adminschema.ErrorNoRedirectURI:      errorMaker("bad_request", "invalid redirectURI.", http.StatusBadRequest),
 	}
 )
 
@@ -117,19 +127,21 @@ func (a *AdminAPI) GetState() (adminschema.State, error) {
 }
 
 func (a *AdminAPI) CreateClient(req adminschema.ClientCreateRequest) (adminschema.ClientCreateResponse, error) {
+	if req.Client == nil {
+		return adminschema.ClientCreateResponse{}, ErrorMissingClient
+	}
+
 	cli, err := adminschema.MapSchemaClientToClient(*req.Client)
 	if err != nil {
-		// TODO  should be 400s
 		return adminschema.ClientCreateResponse{}, mapError(err)
 	}
 
 	if err := cli.Metadata.Valid(); err != nil {
-		// TODO make sure this is not 500
-		return adminschema.ClientCreateResponse{}, mapError(err)
+		return adminschema.ClientCreateResponse{}, ErrorInvalidClientFunc(err)
 	}
 
-	// metadata is guarenteed to have at least one redirect_uri by earlier validation.
-	id, err := oidc.GenClientID(cli.Metadata.RedirectURIs[0].Host)
+	// metadata is guaranteed to have at least one redirect_uri by earlier validation.
+	id, err := ClientIDGenerator(cli.Metadata.RedirectURIs[0].Host)
 	if err != nil {
 		return adminschema.ClientCreateResponse{}, mapError(err)
 	}
@@ -146,8 +158,6 @@ func (a *AdminAPI) CreateClient(req adminschema.ClientCreateRequest) (adminschem
 	return adminschema.ClientCreateResponse{
 		Client: req.Client,
 	}, nil
-
-	//	github.com/coreos/dex/integrationoidc.ClientRegistrationResponse{ClientID: c.ID, ClientSecret: c.Secret, ClientMetadata: req.Client.Metadata}, nil
 }
 
 func mapError(e error) error {

admin/api_test.go

@@ -3,6 +3,7 @@ package admin
 import (
 	"testing"
 
+	"github.com/coreos/dex/client"
 	"github.com/coreos/dex/connector"
 	"github.com/coreos/dex/db"
 	"github.com/coreos/dex/schema/adminschema"
@@ -15,6 +16,7 @@ import (
 type testFixtures struct {
 	ur    user.UserRepo
 	pwr   user.PasswordInfoRepo
+	cr    client.ClientRepo
 	mgr   *manager.UserManager
 	adAPI *AdminAPI
 }
@@ -69,7 +71,7 @@ func makeTestFixtures() *testFixtures {
 	}()
 
 	f.mgr = manager.NewUserManager(f.ur, f.pwr, ccr, db.TransactionFactory(dbMap), manager.ManagerOptions{})
-	f.adAPI = NewAdminAPI(dbMap, f.mgr, "local")
+	f.adAPI = NewAdminAPI(f.ur, f.pwr, f.cr, f.mgr, "local")
 
 	return f
 }

cmd/dex-overlord/main.go

@@ -116,10 +116,11 @@ func main() {
 	userRepo := db.NewUserRepo(dbc)
 	pwiRepo := db.NewPasswordInfoRepo(dbc)
 	connCfgRepo := db.NewConnectorConfigRepo(dbc)
+	clientRepo := db.NewClientRepo(dbc)
 	userManager := manager.NewUserManager(userRepo,
 		pwiRepo, connCfgRepo, db.TransactionFactory(dbc), manager.ManagerOptions{})
 
-	adminAPI := admin.NewAdminAPI(dbc, userManager, *localConnectorID)
+	adminAPI := admin.NewAdminAPI(userRepo, pwiRepo, clientRepo, userManager, *localConnectorID)
 	kRepo, err := db.NewPrivateKeySetRepo(dbc, *useOldFormat, keySecrets.BytesSlice()...)
 	if err != nil {
 		log.Fatalf(err.Error())

db/client.go

@@ -92,10 +92,20 @@ func (m *clientModel) Client() (*client.Client, error) {
 
 func NewClientRepo(dbm *gorp.DbMap) client.ClientRepo {
 	return newClientRepo(dbm)
+
+}
+
+func NewClientRepoWithSecretGenerator(dbm *gorp.DbMap, secGen SecretGenerator) client.ClientRepo {
+	rep := newClientRepo(dbm)
+	rep.secretGenerator = secGen
+	return rep
 }
 
 func newClientRepo(dbm *gorp.DbMap) *clientRepo {
-	return &clientRepo{db: &db{dbm}}
+	return &clientRepo{
+		db:              &db{dbm},
+		secretGenerator: DefaultSecretGenerator,
+	}
 }
 
 func NewClientRepoFromClients(dbm *gorp.DbMap, clients []client.Client) (client.ClientRepo, error) {
@@ -127,6 +137,7 @@ func NewClientRepoFromClients(dbm *gorp.DbMap, clients []client.Client) (client.
 
 type clientRepo struct {
 	*db
+	secretGenerator SecretGenerator
 }
 
 func (r *clientRepo) Get(clientID string) (client.Client, error) {
@@ -249,8 +260,14 @@ func isAlreadyExistsErr(err error) bool {
 	return false
 }
 
+type SecretGenerator func() ([]byte, error)
+
+func DefaultSecretGenerator() ([]byte, error) {
+	return pcrypto.RandBytes(maxSecretLength)
+}
+
 func (r *clientRepo) New(cli client.Client) (*oidc.ClientCredentials, error) {
-	secret, err := pcrypto.RandBytes(maxSecretLength)
+	secret, err := r.secretGenerator()
 	if err != nil {
 		return nil, err
 	}

integration/admin_api_test.go

 package integration
 
 import (
-	"errors"
+	"encoding/base64"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"testing"
 
+	"github.com/coreos/go-oidc/oidc"
 	"github.com/kylelemons/godebug/pretty"
 	"google.golang.org/api/googleapi"
 
 	"github.com/coreos/dex/admin"
+	"github.com/coreos/dex/client"
+	"github.com/coreos/dex/db"
 	"github.com/coreos/dex/schema/adminschema"
 	"github.com/coreos/dex/server"
 	"github.com/coreos/dex/user"
-	"github.com/coreos/go-oidc/oidc"
 )
 
 const (
@@ -24,6 +27,7 @@ const (
 type adminAPITestFixtures struct {
 	ur       user.UserRepo
 	pwr      user.PasswordInfoRepo
+	cr       client.ClientRepo
 	adAPI    *admin.AdminAPI
 	adSrv    *server.AdminServer
 	hSrv     *httptest.Server
@@ -78,9 +82,17 @@ func makeAdminAPITestFixtures() *adminAPITestFixtures {
 	f := &adminAPITestFixtures{}
 
 	dbMap, ur, pwr, um := makeUserObjects(adminUsers, adminPasswords)
+
+	var cliCount int
+	secGen := func() ([]byte, error) {
+		return []byte(fmt.Sprintf("client_%v", cliCount)), nil
+	}
+	cr := db.NewClientRepoWithSecretGenerator(dbMap, secGen)
+
+	f.cr = cr
 	f.ur = ur
 	f.pwr = pwr
-	f.adAPI = admin.NewAdminAPI(dbMap, um, "local")
+	f.adAPI = admin.NewAdminAPI(ur, pwr, cr, um, "local")
 	f.adSrv = server.NewAdminServer(f.adAPI, nil, adminAPITestSecret)
 	f.hSrv = httptest.NewServer(f.adSrv.HTTPHandler())
 	f.hc = &http.Client{
@@ -256,50 +268,147 @@ func TestCreateAdmin(t *testing.T) {
 }
 
 func TestCreateClient(t *testing.T) {
+	oldGen := admin.ClientIDGenerator
+	admin.ClientIDGenerator = func(hostport string) (string, error) {
+		return fmt.Sprintf("client_%v", hostport), nil
+	}
+	defer func() {
+		admin.ClientIDGenerator = oldGen
+	}()
+
+	mustParseURL := func(s string) *url.URL {
+		u, err := url.Parse(s)
+		if err != nil {
+			t.Fatalf("couldn't parse URL: %v", err)
+		}
+		return u
+	}
+	addIDAndSecret := func(cli adminschema.Client) *adminschema.Client {
+		cli.Id = "client_auth.example.com"
+		cli.Secret = base64.URLEncoding.EncodeToString([]byte("client_0"))
+		return &cli
+	}
+
+	adminClientGood := adminschema.Client{
+		RedirectURIs: []string{"https://auth.example.com/"},
+	}
+	clientGood := client.Client{
+		Credentials: oidc.ClientCredentials{
+			ID: "client_auth.example.com",
+		},
+		Metadata: oidc.ClientMetadata{
+			RedirectURIs: []url.URL{*mustParseURL("https://auth.example.com/")},
+		},
+	}
+
+	adminAdminClient := adminClientGood
+	adminAdminClient.IsAdmin = true
+	clientGoodAdmin := clientGood
+	clientGoodAdmin.Admin = true
+
+	adminMultiRedirect := adminClientGood
+	adminMultiRedirect.RedirectURIs = []string{"https://auth.example.com/", "https://auth2.example.com/"}
+	clientMultiRedirect := clientGoodAdmin
+	clientMultiRedirect.Metadata.RedirectURIs = append(
+		clientMultiRedirect.Metadata.RedirectURIs,
+		*mustParseURL("https://auth2.example.com/"))
+
 	tests := []struct {
-		client    oidc.ClientMetadata
-		wantError bool
+		req        adminschema.ClientCreateRequest
+		want       adminschema.ClientCreateResponse
+		wantClient client.Client
+		wantError  int
 	}{
 		{
-			client:    oidc.ClientMetadata{},
-			wantError: true,
+			req:       adminschema.ClientCreateRequest{},
+			wantError: http.StatusBadRequest,
+		},
+		{
+			req: adminschema.ClientCreateRequest{
+				Client: &adminschema.Client{
+					IsAdmin: true,
+				},
+			},
+			wantError: http.StatusBadRequest,
 		},
 		{
-			client: oidc.ClientMetadata{
-				RedirectURIs: []url.URL{
-					{Scheme: "https", Host: "auth.example.com", Path: "/"},
+			req: adminschema.ClientCreateRequest{
+				Client: &adminschema.Client{
+					RedirectURIs: []string{"909090"},
 				},
 			},
+			wantError: http.StatusBadRequest,
+		},
+		{
+			req: adminschema.ClientCreateRequest{
+				Client: &adminClientGood,
+			},
+			want: adminschema.ClientCreateResponse{
+				Client: addIDAndSecret(adminClientGood),
+			},
+			wantClient: clientGood,
+		},
+		{
+			req: adminschema.ClientCreateRequest{
+				Client: &adminAdminClient,
+			},
+			want: adminschema.ClientCreateResponse{
+				Client: addIDAndSecret(adminAdminClient),
+			},
+			wantClient: clientGoodAdmin,
+		},
+		{
+			req: adminschema.ClientCreateRequest{
+				Client: &adminMultiRedirect,
+			},
+			want: adminschema.ClientCreateResponse{
+				Client: addIDAndSecret(adminMultiRedirect),
+			},
+			wantClient: clientMultiRedirect,
 		},
 	}
 
 	for i, tt := range tests {
-		err := func() error {
+		if i != 3 {
+			continue
+		}
 		f := makeAdminAPITestFixtures()
-			req := &adminschema.ClientCreateRequest{
-				Client: &adminschema.Client{},
+
+		resp, err := f.adClient.Client.Create(&tt.req).Do()
+		if tt.wantError != 0 {
+			if err == nil {
+				t.Errorf("case %d: want non-nil error.", i)
+				continue
 			}
 
-			for _, redirectURI := range tt.client.RedirectURIs {
-				req.Client.RedirectURIs = append(req.Client.RedirectURIs, redirectURI.String())
+			aErr, ok := err.(*googleapi.Error)
+			if !ok {
+				t.Errorf("case %d: could not assert as adminSchema.Error: %v", i, err)
+				continue
 			}
-			resp, err := f.adClient.Client.Create(req).Do()
-			if err != nil {
-				if tt.wantError {
-					return nil
+			if aErr.Code != tt.wantError {
+				t.Errorf("case %d: want aErr.Code=%v, got %v", i, tt.wantError, aErr.Code)
+				continue
 			}
-				return err
+			continue
 		}
-			if resp.Client.Id == "" {
-				return errors.New("no client id returned")
+
+		if err != nil {
+			t.Errorf("case %d: unexpected error creating client: %v", i, err)
+			continue
 		}
-			if resp.Client.Secret == "" {
-				return errors.New("no client secret returned")
+
+		if diff := pretty.Compare(tt.want, resp); diff != "" {
+			t.Errorf("case %d: Compare(want, got) = %v", i, diff)
 		}
-			return nil
-		}()
+
+		repoClient, err := f.cr.Get(resp.Client.Id)
 		if err != nil {
-			t.Errorf("case %d: %v", i, err)
+			t.Errorf("case %d: Unexpected error getting client: %v", i, err)
+		}
+
+		if diff := pretty.Compare(tt.wantClient, repoClient); diff != "" {
+			t.Errorf("case %d: Compare(wantClient, repoClient) = %v", i, diff)
 		}
 	}
 }

schema/adminschema/mapper.go

@@ -8,6 +8,13 @@ import (
 	"github.com/coreos/go-oidc/oidc"
 )
 
+var (
+	ErrorNoRedirectURI      = errors.New("No Redirect URIs")
+	ErrorInvalidRedirectURI = errors.New("Invalid Redirect URI")
+	ErrorInvalidLogoURI     = errors.New("Invalid Logo URI")
+	ErrorInvalidClientURI   = errors.New("Invalid Client URI")
+)
+
 func MapSchemaClientToClient(sc Client) (client.Client, error) {
 	c := client.Client{
 		Credentials: oidc.ClientCredentials{
@@ -20,12 +27,12 @@ func MapSchemaClientToClient(sc Client) (client.Client, error) {
 	}
 	for i, ru := range sc.RedirectURIs {
 		if ru == "" {
-			return client.Client{}, errors.New("redirect URL empty")
+			return client.Client{}, ErrorNoRedirectURI
 		}
 
 		u, err := url.Parse(ru)
 		if err != nil {
-			return client.Client{}, errors.New("redirect URL invalid")
+			return client.Client{}, ErrorInvalidRedirectURI
 		}
 
 		c.Metadata.RedirectURIs[i] = *u
@@ -36,7 +43,7 @@ func MapSchemaClientToClient(sc Client) (client.Client, error) {
 	if sc.LogoURI != "" {
 		logoURI, err := url.Parse(sc.LogoURI)
 		if err != nil {
-			return client.Client{}, errors.New("logoURI invalid")
+			return client.Client{}, ErrorInvalidLogoURI
 		}
 		c.Metadata.LogoURI = logoURI
 	}
@@ -44,10 +51,12 @@ func MapSchemaClientToClient(sc Client) (client.Client, error) {
 	if sc.ClientURI != "" {
 		clientURI, err := url.Parse(sc.ClientURI)
 		if err != nil {
-			return client.Client{}, errors.New("clientURI invalid")
+			return client.Client{}, ErrorInvalidClientURI
 		}
 		c.Metadata.ClientURI = clientURI
 	}
+
+	c.Admin = sc.IsAdmin
 	return c, nil
 }
 
@@ -69,5 +78,6 @@ func MapClientToSchemaClient(c client.Client) Client {
 	if c.Metadata.ClientURI != nil {
 		cl.ClientURI = c.Metadata.ClientURI.String()
 	}
+	cl.IsAdmin = c.Admin
 	return cl
 }