ClientID property is ignored when creating a new OAuth2 Client

[zikes]

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
When creating a new OAuth2 client using the client.Client struct, if an ID is specified it must currently be done via the ID property, while the ClientID property is ignored. This is counter to the documentation, which states that ID is an alias of ClientID, whereas the opposite appears to be true.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

SQLManager.CreateClient(&client.Client{
  ClientID: "my-client",
  Name: "My Client",
})

Results in a client with a generated UUID for a Client ID.

What is the expected behavior?
That ClientID be an accepted form of input for client IDs.

Which version of the software is affected?
v1.0.0-beta.6

Suggested Fix
Step 1: In github.com/ory/go-convenience/stringsx add a new convenience function called Coalesce (concept borrowed from SQL):

// Coalesce returns the first non-empty string value
func Coalesce(str ...string) string {
	for _, s := range str {
		if s != "" {
			return s
		}
	}
	return ""
}

Playground Link

Step 2: At

hydra/client/manager_sql.go

Line 205 in 694b483

ID: d.ID,

update the line to use the new Coalesce function:

ID: stringsx.Coalesce(d.ID, d.ClientID),

This would maintain compatibility with existing applications, prioritizing the existing behavior, while enabling the ClientID option.

[aeneasr]

Are you calling the SQLManager directly? The HTTP handler (aka REST API) calls a validator which in turn hydrates the client id based on both id and client_id.

If you're calling the SQLManager directly please be aware that those are internals and they are not intended to be exposed/consumed by outside code. The respective storage structs are "dumb" - ie don't care about validation as the only source of data should be the REST handler.

If this is in fact about the REST API then I'll investigate, otherwise this will be closed as wontfix.

[zikes]

Apologies, the error occurs while using the Swagger Go SDK. I mistakenly believed the SQLManager example would be a minimal case. A more appropriate example would be

sdk, _ := hydra.NewSDK(&hydra.Configuration{
  EndpointURL: hydraURL,
})
client := &swagger.OAuth2Client{
  ClientId: "my-client",
  Name: "My Client",
}
sdk.CreateOAuth2Client(client)

I have tested using Id rather than ClientId in the above scenario and that does work as expected.

[aeneasr]

Hm, first of all I really like the Coalesce solution and will replace this with the current way this is being solved. What's weird though is that I wrote a test case to test your code and was unable to get it to failing. The test case is:

		for k, tc := range []struct {
			client   hydra.OAuth2Client
			expectID string
		}{
			{
				client: hydra.OAuth2Client{},
			},
			{
				client:   hydra.OAuth2Client{Id: "set-properly-1"},
				expectID: "set-properly-1",
			},
			{
				client:   hydra.OAuth2Client{ClientId: "set-properly-2"},
				expectID: "set-properly-2",
			},
		} {
			t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
				result, response, err := c.CreateOAuth2Client(tc.client)
				require.NoError(t, err)
				require.EqualValues(t, http.StatusCreated, response.StatusCode, "%s", response.Payload)

				assert.NotEmpty(t, result.Id)
				assert.NotEmpty(t, result.ClientId)
				assert.EqualValues(t, result.Id, result.ClientId)

				id := result.Id
				if tc.expectID != "" {
					assert.EqualValues(t, tc.expectID, result.Id)
					assert.EqualValues(t, tc.expectID, result.ClientId)
					id = tc.expectID
				}

				result, response, err = c.GetOAuth2Client(id)
				require.NoError(t, err)
				require.EqualValues(t, http.StatusOK, response.StatusCode, "%s", response.Payload)

				assert.EqualValues(t, id, result.Id)
				assert.EqualValues(t, id, result.ClientId)
			})
		}

Did I miss something here? I'll update the code nonetheless because I really like the Coalesce solution.

[aeneasr]

See #927

[zikes]

That test case is using the client.MemoryManager, whereas the bug is in client.SQLManager.