Added support for passing session ID using HTTP header#79
Conversation
There was a problem hiding this comment.
Discuss: change to just header, or probably better, id-header?
There was a problem hiding this comment.
@Fishrock123 I think header will be better name for this option.
There was a problem hiding this comment.
in index.js
var signed = 's:' + signature.sign(val, secret);
secret is an Array, instead of a string, so your code has broken.
There was a problem hiding this comment.
Hmm I don't think it should be lower-casing it.
There was a problem hiding this comment.
@Fishrock123 Express.js lower-casing all header names in req.headers object, so it must be lower-cased. Package user can write header name in any case. Probably I should add a test for case-insensitivity of header name option?
There was a problem hiding this comment.
since this is an internal help function, it can make assumptions. just lc the header on the constructor so it's done only once
There was a problem hiding this comment.
@dougwilson but if I convert header name to lower case in the constructor, package user will see lower-cased HTTP header in response, is this ok? I beleive package user should see exactly what he passed to the options.
Probably there should be two variables, one for getHeader function and one for passing unchanged header name back in the response?
There was a problem hiding this comment.
right. i haven't yet read through the changes :) but yes, we should only lc one time. if we need both representations, then just store both
Lower casing header name was removed from getHeader() function.
9ff5ab5 to
5936c25
Compare
Conflicts: index.js
Conflicts: index.js
|
Hi @alexey-detr I'm so sorry I have not paid attention to this PR :( So I was looking over it, and to me, it seems a little weird for someone to be wanting to read from a header the signed session ID value, and especially to want to write out a response header with this value. an you explain to me a little bit about your use-case that inspired you to implement this? |
|
Hi! If you remember we already discussed it in #77. Actually it is a very rare use case when requests are sending with |
Sorry, I forgot :) There are just so many things to remember, and it's mainly just hard to remember people's GitHub handles and put together with convos, sorry!! Ok. So I understand what you are saying. Basically, to me, it sounds like you want to be able to have something you can use as a |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
What if we look to see if This would require a good explanation in the docs, but I think it would be more extensible. |
|
@joewagner I try your solution it is working and consists in :
This last middleware allow the client to grab the session ID easily (no need to parse the set-cookie header — still here but not uses in my case) In my opinion, a better solution could be to merge all this in expressjs-session middleware and add an extra parameter in configuration object (the header name) if someone is interested, i can propose my solution wrapped in a pull request 🍻 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…not use cookies for authorization at all.
|
Guys I have updated the PR. Now it is possible to specify |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
FWIW, the express-mysql-session package (and maybe others) specifically look for cookies.. so this doesn't work for them. Issues should be opened on those packages once this finally makes it in.. |
|
FYI for those coming by here waiting for this to be implemented, for now the only work-around is to add the session ID in the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Just looking through the PR, there was a comment on the name of the option, which was only partially changed; the docs (where the comment still remains) hasn't been updated. Also, the PR currently has conflicts and can't be merged just as a general comment. Even then, there are multiple threads discussing this topic, at in at least one of them, it was said I would rather accept an PR that allowed a user to read the session ID out in whatever way they choose, instead of still locking them into the decisions of this library. For example, you are locked into Cookies because that's who this module did; this PR would now just lock you into Cookie or a Header, which is not a generally better situation, just a quick Band-Aid that should be better addressed in some PR, please. |
This comment has been minimized.
This comment has been minimized.
Totally agree with @dougwilson. There should be a better more general solution for this problem. For example it could be an optional callback functions specified in configuration that will fetch/save session ID in any way you want. So you will be able to pass session ID in cookies, headers, query string, custom field of JSON-objects in POST request... Not just only cookie or header. |
dougwilson
left a comment
There was a problem hiding this comment.
PR needs to be reworked following the discussion in the comments. Also resolve merge conflicts.
|
Just for reference: the above mentioned more general solution can be found in PR #159 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9d2e29b to
408229e
Compare
|
Can someone please check this PR out? It's very needed now since third party cookies are blocked in most browsers... |
These changes add new option
headerNamethat can be used to optional passing session ID using custom HTTP header.Already discussed this issue in #77