Merge pull request #496 from ericchiang/return-409-for-duplicate-client-ids

return 409 for duplicate client ids

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),

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.")

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
 	}

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
 		}

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