From f899cbaea88449216693754afff6938040e7eecf Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Tue, 28 Jun 2016 16:09:20 -0700 Subject: [PATCH] return 409 for duplicate client ids --- admin/api.go | 1 + client/client.go | 2 ++ db/client.go | 2 +- db/conn_sqlite3.go | 2 +- integration/admin_api_test.go | 34 +++++++++++++++++++++++++++++++++- 5 files changed, 38 insertions(+), 3 deletions(-) diff --git a/admin/api.go b/admin/api.go index 324f55e9..6039e238 100644 --- a/admin/api.go +++ b/admin/api.go @@ -79,6 +79,7 @@ var ( client.ErrorPublicClientMissingName: errorMaker("bad_request", "Public clients require a ClientName", http.StatusBadRequest), client.ErrorInvalidClientSecret: errorMaker("bad_request", "Secret must be a base64 encoded string", http.StatusBadRequest), + client.ErrorDuplicateClientID: errorMaker("bad_request", "Client ID already exists.", http.StatusConflict), user.ErrorNotFound: errorMaker("resource_not_found", "Resource could not be found.", http.StatusNotFound), user.ErrorDuplicateEmail: errorMaker("bad_request", "Email already in use.", http.StatusConflict), diff --git a/client/client.go b/client/client.go index 4105dba0..0d6700cf 100644 --- a/client/client.go +++ b/client/client.go @@ -20,6 +20,8 @@ var ( ErrorInvalidClientSecret = errors.New("not a valid client Secret") + ErrorDuplicateClientID = errors.New("client ID already exists") + ErrorInvalidRedirectURL = errors.New("not a valid redirect url for the given client") ErrorCantChooseRedirectURL = errors.New("must provide a redirect url; client has many") ErrorNoValidRedirectURLs = errors.New("no valid redirect URLs for this client.") diff --git a/db/client.go b/db/client.go index e1a65292..e1575042 100644 --- a/db/client.go +++ b/db/client.go @@ -196,7 +196,7 @@ func (r *clientRepo) New(tx repo.Transaction, cli client.Client) (*oidc.ClientCr if err := r.executor(tx).Insert(cim); err != nil { if isAlreadyExistsErr(err) { - err = errors.New("client ID already exists") + return nil, client.ErrorDuplicateClientID } return nil, err } diff --git a/db/conn_sqlite3.go b/db/conn_sqlite3.go index 5c2d332b..72992585 100644 --- a/db/conn_sqlite3.go +++ b/db/conn_sqlite3.go @@ -8,7 +8,7 @@ import "github.com/mattn/go-sqlite3" func init() { registerAlreadyExistsChecker(func(err error) bool { - sqlErr, ok := err.(*sqlite3.Error) + sqlErr, ok := err.(sqlite3.Error) if !ok { return false } diff --git a/integration/admin_api_test.go b/integration/admin_api_test.go index 6d483617..26fd3b8d 100644 --- a/integration/admin_api_test.go +++ b/integration/admin_api_test.go @@ -68,6 +68,22 @@ var ( Password: []byte("hi."), }, } + + clients = []client.Client{ + { + Credentials: oidc.ClientCredentials{ + ID: "client-1", + Secret: "Zm9vYmFy", // "foobar" + }, + Metadata: oidc.ClientMetadata{ + RedirectURIs: []url.URL{ + url.URL{Scheme: "http", Host: "127.0.0.1:5556", Path: "/cb"}, + url.URL{Scheme: "https", Host: "example.com", Path: "/callback"}, + }, + }, + Admin: true, + }, + } ) type adminAPITransport struct { @@ -94,6 +110,12 @@ func makeAdminAPITestFixtures() *adminAPITestFixtures { clientIDGenerator := func(hostport string) (string, error) { return fmt.Sprintf("client_%v", hostport), nil } + for _, client := range clients { + _, err := cr.New(nil, client) + if err != nil { + panic(err) + } + } cm := manager.NewClientManager(cr, db.TransactionFactory(dbMap), manager.ManagerOptions{SecretGenerator: secGen, ClientIDGenerator: clientIDGenerator}) ccr := db.NewConnectorConfigRepo(dbMap) @@ -563,6 +585,16 @@ func TestCreateClient(t *testing.T) { Client: &adminClientBadSecret, }, wantError: http.StatusBadRequest, + }, { + // Client ID already exists + req: adminschema.ClientCreateRequest{ + Client: &adminschema.Client{ + Id: "client-1", + Secret: "Zm9vYmFy", + RedirectURIs: []string{"https://auth.example.com/"}, + }, + }, + wantError: http.StatusConflict, }, } @@ -597,7 +629,7 @@ func TestCreateClient(t *testing.T) { continue } if aErr.Code != tt.wantError { - t.Errorf("case %d: want aErr.Code=%v, got %v", i, tt.wantError, aErr.Code) + t.Errorf("case %d: want aErr.Code=%v, got %v: %v", i, tt.wantError, aErr.Code, aErr) continue } continue