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.

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

add port to cookie name to differentiate multiple servers on same domain #95

Conversation

@toddwellman
Copy link
Contributor

commented Apr 29, 2019

Signed-off-by: Todd Wellman twellman@rocketsoftware.com

add port to cookie name to differentiate multiple servers on same domain
Signed-off-by: Todd Wellman <twellman@rocketsoftware.com>

@toddwellman toddwellman requested a review from 1000TurquoisePogs Apr 29, 2019

@ghost ghost assigned toddwellman Apr 29, 2019

@ghost ghost added the review label Apr 29, 2019

@@ -533,6 +533,7 @@ function WebApp(options){
this.expressApp.use(cookieParser());
this.expressApp.use(session({
//TODO properly generate this secret
name: 'connect.sid.' + options.httpsPort,

This comment has been minimized.

Copy link
@1000TurquoisePogs

1000TurquoisePogs Apr 30, 2019

Contributor

This is not 100%.... it's possible for someone to have enabled http and disabled https (though discouraged).
Instead, check if https ISNT being used. If it isn't, use http. If both are being used, you can still use https since you have to choose only 1.

allow cookie to use http port if https port not used
Signed-off-by: Todd Wellman <twellman@rocketsoftware.com>
let port = options.httpsPort;
if (!options.httpsPort) {
port = options.httpPort;
}

This comment has been minimized.

Copy link
@1000TurquoisePogs

1000TurquoisePogs May 2, 2019

Contributor

Better way to do this is:
const port = options.httpsPort ? options.httpsPort : options.httpPort;
Allows use of const for read-only, and also does not need reassignment.

@1000TurquoisePogs 1000TurquoisePogs self-requested a review May 2, 2019

toddwellman added some commits May 2, 2019

clean code for switching ports
Signed-off-by: Todd Wellman <twellman@rocketsoftware.com>
clean code for switching ports
Signed-off-by: Todd Wellman <twellman@rocketsoftware.com>
@1000TurquoisePogs
Copy link
Contributor

left a comment

Yup - tested in the following ways

  • If I login to two servers in 1 browser, does one get logged out?
  • If I do REST requests to either, is one logged out?
  • If I log out of one, am I logged out of the other?
  • If I did log out, do I get 401s on one but not on the other?
    Behavior seems correct now.

@1000TurquoisePogs 1000TurquoisePogs merged commit 9dc88f0 into zowe:staging May 7, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@ghost ghost removed the review label May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.