From 125dd1eb44123662a26bc8570710f68d53199c59 Mon Sep 17 00:00:00 2001 From: Chad Shurtz Date: Wed, 11 Jan 2017 08:56:45 -0800 Subject: [PATCH 1/5] Update string to include live.com in the message When I was playing around with the third-party cookie settings in Firefox, it looked like live.com was set as an exception by default, so I decided to take it out of the original error message about cookies being disabled. But, it seems that cookies are only allowed on the sign in endpoint. For whatever reason, if you don't have the live.com domain added as an exception, you are unable to sign out of the clipper. So, I've added back in the need to add live.com to the list of exceptions. --- src/strings.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/strings.json b/src/strings.json index cf820c2e..9bf171a3 100644 --- a/src/strings.json +++ b/src/strings.json @@ -45,7 +45,7 @@ "WebClipper.Error.ConflictingExtension": "Your PDF viewer or another extension might be blocking the OneNote Web Clipper. You could temporarily disable the following extension and try clipping again.", "WebClipper.Error.CannotClipPage": "Sorry, this type of page can\u0027t be clipped.", "WebClipper.Error.CookiesDisabled.Line1": "Cookies must be enabled in order for OneNote Web Clipper to work correctly.", - "WebClipper.Error.CookiesDisabled.Line2": "Please allow third-party cookies in your browser or add the onenote.com domain as an exception.", + "WebClipper.Error.CookiesDisabled.Line2": "Please allow third-party cookies in your browser or add the onenote.com and live.com domains as an exception.", "WebClipper.Error.CorruptedSection": "Your clip can\u0027t be saved here because the section is corrupt.", "WebClipper.Error.GenericError": "Something went wrong. Please try clipping the page again.", "WebClipper.Error.GenericExpiredTokenRefreshError": "Your login session has ended and we were unable to clip the page. Please sign in again.", From b2ff2e7aaccc54517da1579a4f70701e5d18ce2d Mon Sep 17 00:00:00 2001 From: Chad Shurtz Date: Wed, 11 Jan 2017 08:59:50 -0800 Subject: [PATCH 2/5] Remove the error description from the "More" info When I was doing the cleanup for the third-party cookie message, it looks like I accidentally left the error description in both places. --- src/scripts/clipperUI/panels/signInPanel.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/scripts/clipperUI/panels/signInPanel.tsx b/src/scripts/clipperUI/panels/signInPanel.tsx index d26ddf77..2f5b7c4c 100644 --- a/src/scripts/clipperUI/panels/signInPanel.tsx +++ b/src/scripts/clipperUI/panels/signInPanel.tsx @@ -100,9 +100,6 @@ class SignInPanelClass extends ComponentBase debugInformation() { if (this.signInErrorDetected() && this.state.debugInformationShowing) { return
- - {this.props.clipperState.userResult.data.errorDescription} -
  • {ClientType[this.props.clipperState.clientInfo.clipperType]}: {this.props.clipperState.clientInfo.clipperVersion}
    • From 542bcecf201604aaa69d782252e5f7bb05114dd2 Mon Sep 17 00:00:00 2001 From: Chad Shurtz Date: Wed, 11 Jan 2017 09:01:32 -0800 Subject: [PATCH 3/5] Fix the handleSignIn() error object In order to know if we should show the error description when sign in errors occur, we were counting on the updateReason property being set on the returning object. This was true until my last change, where we always return a valid UserInfo (except in this one case). After my change, the updateReason property was getting blown away and because of that, the logic around when to show the error message could never be true. --- src/scripts/clipperUI/clipper.tsx | 4 ++-- src/scripts/extensions/extensionWorkerBase.ts | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/scripts/clipperUI/clipper.tsx b/src/scripts/clipperUI/clipper.tsx index d049237d..6ac4f09c 100644 --- a/src/scripts/clipperUI/clipper.tsx +++ b/src/scripts/clipperUI/clipper.tsx @@ -576,7 +576,7 @@ class ClipperClass extends ComponentBase { let handleSignInEvent = new Log.Event.PromiseEvent(Log.Event.Label.HandleSignInEvent); this.setState({ userResult: { status: Status.InProgress } }); - type ErrorObject = { correlationId?: string, error: string, errorDescription: string }; + type ErrorObject = { updateReason: UpdateReason, correlationId?: string, error: string, errorDescription: string }; Clipper.getExtensionCommunicator().callRemoteFunction(Constants.FunctionKeys.signInUser, { param: authType, callback: (data: UserInfo | ErrorObject) => { // For cleaner referencing let updatedUser = data as UserInfo; @@ -598,7 +598,7 @@ class ClipperClass extends ComponentBase { handleSignInEvent.setFailureInfo({ error: error }); errorObject.errorDescription = error; - this.state.setState({ userResult: { status: Status.Failed, data: updatedUser } }); + this.state.setState({ userResult: { status: Status.Failed, data: errorObject } }); Clipper.logger.logUserFunnel(Log.Funnel.Label.AuthSignInFailed); } diff --git a/src/scripts/extensions/extensionWorkerBase.ts b/src/scripts/extensions/extensionWorkerBase.ts index 78049aee..823c5ac7 100644 --- a/src/scripts/extensions/extensionWorkerBase.ts +++ b/src/scripts/extensions/extensionWorkerBase.ts @@ -511,6 +511,11 @@ export abstract class ExtensionWorkerBase { }).catch((errorObject) => { // Set the user info object to undefined as a result of an attempted sign in this.auth.user.set({ updateReason: UpdateReason.SignInAttempt }); + + // Right now we're adding the update reason to the errorObject as well so that it is preserved in the callback. + // The right thing to do is revise the way we use callbacks in the communicator and instead use Promises so that + // we are able to return distinct objects. + errorObject.updateReason = UpdateReason.SignInAttempt; return Promise.reject(errorObject); }); }); From 2e338bb3f94169e9ff913abd7569b9a51c00bae1 Mon Sep 17 00:00:00 2001 From: Chad Shurtz Date: Wed, 11 Jan 2017 11:28:09 -0800 Subject: [PATCH 4/5] Clean up the signInUser callback Should have done this at the beginning. The previous version is a little too hard to read. --- src/scripts/clipperUI/clipper.tsx | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/scripts/clipperUI/clipper.tsx b/src/scripts/clipperUI/clipper.tsx index 6ac4f09c..91a8d7bc 100644 --- a/src/scripts/clipperUI/clipper.tsx +++ b/src/scripts/clipperUI/clipper.tsx @@ -577,40 +577,35 @@ class ClipperClass extends ComponentBase { this.setState({ userResult: { status: Status.InProgress } }); type ErrorObject = { updateReason: UpdateReason, correlationId?: string, error: string, errorDescription: string }; - Clipper.getExtensionCommunicator().callRemoteFunction(Constants.FunctionKeys.signInUser, { param: authType, callback: (data: UserInfo | ErrorObject) => { + + Clipper.getExtensionCommunicator().callRemoteFunction(Constants.FunctionKeys.signInUser, { + param: authType, callback: (data: UserInfo | ErrorObject) => { // For cleaner referencing let updatedUser = data as UserInfo; let errorObject = data as ErrorObject; - // Unexpected errors - let errorPrefix = AuthType[authType] + "; "; - let error: string; - if (!updatedUser) { - error = errorPrefix + "The " + Constants.FunctionKeys.signInUser + " remote function incorrectly returned an undefined object"; - } else if (errorObject.error || errorObject.errorDescription) { - // Something went wrong on the auth server - error = errorPrefix + errorObject.error + ": " + errorObject.errorDescription; - handleSignInEvent.setCustomProperty(Log.PropertyName.Custom.CorrelationId, errorObject.correlationId); - } - - if (error) { - handleSignInEvent.setStatus(Log.Status.Failed); - handleSignInEvent.setFailureInfo({ error: error }); + let errorsFound = errorObject.error || errorObject.errorDescription; + if (errorsFound) { + errorObject.errorDescription = AuthType[authType] + ": " + errorObject.error + ": " + errorObject.errorDescription; - errorObject.errorDescription = error; this.state.setState({ userResult: { status: Status.Failed, data: errorObject } }); + handleSignInEvent.setStatus(Log.Status.Failed); + handleSignInEvent.setFailureInfo({ error: errorObject.errorDescription }); + handleSignInEvent.setCustomProperty(Log.PropertyName.Custom.CorrelationId, errorObject.correlationId); + Clipper.logger.logUserFunnel(Log.Funnel.Label.AuthSignInFailed); } let userInfoReturned = updatedUser && !!updatedUser.user; if (userInfoReturned) { - // Sign in succeeded Clipper.storeValue(ClipperStorageKeys.hasPatchPermissions, "true"); Clipper.logger.logUserFunnel(Log.Funnel.Label.AuthSignInCompleted); } + handleSignInEvent.setCustomProperty(Log.PropertyName.Custom.UserInformationReturned, userInfoReturned); - handleSignInEvent.setCustomProperty(Log.PropertyName.Custom.SignInCancelled, !error && !userInfoReturned); + handleSignInEvent.setCustomProperty(Log.PropertyName.Custom.SignInCancelled, !errorsFound && !userInfoReturned); + Clipper.logger.logEvent(handleSignInEvent); }}); } From 8e60663cb603470d12dd27349988217a2c83f65e Mon Sep 17 00:00:00 2001 From: Chad Shurtz Date: Wed, 11 Jan 2017 11:38:18 -0800 Subject: [PATCH 5/5] Minor cleanup tweak --- src/scripts/clipperUI/clipper.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/scripts/clipperUI/clipper.tsx b/src/scripts/clipperUI/clipper.tsx index 91a8d7bc..3afbc57f 100644 --- a/src/scripts/clipperUI/clipper.tsx +++ b/src/scripts/clipperUI/clipper.tsx @@ -576,11 +576,18 @@ class ClipperClass extends ComponentBase { let handleSignInEvent = new Log.Event.PromiseEvent(Log.Event.Label.HandleSignInEvent); this.setState({ userResult: { status: Status.InProgress } }); - type ErrorObject = { updateReason: UpdateReason, correlationId?: string, error: string, errorDescription: string }; + + type ErrorObject = { + updateReason: UpdateReason, + correlationId?: string, + error: string, + errorDescription: string + }; Clipper.getExtensionCommunicator().callRemoteFunction(Constants.FunctionKeys.signInUser, { param: authType, callback: (data: UserInfo | ErrorObject) => { // For cleaner referencing + // TODO: This kind of thing should go away as we move the communicator to be Promise based. let updatedUser = data as UserInfo; let errorObject = data as ErrorObject;