-
Notifications
You must be signed in to change notification settings - Fork 141
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 custom state handling #60
Add custom state handling #60
Conversation
EXAMPLES.md
Outdated
customProperty: req.someProperty, | ||
}; | ||
// This value will be sent in a URL parameter so it should be transfer-safe. | ||
return req.openid.encodeState(state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing this for flexibility but could do encoding/decoding in the library automatically ... or allow both, if the value is not a string then encode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any use cases where the state should not be encoded? Even if it is a string, it presumably has the potential to contain URL-unsafe characters.
If that holds true I would move the encode operation into the library so that it just always does it and not let the developer hang themselves by doing something that might break the redirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Steve. I think you're right, there's just really no reason to require a developer to encode this state if we're managing everything else for them. Will adjust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -84,7 +88,3 @@ function createNonce() { | |||
function deleteCookie(name, res) { | |||
res.cookie(name, '', {maxAge: 0}); | |||
} | |||
|
|||
exports.store = store; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moving these to the top for clarity
EXAMPLES.md
Outdated
customProperty: req.someProperty, | ||
}; | ||
// This value will be sent in a URL parameter so it should be transfer-safe. | ||
return req.openid.encodeState(state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any use cases where the state should not be encoded? Even if it is a string, it presumably has the potential to contain URL-unsafe characters.
If that holds true I would move the encode operation into the library so that it just always does it and not let the developer hang themselves by doing something that might break the redirect.
Added the config key getLoginState to accept a function for generating custom state values. By default, this will store the post-callback redirect URL and a nonce to ensure unique values. This can be used to store information about the request before redirecting to login.
@stevehobbsdev - Thanks again for your feedback here. I like this implementation much better! Ready for re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
getLoginState
set to a function. See EXAMPLES.md in this PR for a how-to.nonce
andstate
in the options object passed toreq.openid.login()
req.openid.encodeState()
andreq.openid.decodeState()
Testing
Checklist
master