always add values to :session #24

Closed
xeqi opened this Issue Sep 13, 2012 · 7 comments

Projects

None yet

2 participants

@xeqi
xeqi commented Sep 13, 2012

Friend looks like it only adds identity information for a new-auth?. This means if another part of the stack adds data to the :session for a request that passes authorization, the identity information will be forgotten.

Talking to @weavejester on irc it sounds like friend needs to be responsible for copying the information it desires to be saved from the request's :session to keep it. Perhaps https://github.com/mmcgrana/ring/blob/master/ring-core/src/ring/middleware/flash.clj could be helpful as a template.

@cemerick
Owner
cemerick commented Oct 2, 2012

I believe gh-26 fixed this?

@xeqi
xeqi commented Oct 2, 2012

gh-26 handled keeping the session where a redirect was required during authentications. This one is for post-authentication. I think it involves adding the session in at https://github.com/deliminator/friend/blob/a93cee659dd24a8e89f6deea6a4e5a66f5d1d2ca/src/cemerick/friend.clj#L193.

It might be easier and cover all the cases to wrap around https://github.com/deliminator/friend/blob/a93cee659dd24a8e89f6deea6a4e5a66f5d1d2ca/src/cemerick/friend.clj#L178 and maintain the session data.

Also, maintaining the session should be a conditional assoc in case a middleware inside has already brought it over from the request and begun changing the session.

@cemerick
Owner
cemerick commented Oct 2, 2012

Good point re: the conditional assoc. That should be settled with e03df1e.

Friend should be entirely transparent outside of workflow handling and the stuff around authorization. So, when it passes the request along to the next handler in the chain, should it be trying to patch up downstream session (mis?) handling? I wouldn't think so, but I'm open to counterpoint.

@cemerick
Owner
cemerick commented Oct 2, 2012

Thanks for pushing back on this in irc. Also with a /ht @weavejester, I think what's on master now is in compliance with normative handling of Ring sessions (not an obvious thing, especially in middleware!).

I'd like to cut an 0.0.12 release later today. Someone holler if things look off. :-)

@xeqi
xeqi commented Oct 2, 2012

Does logout needs the conditional check for a :session already in the response?

@cemerick
Owner
cemerick commented Oct 2, 2012

@xeqi Good eye. Tweaked on master.

@cemerick
Owner
cemerick commented Oct 2, 2012

Going to call this good for now. If anyone sees other :session-related oddness with Friend, please file another issue.

@cemerick cemerick closed this Oct 2, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment