search

flexdash / node-red-flexdash

Node-RED server integration for FlexDash
MIT License
10 stars 3 forks source link

Only add cookie-session if req.session doesn't already exist #13

Closed knolleary closed 1 year ago

knolleary commented 1 year ago

Fixes #12

One piece I'm not sure on is the socket.io handling - I can see you use some tricks to add headers to the websocket request. In my local testing I think that was okay to leave as-is, but would appreciate your input as you've clearly been down that path before.

knolleary commented 1 year ago

Hey @tve - are you able to look at this PR? The issue is prevent anyone with adminAuth setup on their Node-RED from using FlexDash - would be great to unblock that.

tve commented 1 year ago

Thanks for the nudge! I had taken a look at the changes and couldn't tell the impact. FlexDash in general doesn't require a cookie, but as soon as the flows need to track users in any way for auth or preferences a cookie is needed. The PR removes the cookie set by FD, which means that this functionality will break as far as I can tell. If FD can't set a cookie then it would need to rely on a cookie set by FF and I don't know whether that's the case and how to hook the cookie ID into FD. That's where I left it, wondering a bit about how I was going to even dev&test that.

knolleary commented 1 year ago

The PR removes the cookie set by FD,

I do not believe that is the case.

This PR prevents FD adding a second session handler if there is one already there. The FD cookie will still get set using the existing session handler.

tve commented 1 year ago

I can't easily test this at the moment, but since it doesn't impact non-FlowForge uses of FD it seems fine to merge it. Thanks for the PR!