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 flag __enableIdPInitiatedLogin to enable idp initiated logins #708

Merged
merged 1 commit into from
Mar 15, 2018

Conversation

luisrudge
Copy link
Contributor

this will also enable impersonation, since it's an IdP initiated login as well. The __enableImpersonation flag is still enabled so we don't break anyone that is already using it.

@luisrudge luisrudge added this to the v9.3.4 milestone Mar 15, 2018
Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ... minor question RE: option name

@@ -179,12 +179,14 @@ WebAuth.prototype.parseHash = function(options, cb) {
*/
WebAuth.prototype.validateAuthenticationResponse = function(options, parsedHash, cb) {
var _this = this;
options.__enableIdPInitiatedLogin =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor but ... I wonder if you shouldn't call this something else since it encompasses 2 different use cases here? Like options.__bypassStateChecking or thereabouts?

@lbalmaceda lbalmaceda self-requested a review March 15, 2018 18:42
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look OK. Backwards compatibility is maintained 👍
On #689 you mentioned "add __enableImpersonation flag to enable impersonation again". You might want to rename this PR's title to reflect that it affects "impersonation" or add a paragraph on the repo's README

@luisrudge luisrudge merged commit 3c60563 into master Mar 15, 2018
@luisrudge
Copy link
Contributor Author

We'll have proper docs for this flag shortly: auth0/docs#5857

@luisrudge luisrudge deleted the feature-new-flag-idp-login branch March 15, 2018 18:46
@jamesacres
Copy link

var shouldBypassStateChecking = !state && !transactionState && options.__enableIdPInitiatedLogin;

I don't think this solves the issue in our usecase where we set state serverside outside of our auth.js and redirect to authorize. We need a way of bypassing state checking entirely with an option for this case I think?

@luisrudge
Copy link
Contributor Author

if you redirect to authorize with the state, authorize will redirect to your redirect_url with the state as well. You just need to make sure to call parseHash with the state you had in the server webAuth.parseHash({hash: window.location.hash, state: stateFromTheServer })

@jamesacres
Copy link

jamesacres commented Mar 16, 2018

Thanks for pointing me in the right direction, I have added the following which allows the state check to always pass.

For security maybe we need a way of finding out if a transaction is in progress and only do this if it isn't, or passing another param to our callback page? Otherwise I guess this simply bypasses the check in all cases even if it did originate from auth0.js?

var hash = (window.location.hash) ? window.location.hash : '#' + location.replace(/http.?:\/\/[^/]+/,'').slice(1);

var parseHashOptions = { 
    hash:   hash
};  

var parsedQs = qs.parse(hash.replace(/^#?\/?/, ''));
if (parsedQs.hasOwnProperty('state')) {
    parseHashOptions.state = parsedQs.state;
}   

auth0.parseHash(parseHashOptions, authorizeCallback);

@luisrudge
Copy link
Contributor Author

Yeah, but then you, as a developer, are intentionally bypassing the state checking, there's nothing we can do. In order to be actually secure, stateFromTheServer has to come from the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants