From 4761e3f3d01493dcaebf7c9f8f1ea8b05a733f32 Mon Sep 17 00:00:00 2001 From: Tao Zhou Date: Thu, 9 Jan 2020 13:59:45 +0100 Subject: [PATCH] Convert gr-auth to class and use new API for auth check auth-check API is available since: https://gerrit-review.googlesource.com/c/gerrit/+/185990 Change-Id: Icd5e0183ee42e746c32bfd7929af9796ab752627 --- .../gr-error-manager/gr-error-manager.html | 10 + .../core/gr-error-manager/gr-error-manager.js | 132 ++-- .../gr-error-manager_test.html | 563 ++++++++++-------- .../gr-js-api-interface_test.html | 6 +- .../shared/gr-rest-api-interface/gr-auth.js | 120 +++- .../gr-rest-api-interface/gr-auth_test.html | 250 +++++++- .../gr-rest-api-interface.js | 61 +- .../gr-rest-api-interface_test.html | 129 +--- .../gr-rest-apis/gr-rest-api-helper.js | 48 +- .../gr-rest-apis/gr-rest-api-helper_test.html | 3 +- 10 files changed, 758 insertions(+), 564 deletions(-) diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.html b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.html index 048d39227e..03d8d6a350 100644 --- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.html +++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.html @@ -23,6 +23,9 @@ limitations under the License. + + + diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js index 506cb73540..2a87708472 100644 --- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js +++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.js @@ -64,15 +64,29 @@ }; } + constructor() { + super(); + + /** @type {!Gerrit.Auth} */ + this._authService = Gerrit.Auth; + + /** @type {?Function} */ + this._authErrorHandlerDeregistrationHook; + } + attached() { super.attached(); this.listen(document, 'server-error', '_handleServerError'); this.listen(document, 'network-error', '_handleNetworkError'); - this.listen(document, 'auth-error', '_handleAuthError'); this.listen(document, 'show-alert', '_handleShowAlert'); this.listen(document, 'show-error', '_handleShowErrorDialog'); this.listen(document, 'visibilitychange', '_handleVisibilityChange'); this.listen(document, 'show-auth-required', '_handleAuthRequired'); + + this._authErrorHandlerDeregistrationHook = Gerrit.on('auth-error', + event => { + this._handleAuthError(event.message, event.action); + }); } detached() { @@ -80,10 +94,11 @@ this._clearHideAlertHandle(); this.unlisten(document, 'server-error', '_handleServerError'); this.unlisten(document, 'network-error', '_handleNetworkError'); - this.unlisten(document, 'auth-error', '_handleAuthError'); this.unlisten(document, 'show-auth-required', '_handleAuthRequired'); this.unlisten(document, 'visibilitychange', '_handleVisibilityChange'); this.unlisten(document, 'show-error', '_handleShowErrorDialog'); + + this._authErrorHandlerDeregistrationHook(); } _shouldSuppressError(msg) { @@ -95,32 +110,41 @@ 'Log in is required to perform that action.', 'Log in.'); } - _handleAuthError() { - this._showAuthErrorAlert('Auth error', 'Refresh credentials.'); + _handleAuthError(msg, action) { + this.$.noInteractionOverlay.open().then(() => { + this._showAuthErrorAlert(msg, action); + }); } _handleServerError(e) { const {request, response} = e.detail; - Promise.all([response.text(), this._getLoggedIn()]) - .then(([errorText, loggedIn]) => { - const url = request && (request.anonymizedUrl || request.url); - const {status, statusText} = response; - if (response.status === 403 && - loggedIn && - errorText === AUTHENTICATION_REQUIRED) { - // The app was logged at one point and is now getting auth errors. - // This indicates the auth token is no longer valid. - this._handleAuthError(); - } else if (!this._shouldSuppressError(errorText)) { - this._showErrorDialog(this._constructServerErrorMsg({ - status, - statusText, - errorText, - url, - })); - } - console.error(errorText); - }); + response.text().then(errorText => { + const url = request && (request.anonymizedUrl || request.url); + const {status, statusText} = response; + if (response.status === 403 + && !this._authService.isAuthed + && errorText === AUTHENTICATION_REQUIRED) { + // if not authed previously, this is trying to access auth required APIs + // show auth required alert + this._handleAuthRequired(); + } else if (response.status === 403 + && this._authService.isAuthed + && errorText === AUTHENTICATION_REQUIRED) { + // The app was logged at one point and is now getting auth errors. + // This indicates the auth token may no longer valid. + // Re-check on auth + this._authService.clearCache(); + this.$.restAPI.getLoggedIn(); + } else if (!this._shouldSuppressError(errorText)) { + this._showErrorDialog(this._constructServerErrorMsg({ + status, + statusText, + errorText, + url, + })); + } + console.log(`server error: ${errorText}`); + }); } _constructServerErrorMsg({errorText, status, statusText, url}) { @@ -142,10 +166,6 @@ console.error(e.detail.error.message); } - _getLoggedIn() { - return this.$.restAPI.getLoggedIn(); - } - /** * @param {string} text * @param {?string=} opt_actionText @@ -222,7 +242,11 @@ this.knownAccountId !== undefined && timeSinceLastCheck > STALE_CREDENTIAL_THRESHOLD_MS) { this._lastCredentialCheck = Date.now(); - this.$.restAPI.checkCredentials(); + + // check auth status in case: + // - user signed out + // - user switched account + this._checkSignedIn(); } } @@ -232,22 +256,36 @@ } _checkSignedIn() { - this.$.restAPI.checkCredentials().then(account => { - const isLoggedIn = !!account; - this._lastCredentialCheck = Date.now(); - if (this._refreshingCredentials) { - if (isLoggedIn) { - // If the credentials were refreshed but the account is different - // then reload the page completely. - if (account._account_id !== this.knownAccountId) { - this._reloadPage(); - return; - } + this._lastCredentialCheck = Date.now(); - this._handleCredentialRefreshed(); - } else { - this._requestCheckLoggedIn(); - } + // force to refetch account info + this.$.restAPI.invalidateAccountsCache(); + this._authService.clearCache(); + + this.$.restAPI.getLoggedIn().then(isLoggedIn => { + // do nothing if its refreshing + if (!this._refreshingCredentials) return; + + if (!isLoggedIn) { + // check later + // 1. guest mode + // 2. or signed out + // in case #2, auth-error is taken care of separately + this._requestCheckLoggedIn(); + } else { + // check account + this.$.restAPI.getAccount().then(account => { + if (this._refreshingCredentials) { + // If the credentials were refreshed but the account is different + // then reload the page completely. + if (account._account_id !== this.knownAccountId) { + this._reloadPage(); + return; + } + + this._handleCredentialRefreshed(); + } + }); } }); } @@ -277,6 +315,10 @@ this._refreshingCredentials = false; this._hideAlert(); this._showAlert('Credentials refreshed.'); + this.$.noInteractionOverlay.close(); + + // Clear the cache for auth + this._authService.clearCache(); } _handleWindowFocus() { @@ -299,4 +341,4 @@ } customElements.define(GrErrorManager.is, GrErrorManager); -})(); +})(); \ No newline at end of file diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html index 84b6717fe7..af978199f8 100644 --- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html +++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.html @@ -23,10 +23,10 @@ limitations under the License. - + - +