feat: remove flow cookie #3639
feat: remove flow cookie #3639
Conversation
hperl
force-pushed
the
hperl/flowcookieless-flow
branch
from
e04bae6
to
16fe681
Compare
Codecov Report
@@ Coverage Diff @@
## master #3639 +/- ##
==========================================
+ Coverage 76.10% 76.17% +0.06%
==========================================
Files 132 132
Lines 10076 10030 -46
==========================================
- Hits 7668 7640 -28
+ Misses 1881 1874 -7
+ Partials 527 516 -11
|
| loginChallenge = x.Must(f.ToLoginChallenge(ctx, deps)) | ||
| got1, err = m.GetLoginRequest(ctx, loginChallenge) | ||
| require.NoError(t, err) | ||
| assert.True(t, got1.WasHandled) |
There was a problem hiding this comment.
Should this assertion not still be here? The flow was handled so it should be true right?
There was a problem hiding this comment.
Previously we persisted the flow in the database, so VerifyAndInvalidateLoginRequest set WasHandled to true in the DB. Since we now store the flow in the login verifier, there is no place to store information about whether the flow was already handled.
This is already true even without this patch.
If we want double-submit protection, we should build a separate NonceService (new creates a new nonce, use uses a nonce once) to make sure that the verifiers are only used once.
There was a problem hiding this comment.
I see - do we protect against double submit for the consent verifier? Or can I generate as many authorization codes as I want with the same consent verifier?
Can I log in as often as I want to using the same login verifier?
There was a problem hiding this comment.
You can log in as often as you want, but the double-submit will be caught at the consent verification step.
I added a test case for consent verifier double submit here: f482f4e
| // Trying to update this again should return an error because the consent request was used. | ||
| h.GrantedAudience = sqlxx.StringSliceJSONFormat{"new-audience", "new-audience-2"} | ||
| _, err = m.HandleConsentRequest(ctx, f, h) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Why is it ok to remove this assertion?
There was a problem hiding this comment.
Because we can't detect double submit any more.
| updatedFlow.LoginCSRF = f.LoginCSRF | ||
| updatedFlow.LoginVerifier = f.LoginVerifier | ||
| *f = *updatedFlow | ||
|
|
There was a problem hiding this comment.
Could you briefly explain why we needed this in the first place so I better understand the reason why it's ok to remove this?
There was a problem hiding this comment.
Previously, we merged two flows here: The one decoded from the login verifier (verifier param), and the one from the cookie (f param). The code above ensured that these two flows are the same, so that the same flow information ends up in the cookie agan as well as in the consent challenge.
After removing the cookie flow, we do not have to do this any more.
hperl
force-pushed
the
hperl/flowcookieless-flow
branch
from
16fe681
to
751fa04
Compare
hperl
force-pushed
the
hperl/flowcookieless-flow
branch
from
5bf04ad
to
fbe203c
Compare
This reverts commit a0c06ec.
|
@aeneasr what is still needed here? |
|
This patch removes the flow cookie. All information is already tracked in the request query parameters as part of the {login|consent}_{challenge|verifier}.
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments