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

Compatibility for auth handlers without refresh (Revisited) #118

Merged
merged 10 commits into from Jul 1, 2019

Conversation

@timgerstel
Copy link

commented Jun 26, 2019

Old authentication handlers which do not support session refreshing will not longer return a 400 or 500 error due to lack of capability, but will allow the session to expire naturally (user logs out/timeout).

Must be merged with:
zowe/zss-auth#19
zowe/zosmf-auth#14

Signed-off-by: Timothy Gerstel tgerstel@rocketsoftware.com

Timothy Gerstel added some commits Jun 24, 2019

Timothy Gerstel
Calls to handler functions are now wrapped in a statement to check th…
…eir capabilities list, namely to see if they support session refreshing

Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>
Timothy Gerstel
Added capabilites structure to constructor and getter for capabilities
Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>
Timothy Gerstel
Added support for old nodeAuthenticators that do not implement or mak…
…e use of the refreshStatus function. A capabilities structure has been added to authenticators (separate from this commit)

Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>
Timothy Gerstel
Getter function for capabilities was previously named incorrectly
Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>
Timothy Gerstel
Modification to trivialAuth capabilities list. canRefresh is now true
Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>
Timothy Gerstel
Removed unused function
Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>
@jordanfilteau1995
Copy link
Contributor

left a comment

Line 211, you have a typo. I don't think this builds.

@timgerstel

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

You are correct, good catch. Will fix then comment back here, thank you @jordanfilteau1995

Timothy Gerstel
Fixed incorrect reference to variable. Added log for failed handlerRe…
…sults

Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>
@timgerstel

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

Added fix @jordanfilteau1995

if (type === SESSION_ACTION_TYPE_REFRESH
&& authManager.sessionTimeoutMs !== TIMEOUT_VALUE_NO_EXPIRE
&& (!timeout || timeout < Date.now())) {
req.session.zlux = undefined;

This comment has been minimized.

Copy link
@1000TurquoisePogs

1000TurquoisePogs Jun 26, 2019

Contributor

This should not be inside of hasGetCapabilities - this is checking if the server itself has timed out, not if an auth plugin has.

Consider:
if (this stuff) {
REASON_ZLUX_SESSION_EXPIRE
} else if (hasGetCapabilities && canRefresh) {
do refresh
} else {
success = true
}

Timothy Gerstel
Separated server check for timeout from capability check for auth han…
…dlers

Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>
} else {
handlerResult = yield handler[functionName](req,
authPluginSession);
handlerResult = (functionName == 'refreshStatus') ? { success: true } : yield handler[functionName](req, authPluginSession);

This comment has been minimized.

Copy link
@fkovinAtRocket

fkovinAtRocket Jun 27, 2019

Contributor

Line longer than 120 character

&& authManager.sessionTimeoutMs !== TIMEOUT_VALUE_NO_EXPIRE
&& (!timeout || timeout < Date.now())) {
req.session.zlux = undefined;
handlerResult = {success:false, reason:REASON_ZLUX_SESSION_EXPIRE};
} else if(hasGetCapabilities && handler.getCapabilities().canRefresh === true && functionName == 'refreshStatus'){

This comment has been minimized.

Copy link
@fkovinAtRocket

fkovinAtRocket Jun 27, 2019

Contributor

Line longer than 120 character

This comment has been minimized.

Copy link
@timgerstel
Timothy Gerstel
Improved conciseness of logic for handler function calls
Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>

Requested change implemented

lib/webauth.js Outdated Show resolved Hide resolved
Timothy Gerstel
Reference to handler functions no longer relies on functionName varia…
…ble. Further separation of refresh and authentication actions

Signed-off-by: Timothy Gerstel <tgerstel@rocketsoftware.com>

@1000TurquoisePogs 1000TurquoisePogs merged commit 5422f71 into zowe:staging Jul 1, 2019

2 checks passed

DCO DCO
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.