fix: defer creation of auth request. #1865
Conversation
|
@al45tair can you please rebase your PR? |
|
I'll try to get to this soon. Probably not going to be this week though (unless I do it at the weekend; we'll see). |
There was a problem hiding this comment.
Hello, @al45tair. Thank you for sending this PR. I have some suggestions/questions regarding the code. Hope that you will find it helpful.
Since I am a new maintainer, it will be impossible to merge this only with my approval. We will have to wait for a review from another maintainer.
Rather than creating the auth request when the user hits /auth, pass
the arguments through to /auth/{connector} and have the auth request
created there. This prevents a database error when using the "Select
another login method" link, and also avoids a few other error cases.
Fixes dexidp#1849, dexidp#646.
Signed-off-by: Alastair Houghton <alastair@alastairs-place.net>
The back link on the password page was using Javascript to tell the browser to navigate back, which won't work if the user has entered a set of incorrect log-in details. Fix this by using an explicit URL instead. Fixes dexidp#1851 Signed-off-by: Alastair Houghton <alastair@alastairs-place.net>
Accidentally added some of these back during merge. Signed-off-by: Alastair Houghton <alastair@alastairs-place.net>
Reinstating this test as it shouldn't have been removed. Signed-off-by: Alastair Houghton <alastair@alastairs-place.net>
By adding an extra endpoint and a redirect, we can avoid a situation where it's trivially easy to generate a large number of AuthRequests by hitting F5/refresh in the browser. Signed-off-by: Alastair Houghton <alastair@alastairs-place.net>
|
Looks good, thanks! I am going to test it and will come back to you with more feedback. |
|
I should probably say: I've changed jobs. This PR was done as part of my previous employment, and because of my new employer's policies surrounding Open Source contributions, I won't be able to work on it — or any other Dex related things —any further. If further changes are needed, @tomqwpl is probably the person to talk to. |
|
@al45tair, thanks for letting us know. I can only wish you a stroke of good luck in a new place then! We appreciate the contributions you have made so far to Dex. |
|
Thanks for your contributions @al45tair ! Good luck out there! |
|
Great to see this happen 👏 🎉 |
Overview:
Defer creation of the auth request in Dex's back-end storage until the user chooses an authentication method.
What problem does it solve?:
Right now, the option to change your mind and use a different authentication method won't work at all, because it finds the existing auth request and the connector ID is already set for that request (#1849).
There are also some other problems, like the auth request expiring while the user is looking at the log-in page (#646).
Additionally, deferring creation of the auth request allows us to fix the back link on the password page so it works properly if the user fails to authenticate first time (#1851).