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

Remove cookie exp & add server exp #97

Conversation

@1000TurquoisePogs
Copy link
Contributor

commented May 8, 2019

The zlux cookie used to have a timeout on it, but it didn't make too much sense versus the individual auth timeouts.
Now, this has been removed in favor of server-side tracking, and the server counts an expiration based on the latest timeout for all auths related to a session.
This makes use of the session object that comes from the cluster, so this works both in single process mode and in cluster mode.

1000TurquoisePogs added some commits May 8, 2019

Relocated auth handler session storage to avoid overlap with id. Made…
… context object to avoid expanding list of args for auth handlers. Made a map to track session ID timeouts without putting it in the cookie.

Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
Some fixes from robert and moving deletion to when no handlers are le…
…ft valid

Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>

@ghost ghost added the review label May 8, 2019

Minor tweaks to make use of the cluster session
Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>

@1000TurquoisePogs 1000TurquoisePogs changed the title WIP Remove cookie exp & add server exp Remove cookie exp & add server exp May 9, 2019

@1000TurquoisePogs 1000TurquoisePogs requested a review from May 9, 2019

1000TurquoisePogs added some commits May 10, 2019

Optional callback should not unconditionally be called
Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
Get rid of testing log
Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
this.configuration, config);
this.configuration,
config,
new AuthPluginContext(plugin));

This comment has been minimized.

Copy link
@fkovinAtRocket

fkovinAtRocket May 10, 2019

Contributor

Formatting question: do we like this style? To me it looks like there's a huge void where the code should be, and then a bunch of code pieces hanging somewhere on the right.

foo(a,
<+4 spaces>b,
<+4 spaces>c);

or

foo(
<+4 spaces>a,
<+4 spaces>b,
<+4 spaces>c);

look less like ASCII art to me.

This comment has been minimized.

Copy link
@fkovinAtRocket

This comment has been minimized.

Copy link
@1000TurquoisePogs

1000TurquoisePogs May 10, 2019

Author Contributor

I like either, as long as observing the 120 char limit, otherwise your suggested style solves it.

@@ -86,14 +92,19 @@ AuthResponse.prototype = {
//or may know ahead of time due to set value or retrievable server config.
handlerResult.expms = handler.sessionExpirationMS;
}
//overall expiration when last auth expires

This comment has been minimized.

Copy link
@fkovinAtRocket

fkovinAtRocket May 10, 2019

Contributor

so when handler A has expired but plugin B hasn't yet, then functions protected by A will stop working, but those protected by B wont?

This comment has been minimized.

Copy link
@1000TurquoisePogs

1000TurquoisePogs May 10, 2019

Author Contributor

Yes. So, we're leaving it up to the UI to make good UI alerts about expiration. I just wanted an overall expiration so that the session wouldn't exist forever.

@fkovinAtRocket

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Just wondering: can we have unit tests for this? It would be super nice if there had been an old set of unit tests that passed before this change. Someone makes changes and also adds/modifies/removes unit tests. Then it's a lot easier to review the code: you see how the set of assertions about the behaviour of the program has changed. It's a nice sort of "index" into the diff, or a "summary" of the changes.

@@ -136,15 +136,17 @@ SessionStore.prototype.destroy = function(sid, callback) {
try {
//console.log('SessionStore.destroy ' + sid);
this.removeSid(sid);
callback(null);
callback && callback(null);

This comment has been minimized.

Copy link
@fkovinAtRocket

fkovinAtRocket May 10, 2019

Contributor

so the callback wasn't optional, but now it is, right?

This comment has been minimized.

Copy link
@1000TurquoisePogs

1000TurquoisePogs May 10, 2019

Author Contributor

Yes - apparently nobody was calling it at all before.

@@ -39,10 +40,15 @@ function getAuthPluginSession(req, pluginID, dflt) {

function setAuthPluginSession(req, pluginID, authPluginSession) {
if (req.session) {
// FIXME Note that it does something only when req.session[pluginID] is
// FIXME Note that it does something only when req.session.authPlugins[pluginID] is

This comment has been minimized.

Copy link
@fkovinAtRocket

fkovinAtRocket May 10, 2019

Contributor

so I used to simply divide the session by having session["com.example.plugin1"], session["com.example.plugin2"], etc, where each of those objects is the plugin's private storage. Now we have session.authPlugins["com.example.plugin1"] instead because we also have session.zluxExp right?

I'd say that, first, we might want to group all framework data also under a separate object in the session, and second, maybe that name can be a little more descriptive, like, session.zlux.expirationTime?

This comment has been minimized.

Copy link
@1000TurquoisePogs

1000TurquoisePogs May 10, 2019

Author Contributor

zluxExp, but also because session.id existed, so previously it was technically true that a plugin could not be named "id". Not sure what wins the race. Sure, we can do zlux & zlux.expirationTime.

result.updateStatus();

result.updateStatus(authManager.sessionTimeoutMs);
if (result.expms !== -1) {

This comment has been minimized.

Copy link
@fkovinAtRocket

fkovinAtRocket May 10, 2019

Contributor

is this -1 some special magic value? where is it supposed to come from?

This comment has been minimized.

Copy link
@1000TurquoisePogs

1000TurquoisePogs May 10, 2019

Author Contributor

I will add a comment: -1 means they explicitly want there to be no expiration.

This comment has been minimized.

Copy link
@fkovinAtRocket

fkovinAtRocket May 10, 2019

Contributor

Maybe we need to also define a constant for it...

@fkovinAtRocket
Copy link
Contributor

left a comment

I've found some minor issues (naming, formatting).

Other than that, this looks fine to me when I'm looking at this "analytically", I can't find any more problems by just gazing at the code. Ideally there need to be unit tests for this, but it's not required right now...

Cleanup & isolation of zlux object in session
Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
@1000TurquoisePogs

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Addressed Fyodor's points, and am told Dmitry gave this a test, so I'm putting it into staging and we can continue from there. Will see if I can get a unit test into the codebase.

@1000TurquoisePogs 1000TurquoisePogs merged commit 6c7eaaa into zowe:staging May 10, 2019

2 checks passed

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

@ghost ghost removed the review label May 10, 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.