From 399b15abeb2021539a4893ca062c4d0390868b54 Mon Sep 17 00:00:00 2001 From: Bobby Rullo Date: Tue, 19 Apr 2016 18:27:45 -0700 Subject: [PATCH] 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 | 40 +++++--- admin/api_test.go | 4 +- cmd/dex-overlord/main.go | 3 +- db/client.go | 21 ++++- integration/admin_api_test.go | 171 ++++++++++++++++++++++++++++------ schema/adminschema/mapper.go | 18 +++- 6 files changed, 203 insertions(+), 54 deletions(-) diff --git a/admin/api.go b/admin/api.go index 5a821a9d..965e9ce2 100644 --- a/admin/api.go +++ b/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 { diff --git a/admin/api_test.go b/admin/api_test.go index 165d8ff5..27214d0d 100644 --- a/admin/api_test.go +++ b/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 } diff --git a/cmd/dex-overlord/main.go b/cmd/dex-overlord/main.go index 8dad8aa0..b1a0aecb 100644 --- a/cmd/dex-overlord/main.go +++ b/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()) diff --git a/db/client.go b/db/client.go index 84496b4b..42dd5f8d 100644 --- a/db/client.go +++ b/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 } diff --git a/integration/admin_api_test.go b/integration/admin_api_test.go index 0714b72f..1348a24b 100644 --- a/integration/admin_api_test.go +++ b/integration/admin_api_test.go @@ -1,20 +1,23 @@ 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, }, { - client: oidc.ClientMetadata{ - RedirectURIs: []url.URL{ - {Scheme: "https", Host: "auth.example.com", Path: "/"}, + req: adminschema.ClientCreateRequest{ + Client: &adminschema.Client{ + IsAdmin: true, }, }, + wantError: http.StatusBadRequest, + }, + { + 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 { - f := makeAdminAPITestFixtures() - req := &adminschema.ClientCreateRequest{ - Client: &adminschema.Client{}, - } + if i != 3 { + continue + } + f := makeAdminAPITestFixtures() - for _, redirectURI := range tt.client.RedirectURIs { - req.Client.RedirectURIs = append(req.Client.RedirectURIs, redirectURI.String()) - } - resp, err := f.adClient.Client.Create(req).Do() - if err != nil { - if tt.wantError { - return nil - } - return err + 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 } - if resp.Client.Id == "" { - return errors.New("no client id returned") + + aErr, ok := err.(*googleapi.Error) + if !ok { + t.Errorf("case %d: could not assert as adminSchema.Error: %v", i, err) + continue } - if resp.Client.Secret == "" { - return errors.New("no client secret returned") + if aErr.Code != tt.wantError { + t.Errorf("case %d: want aErr.Code=%v, got %v", i, tt.wantError, aErr.Code) + continue } - return nil - }() + continue + } + if err != nil { - t.Errorf("case %d: %v", i, err) + t.Errorf("case %d: unexpected error creating client: %v", i, err) + continue + } + + if diff := pretty.Compare(tt.want, resp); diff != "" { + t.Errorf("case %d: Compare(want, got) = %v", i, diff) + } + + repoClient, err := f.cr.Get(resp.Client.Id) + if err != nil { + 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) } } } diff --git a/schema/adminschema/mapper.go b/schema/adminschema/mapper.go index 3f6c32d1..f360750a 100644 --- a/schema/adminschema/mapper.go +++ b/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 }