client.GetClients() returns invalid information

[boyvinall]

Looks like client.GetClients() is returning a map with all map values set to the same value. The actual value can vary each time you call it.

e.g.

Got 4 clients:
client [0c21e5d8-9340-4f35-a356-eed0da7d7192] = [Dev Proxy][0c21e5d8-9340-4f35-a356-eed0da7d7192]
client [85e7beb7-42b4-4aec-8ade-a865c28fb859] = [Dev Proxy][0c21e5d8-9340-4f35-a356-eed0da7d7192]
client [e0b7bd4d-acef-491a-a3da-f9e35c4118c0] = [Dev Proxy][0c21e5d8-9340-4f35-a356-eed0da7d7192]
client [06573bd3-4908-4621-af56-83efc675c992] = [Dev Proxy][0c21e5d8-9340-4f35-a356-eed0da7d7192]

where "Dev Proxy" is one of my clients.

Looks like the problem is https://github.com/ory-am/hydra/blob/9b51600/client/manager_rethinkdb.go#L84, although the implementation is the same in the memory storage too.

I think the best thing is actually to change the signature of GetClients, from:

func (m *MemoryManager) GetClients() (clients map[string]*Client, err error)

to

func (m *MemoryManager) GetClients() (clients map[string]Client, err error)

i.e. not returning a pointer. This saves on garbage collection slightly.

Happy to submit a PR if you agree this signature change @arekkas.

[aeneasr]

Nice catch, this could also be an issue in all implementations of the following interfaces:

• https://github.com/ory-am/hydra/blob/9b516002d04c3fd84ec9fac214f937f891d53829/client/manager.go#L20
• https://github.com/ory-am/hydra/blob/9b516002d04c3fd84ec9fac214f937f891d53829/connection/manager.go#L14
• https://github.com/ory-am/ladon/blob/master/manager.go#L16 (because Policies is a slice of Policy which is in turn an interface)

[aeneasr]

I think ladon is safe from this issue (both memory manager and rethinkdb manager) as they aren't taking an address of a variable in a loop. Would still be good to check that with tests though. I'll look into it.

[aeneasr]

Switching to (clients map[string]Client, err error) is a good idea IMHO. I will do the same in the connection managers

[boyvinall]

I've tested this change with my use-case, but not yet added anything to the regressions tests, sorry.

[aeneasr]

solved by #151