-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't touch authentication cookie on skipped logins #1564
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Could you also add a test please? If you need guidance let me know
Oh and please sign the CLA :) |
Ping :) |
I signed the CLA. I'll try to add a test this week. I assume there was no test for this specific case then? ;) |
Perfect - yes exactly ;) |
…For==0 (for login)
@aeneasr I updated the docs regarding RememberFor value for login. Regarding the tests I was thinking of retrieving the auth cookie from the cookie jar and making sure the for _, e := range selected {
cookies = append(cookies, &http.Cookie{Name: e.Name, Value: e.Value})
} So I'm not sure how to properly test this. |
Ah, I see - I think that's why the tests didn't catch it, because the Cookie Jar doesn't really concern itself with sessions like the browser does. I think the only way to get the cookie value is to somehow use the gorilla session decoder to get it (I've tried before but failed but it should in theory be possible IMO) or write a small endpoint and add it to the test server that simply echoes the cookie values as e.g. a json string. |
@aeneasr Since I'm not too familiar with Go and don't have much time to work on this, can you take it from there? |
SGTM - thank you for your contribution! |
Related issue
This will fix #1557
Proposed changes
When login was skipped, return the session and don't touch the existing authentication cookie (which would alter its expiry date).
Checklist
vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
Further comments