Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add private_cookie to LocalRequest #430

Closed
wants to merge 1 commit into from
Closed

add private_cookie to LocalRequest #430

wants to merge 1 commit into from

Conversation

henriiik
Copy link

Hello!

I implemented the feature request in #368 since it had the "help wanted" label. I am still new to rust and any feedback is appreciated. The new method is very much like the cookie method.

@turboladen
Copy link

turboladen commented Nov 6, 2017 •
edited
Loading

Looks good to me. I actually forked earlier to make these same changes. Looking forward to seeing them merged (I can't test most of my app without this).

turboladen added a commit to turboladen/shot_log that referenced this pull request Nov 6, 2017
@turboladen
Add a couple login specs
26dde93 
The ability test things is pretty low. I need to be able to set a
private cookie to get to any of my routes, but that’s not yet in
rocket. A PR for that has been up for almost 2 months
(rwf2/Rocket#430).

Also, I can’t follow redirects like a browser; following them manually
doesn’t cause flashes to occur; I can’t get at the Client’s cookies to
see if the flash cookie is set. …thus no way to check if my “redirect &
flash the error” is actually working. Kinda makes it feel like testing
is a bit of an afterthought.
turboladen added a commit to turboladen/shot_log that referenced this pull request Nov 6, 2017
@turboladen
Add a couple login specs
93129a1 
The ability test things is pretty low. I need to be able to set a
private cookie to get to any of my routes, but that’s not yet in
rocket. A PR for that has been up for almost 2 months
(rwf2/Rocket#430).

Also, I can’t follow redirects like a browser; following them manually
doesn’t cause flashes to occur; I can’t get at the Client’s cookies to
see if the flash cookie is set. …thus no way to check if my “redirect &
flash the error” is actually working.
turboladen added a commit to turboladen/shot_log that referenced this pull request Nov 6, 2017
@turboladen
Add a couple login specs
9e785df 
The ability test things is pretty low. I need to be able to set a
private cookie to get to any of my routes, but that’s not yet in
rocket. A PR for that has been up for almost 2 months
(rwf2/Rocket#430).

Also, I can’t follow redirects like a browser; following them manually
doesn’t cause flashes to occur; I can’t get at the Client’s cookies to
see if the flash cookie is set. …thus no way to check if my “redirect &
flash the error” is actually working.
@SergioBenitez
Copy link
Member

I'm sorry; I haven't had any time to review changes lately. But I'll note that this isn't correct. The way the code's written in this PR means that the added cookie will be marked as "added" to the request, not as part of the original request. As a result, Rocket will generate a Set-Cookie header incorrectly. If you look at the code for Cookies, you'll see that Rocket calls add_original, not simply add, for regular cookies. This prevents the cookie from being marked as "added" and thus the header being emitted. We need to do something similar for private cookies.

There's a correct but inelegant solution that doesn't require changes to the cookie crate, and then there's the better solution that does. Unfortunately I don't have time to go into this in detail now, but I'll leave this here as a note.

@SergioBenitez
Copy link
Member

Closing in favor of #487.

@SergioBenitez SergioBenitez added the pr: closed This pull request was not merged label Dec 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
pr: closed This pull request was not merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@henriiik @turboladen @SergioBenitez