Restrict Front/Back Channel logout PRs associated with logged out session. #1673
Conversation
|
|
obasajujoshua31
force-pushed
the
fix-1660
branch
2 times, most recently
from
ab58267
to
f4b0adf
Compare
|
Let me know when this is good to review! |
obasajujoshua31
force-pushed
the
fix-1660
branch
from
f4b0adf
to
3b7ec8a
Compare
|
Hi @aeneasr it is ready for review now. |
consent/manager_test_helpers.go
Outdated
| } | ||
| err = m.CreateLoginSession(context.TODO(), &fakeLoginSession) | ||
| require.NoError(t, err) | ||
| c.LoginSessionID = fakeLoginSession.ID // otherwise we had to create the login session as well.. |
There was a problem hiding this comment.
You can remove the comment because it is no longer accurate
There was a problem hiding this comment.
This also applies to the other modified tests!
consent/manager_test_helpers.go
Outdated
| require.NoError(t, m.CreateLoginRequest(context.TODO(), lc)) | ||
|
|
||
| require.NoError(t, m.CreateConsentRequest(context.TODO(), c)) | ||
| _, err = m.HandleConsentRequest(context.TODO(), c.Challenge, h) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| clients, err := m.ListUserAuthenticatedClientsWithFrontChannelLogout(context.TODO(), "subjectLUACWFCL") | ||
| clients, err := m.ListUserAuthenticatedClientsWithFrontChannelLogout(context.TODO(), "subjectLUACWFCL-5", "fk-login-session-LUACWFCL-5") |
There was a problem hiding this comment.
The test is misleading in my opinion. So what you're doing is you are creating 5 sessions for 5 different identities and assign that to the same client.
This will always return the one client you expect - unless you call for an invalid session (e.g. I-AM-NOT-fk-login-session-LUACWFCL-5), which is why all the tests pass here.
What you should do instead is create two identities with two sessions each, and assign each of these sessions to two clients each. So for example:
- identity 1 + session 1 + client id 1
- identity 1 + session 2 + client id 2
- identity 2 + session 3 + client id 3
- identity 2 + session 4 + client id 4
Now you will be checking against those combinations, so basically:
- Does identity 1 and session 1 give client 1?
- Does identity 1 and session 2 give client 2?
- ...
And you will also need a negative test, so:
- Does identity 1 and session 1 give client 2? -> Expected result is no!
- Does identity 1 and session 2 give client 1? -> Expected result is no!
There was a problem hiding this comment.
This also applies to the other modified tests!
obasajujoshua31
force-pushed
the
fix-1660
branch
from
3b7ec8a
to
921c083
Compare
|
Please run |
|
Closing in favor of #1691 |
Related issue
Issue #1660
Proposed changes
Checklist
I added logging_session_id with the db query script to search for clients from the hydra_oauth2_consent_request table.
vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
Further comments