Merge pull request #411 from bobbyrullo/cross_client

Use Client defined in dex instead of go-oidc for storing clients

admin/api.go

@@ -5,36 +5,36 @@ 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
-	userRepo           user.UserRepo
-	passwordInfoRepo   user.PasswordInfoRepo
-	clientIdentityRepo client.ClientIdentityRepo
-	localConnectorID   string
+	userManager      *manager.UserManager
+	userRepo         user.UserRepo
+	passwordInfoRepo user.PasswordInfoRepo
+	clientRepo       client.ClientRepo
+	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),
-		clientIdentityRepo: db.NewClientIdentityRepo(dbMap),
-		localConnectorID:   localConnectorID,
+		userManager:      userManager,
+		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),
 	}
 )
 
@@ -116,25 +126,38 @@ func (a *AdminAPI) GetState() (adminschema.State, error) {
 	return state, nil
 }
 
-type ClientRegistrationRequest struct {
-	IsAdmin bool                `json:"isAdmin"`
-	Client  oidc.ClientMetadata `json:"client"`
-}
+func (a *AdminAPI) CreateClient(req adminschema.ClientCreateRequest) (adminschema.ClientCreateResponse, error) {
+	if req.Client == nil {
+		return adminschema.ClientCreateResponse{}, ErrorMissingClient
+	}
 
-func (a *AdminAPI) CreateClient(req ClientRegistrationRequest) (oidc.ClientRegistrationResponse, error) {
-	if err := req.Client.Valid(); err != nil {
-		return oidc.ClientRegistrationResponse{}, mapError(err)
+	cli, err := adminschema.MapSchemaClientToClient(*req.Client)
+	if err != nil {
+		return adminschema.ClientCreateResponse{}, mapError(err)
+	}
+
+	if err := cli.Metadata.Valid(); err != nil {
+		return adminschema.ClientCreateResponse{}, ErrorInvalidClientFunc(err)
 	}
-	// metadata is guarenteed to have at least one redirect_uri by earlier validation.
-	id, err := oidc.GenClientID(req.Client.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 oidc.ClientRegistrationResponse{}, mapError(err)
+		return adminschema.ClientCreateResponse{}, mapError(err)
 	}
-	c, err := a.clientIdentityRepo.New(id, req.Client, req.IsAdmin)
+
+	cli.Credentials.ID = id
+
+	creds, err := a.clientRepo.New(cli)
 	if err != nil {
-		return oidc.ClientRegistrationResponse{}, mapError(err)
+		return adminschema.ClientCreateResponse{}, mapError(err)
 	}
-	return oidc.ClientRegistrationResponse{ClientID: c.ID, ClientSecret: c.Secret, ClientMetadata: req.Client}, nil
+
+	req.Client.Id = creds.ID
+	req.Client.Secret = creds.Secret
+	return adminschema.ClientCreateResponse{
+		Client: req.Client,
+	}, 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
 }

client/client.go

@@ -1,7 +1,9 @@
 package client
 
 import (
+	"encoding/json"
 	"errors"
+	"io"
 	"net/url"
 	"reflect"
 
@@ -15,7 +17,15 @@ var (
 	ErrorNotFound              = errors.New("no data found")
 )
 
-type ClientIdentityRepo interface {
+type Client struct {
+	Credentials oidc.ClientCredentials
+	Metadata    oidc.ClientMetadata
+	Admin       bool
+}
+
+type ClientRepo interface {
+	Get(clientID string) (Client, error)
+
 	// Metadata returns one matching ClientMetadata if the given client
 	// exists, otherwise nil. The returned error will be non-nil only
 	// if the repo was unable to determine client existence.
@@ -27,13 +37,13 @@ type ClientIdentityRepo interface {
 	// to make these assertions will a non-nil error be returned.
 	Authenticate(creds oidc.ClientCredentials) (bool, error)
 
-	// All returns all registered Client Identities.
-	All() ([]oidc.ClientIdentity, error)
+	// All returns all registered Clients
+	All() ([]Client, error)
 
-	// New registers a ClientIdentity with the repo for the given metadata.
+	// New registers a Client with the repo.
 	// An unused ID must be provided. A corresponding secret will be returned
 	// in a ClientCredentials struct along with the provided ID.
-	New(id string, meta oidc.ClientMetadata, admin bool) (*oidc.ClientCredentials, error)
+	New(client Client) (*oidc.ClientCredentials, error)
 
 	SetDexAdmin(clientID string, isAdmin bool) error
 
@@ -64,3 +74,42 @@ func ValidRedirectURL(rURL *url.URL, redirectURLs []url.URL) (url.URL, error) {
 	}
 	return url.URL{}, ErrorInvalidRedirectURL
 }
+
+func ClientsFromReader(r io.Reader) ([]Client, error) {
+	var c []struct {
+		ID           string   `json:"id"`
+		Secret       string   `json:"secret"`
+		RedirectURLs []string `json:"redirectURLs"`
+	}
+	if err := json.NewDecoder(r).Decode(&c); err != nil {
+		return nil, err
+	}
+	clients := make([]Client, len(c))
+	for i, client := range c {
+		if client.ID == "" {
+			return nil, errors.New("clients must have an ID")
+		}
+		if len(client.Secret) == 0 {
+			return nil, errors.New("clients must have a Secret")
+		}
+		redirectURIs := make([]url.URL, len(client.RedirectURLs))
+		for j, u := range client.RedirectURLs {
+			uri, err := url.Parse(u)
+			if err != nil {
+				return nil, err
+			}
+			redirectURIs[j] = *uri
+		}
+
+		clients[i] = Client{
+			Credentials: oidc.ClientCredentials{
+				ID:     client.ID,
+				Secret: client.Secret,
+			},
+			Metadata: oidc.ClientMetadata{
+				RedirectURIs: redirectURIs,
+			},
+		}
+	}
+	return clients, nil
+}

client/client_test.go

@@ -0,0 +1,149 @@
+package client
+
+import (
+	"encoding/base64"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/coreos/go-oidc/oidc"
+	"github.com/kylelemons/godebug/pretty"
+)
+
+var (
+	goodSecret1 = base64.URLEncoding.EncodeToString([]byte("my_secret"))
+	goodSecret2 = base64.URLEncoding.EncodeToString([]byte("my_other_secret"))
+
+	goodClient1 = `{ 
+  "id": "my_id",
+  "secret": "` + goodSecret1 + `",
+  "redirectURLs": ["https://client.example.com"]
+}`
+
+	goodClient2 = `{ 
+  "id": "my_other_id",
+  "secret": "` + goodSecret2 + `",
+  "redirectURLs": ["https://client2.example.com","https://client2_a.example.com"]
+}`
+
+	badURLClient = `{ 
+  "id": "my_id",
+  "secret": "` + goodSecret1 + `",
+  "redirectURLs": ["hdtp:/\(bad)(u)(r)(l)"]
+}`
+
+	badSecretClient = `{ 
+  "id": "my_id",
+  "secret": "` + "****" + `",
+  "redirectURLs": ["https://client.example.com"]
+}`
+
+	noSecretClient = `{ 
+  "id": "my_id",
+  "redirectURLs": ["https://client.example.com"]
+}`
+	noIDClient = `{ 
+  "secret": "` + goodSecret1 + `",
+  "redirectURLs": ["https://client.example.com"]
+}`
+)
+
+func TestClientsFromReader(t *testing.T) {
+	tests := []struct {
+		json    string
+		want    []Client
+		wantErr bool
+	}{
+		{
+			json: "[]",
+			want: []Client{},
+		},
+		{
+			json: "[" + goodClient1 + "]",
+			want: []Client{
+				{
+					Credentials: oidc.ClientCredentials{
+						ID:     "my_id",
+						Secret: "my_secret",
+					},
+					Metadata: oidc.ClientMetadata{
+						RedirectURIs: []url.URL{
+							mustParseURL(t, "https://client.example.com"),
+						},
+					},
+				},
+			},
+		},
+		{
+			json: "[" + strings.Join([]string{goodClient1, goodClient2}, ",") + "]",
+			want: []Client{
+				{
+					Credentials: oidc.ClientCredentials{
+						ID:     "my_id",
+						Secret: "my_secret",
+					},
+					Metadata: oidc.ClientMetadata{
+						RedirectURIs: []url.URL{
+							mustParseURL(t, "https://client.example.com"),
+						},
+					},
+				},
+				{
+					Credentials: oidc.ClientCredentials{
+						ID:     "my_other_id",
+						Secret: "my_other_secret",
+					},
+					Metadata: oidc.ClientMetadata{
+						RedirectURIs: []url.URL{
+							mustParseURL(t, "https://client2.example.com"),
+							mustParseURL(t, "https://client2_a.example.com"),
+						},
+					},
+				},
+			},
+		}, {
+			json:    "[" + badURLClient + "]",
+			wantErr: true,
+		},
+		{
+			json:    "[" + badSecretClient + "]",
+			wantErr: true,
+		},
+		{
+			json:    "[" + noSecretClient + "]",
+			wantErr: true,
+		},
+		{
+			json:    "[" + noIDClient + "]",
+			wantErr: true,
+		},
+	}
+
+	for i, tt := range tests {
+		r := strings.NewReader(tt.json)
+		cs, err := ClientsFromReader(r)
+		if tt.wantErr {
+			if err == nil {
+				t.Errorf("case %d: want non-nil err", i)
+				t.Logf(pretty.Sprint(cs))
+			}
+			continue
+		}
+		if err != nil {
+			t.Errorf("case %d: got unexpected error parsing clients: %v", i, err)
+			t.Logf(tt.json)
+		}
+
+		if diff := pretty.Compare(tt.want, cs); diff != "" {
+			t.Errorf("case %d: Compare(want, got): %v", i, diff)
+		}
+	}
+}
+
+func mustParseURL(t *testing.T, s string) url.URL {
+	u, err := url.Parse(s)
+	if err != nil {
+		t.Fatalf("Cannot parse %v as url: %v", s, err)
+	}
+	return *u
+}