Fix missing LoginChallenge and LoginSessionID from GetConsentRequest

[jcxplorer]

I believe this was missing from #1013. Responses from GET /oauth2/auth/requests/consent/:challenge have the login_challenge and login_session_id fields, but they're always empty strings, because they were not being converted from the database response.

This PR adds the missing fields to the relevant SQL converter test and implementation.

cc @aeneasr

[aeneasr]

Yup, that looks definitely like a bug! Can you please add a test for this as well? I think one place to do that is here and another one in this file. Thank you!

[aeneasr]

Oh sorry, I missed that you added it to the latter file already. Adding it to the first one should assure that the behaviour is correct across all managers.

[aeneasr]

Because another PR was merged, there's a small conflict now which has to be addressed as well.

[jcxplorer]

Thanks for looking at this so quickly, and for the pointers!

I've rebased, and also added the LoginChallenge assertion to compareConsentRequest. However, adding an assertion for LoginSessionID causes a bunch of test failures. I know that a login session ID is only provided in certain cases, but unsure of how to add it to the test. Any idea how to proceed?

[aeneasr]

Hm, that's interesting, the session ID should always be set. Maybe it hints to another error?

[jcxplorer]

Found the issue. There were two struct fields with the same tag of db:"login_session_id" in sqlAuthenticationRequest and sqlConsentRequest. Since sqlConsentRequest embeds sqlAuthenticationRequest it ended up having both fields with the same tag. When mapping from the DB to the struct, it was only setting the value to sqlAuthenticationRequest.SessionID (first struct field it finds with tag).

What I've done was rename sqlAuthenticationRequest.SessionID to sqlAuthenticationRequest.LoginSessionID and remove sqlConsentRequest.LoginSessionID. I think this reflects the database structure better.

I've also added the missing test for session ID in the authentication request and also fixed that converter.

[aeneasr]

Yeah awesome, good thing the other test was added too :) Thank you!

[jcxplorer]

And thank you @aeneasr for handling this so quickly. This feature makes Hydra really flexible for us. 👍