WIP feat(oauth): signal action=signup or action=signin at end of oauth flow. #2369
Conversation
|
|
||
| if (result.error) { | ||
| // really, we should be parsing the URL and adding the error | ||
| // parameter. That requires more code than this. | ||
| var separator = redirectTo.indexOf('?') === -1 ? '?' : '&'; | ||
| separator = redirectTo.indexOf('?') === -1 ? '?' : '&'; |
shane-tomlinson
May 3, 2015
Member
Maybe it's time to parse the URI, then make use of lib.js->objToSearchString, this is starting to look a bit out of control. Not sure how much more effort that would be.
Maybe it's time to parse the URI, then make use of lib.js->objToSearchString, this is starting to look a bit out of control. Not sure how much more effort that would be.
rfk
May 3, 2015
Author
Member
yes, it's definitely time
yes, it's definitely time
| @@ -103,13 +103,13 @@ define([ | |||
| // The slight delay is to allow the functional tests time to bind | |||
| // event handlers before the flow completes. | |||
| var self = this; | |||
| return p().delay(100).then(_.bind(self.finishOAuthFlow, self, account)); | |||
| return p().delay(100).then(_.bind(self.finishOAuthFlow, self, account, { action: 'signup' })); | |||
shane-tomlinson
May 3, 2015
Member
nit: this line wraps in GitHub side by side view, can you split it before the .delay(100) or before the .then?
nit: this line wraps in GitHub side by side view, can you split it before the .delay(100) or before the .then?
| @@ -119,6 +119,8 @@ define([ | |||
| }, | |||
|
|
|||
| afterSignIn: function (account, additionalResultData) { | |||
| // Signal to the RP that this was an existing account sign-in. | |||
| additionalResultData.action = 'signin'; | |||
|
@rfk - can you rebase this and I can review when ready? |
| @@ -156,9 +163,12 @@ function (chai, sinon, Session, p, OAuthClient, Assertion, AuthErrors, | |||
|
|
|||
| return broker.afterResetPasswordConfirmationPoll(account) | |||
| .then(function () { | |||
| assert.isTrue(broker.finishOAuthFlow.calledWith(account)); | |||
| assert.isTrue(broker.finishOAuthFlow.calledWith(account, {; | |||
pdehaan
May 18, 2015
Contributor
What's this little ; doing at the end here?
What's this little ; doing at the end here?
rfk
force-pushed the
rfk/signal-auth-type
branch
2 times, most recently
from
9b7dbb0
to
3381470
|
OK, rebased and cleaned up a little. I'm not thrilled about this architecturally, it seems to spread the concern across too much of the codebase. But I don't have a better suggestion, since it really is a property of the flow rather than a property of any particular object in the system. I toyed with putting some sort of "isNewAccount()" indicator on the account object, but it didn't seem to come out any cleaner by the time you figure out how to un-set this flag properly after the first sign-in. At least here the logic is contained to the oauth backends. |
| @@ -59,14 +68,15 @@ define([ | |||
| }); | |||
| }, | |||
|
|
|||
| finishOAuthFlow: function (account) { | |||
| finishOAuthFlow: function (account, additionalResultData) { | |||
shane-tomlinson
May 19, 2015
Member
Maybe instead of only using finishOAuthFlow, there could be two functions finishOAuthSignUp and finishOAuthSignIn. These two functions would each call finishOAuthFlow with the appropriate action.
return self.finishOAuthFlow(account, {
action: Constants.OAUTH_ACTION_SIGNIN
}).then(function () {
This could be converted to:
return self.finishOAuthSignIn(account)
.then(function () {
Maybe instead of only using finishOAuthFlow, there could be two functions finishOAuthSignUp and finishOAuthSignIn. These two functions would each call finishOAuthFlow with the appropriate action.
return self.finishOAuthFlow(account, {
action: Constants.OAUTH_ACTION_SIGNIN
}).then(function () {This could be converted to:
return self.finishOAuthSignIn(account)
.then(function () {| @@ -19,15 +20,23 @@ define([ | |||
| return p() | |||
| .then(function () { | |||
| var redirectTo = result.redirect; | |||
| var params = {}; | |||
|
|
|||
| var startOfParams = redirectTo.lastIndexOf('?'); | |||
shane-tomlinson
May 19, 2015
Member
Maybe extract the URL->search params section into lib/url.js? It certainly could be reused in the future, and it'll hopefully reduce the amount of code in this function.
Maybe extract the URL->search params section into lib/url.js? It certainly could be reused in the future, and it'll hopefully reduce the amount of code in this function.
| @@ -92,6 +95,18 @@ define([ | |||
| return p.reject(new Error('subclasses must override sendOAuthResultToRelier')); | |||
| }, | |||
|
|
|||
| finishOAuthSignInFlow: function (account, additionalResultData) { | |||
| if (! additionalResultData) { additionalResultData = {}; } | |||
shane-tomlinson
May 20, 2015
Member
This is my lone nit in this PR.
We usually set defaults/missing parameters using this form:
additionalResultData = additionalResultData || {};
This is my lone nit in this PR.
We usually set defaults/missing parameters using this form:
additionalResultData = additionalResultData || {};
Here's my WIP experiment with signalling "action=signup" or "action=signin" at the end of the OAuth flow. Most definitely now ready yet, but thought I'd put it out there early.
Fixes #2345