From 871b47369c8d20052a32df0f80dda33379a02de1 Mon Sep 17 00:00:00 2001 From: dpalou Date: Thu, 4 Oct 2018 10:21:42 +0200 Subject: [PATCH 1/2] MOBILE-2553 login: Improve error handling in check site --- src/lang/en.json | 2 +- src/providers/sites.ts | 34 ++++++++++++++++------------------ src/providers/utils/text.ts | 4 ++++ 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/lang/en.json b/src/lang/en.json index 4e165a8b6..a3989c56f 100644 --- a/src/lang/en.json +++ b/src/lang/en.json @@ -9,7 +9,7 @@ "areyousure": "Are you sure?", "back": "Back", "cancel": "Cancel", - "cannotconnect": "Cannot connect: Verify that you have correctly typed the URL and that your site uses Moodle 2.4 or later.", + "cannotconnect": "Cannot connect: verify that you have typed correctly the URL.", "cannotdownloadfiles": "File downloading is disabled. Please contact your site administrator.", "captureaudio": "Record audio", "capturedimage": "Taken picture.", diff --git a/src/providers/sites.ts b/src/providers/sites.ts index 8ceaffac4..ee31c0127 100644 --- a/src/providers/sites.ts +++ b/src/providers/sites.ts @@ -21,6 +21,7 @@ import { CoreLoggerProvider } from './logger'; import { CoreSitesFactoryProvider } from './sites-factory'; import { CoreTextUtilsProvider } from './utils/text'; import { CoreUrlUtilsProvider } from './utils/url'; +import { CoreUtilsProvider } from './utils/utils'; import { CoreConstants } from '@core/constants'; import { CoreConfigConstants } from '../configconstants'; import { CoreSite } from '@classes/site'; @@ -211,7 +212,8 @@ export class CoreSitesProvider { constructor(logger: CoreLoggerProvider, private http: HttpClient, private sitesFactory: CoreSitesFactoryProvider, private appProvider: CoreAppProvider, private translate: TranslateService, private urlUtils: CoreUrlUtilsProvider, - private eventsProvider: CoreEventsProvider, private textUtils: CoreTextUtilsProvider) { + private eventsProvider: CoreEventsProvider, private textUtils: CoreTextUtilsProvider, + private utils: CoreUtilsProvider) { this.logger = logger.getInstance('CoreSitesProvider'); this.appDB = appProvider.getDB(); @@ -258,14 +260,10 @@ export class CoreSitesProvider { protocol = protocol == 'https://' ? 'http://' : 'https://'; return this.checkSiteWithProtocol(siteUrl, protocol).catch((secondError) => { - // Site doesn't exist. - if (secondError.error) { - return Promise.reject(secondError.error); - } else if (error.error) { - return Promise.reject(error.error); - } - - return Promise.reject(this.translate.instant('core.login.checksiteversion')); + // Site doesn't exist. Return the error message. + return Promise.reject(this.textUtils.getErrorMessageFromError(error) || + this.textUtils.getErrorMessageFromError(secondError) || + this.translate.instant('core.cannotconnect')); }); }); } @@ -286,7 +284,7 @@ export class CoreSitesProvider { return this.siteExists(siteUrl).catch((error) => { // Do not continue checking if WS are not enabled. - if (error.errorcode && error.errorcode == 'enablewsdescription') { + if (error.errorcode == 'enablewsdescription') { return rejectWithCriticalError(error.error, error.errorcode); } @@ -298,13 +296,13 @@ export class CoreSitesProvider { siteUrl = treatedUrl; }).catch((secondError) => { // Do not continue checking if WS are not enabled. - if (secondError.errorcode && secondError.errorcode == 'enablewsdescription') { + if (secondError.errorcode == 'enablewsdescription') { return rejectWithCriticalError(secondError.error, secondError.errorcode); } - error = secondError || error; - - return Promise.reject({ error: typeof error == 'object' ? error.error : error }); + // Return the error message. + return Promise.reject(this.textUtils.getErrorMessageFromError(error) || + this.textUtils.getErrorMessageFromError(secondError)); }); }).then(() => { // Create a temporary site to check if local_mobile is installed. @@ -385,11 +383,11 @@ export class CoreSitesProvider { data.service = 'c'; } - const promise = this.http.post(siteUrl + '/login/token.php', data).timeout(CoreConstants.WS_TIMEOUT).toPromise(); - - return promise.catch((error) => { - return Promise.reject(error); + return this.http.post(siteUrl + '/login/token.php', data).timeout(CoreConstants.WS_TIMEOUT).toPromise().catch(() => { + // Default error messages are kinda bad, return our own message. + return Promise.reject({error: this.translate.instant('core.cannotconnect')}); }).then((data: any) => { + if (data.errorcode && (data.errorcode == 'enablewsdescription' || data.errorcode == 'requirecorrectaccess')) { return Promise.reject({ errorcode: data.errorcode, error: data.error }); } else if (data.error && data.error == 'Web services must be enabled in Advanced features.') { diff --git a/src/providers/utils/text.ts b/src/providers/utils/text.ts index 7ae4cecbf..668d3589d 100644 --- a/src/providers/utils/text.ts +++ b/src/providers/utils/text.ts @@ -407,6 +407,10 @@ export class CoreTextUtilsProvider { * @return {string} Error message, undefined if not found. */ getErrorMessageFromError(error: any): string { + if (typeof error == 'string') { + return error; + } + return error && (error.message || error.error || error.content || error.body); } From e5b013f2039fcebb1d5fc7c62ce3c5ac490dbce7 Mon Sep 17 00:00:00 2001 From: dpalou Date: Tue, 16 Oct 2018 11:39:48 +0200 Subject: [PATCH 2/2] MOBILE-2553 login: Detect redirect if login fails --- scripts/langindex.json | 1 + src/core/login/lang/en.json | 1 + src/providers/sites.ts | 13 +++++++- src/providers/utils/utils.ts | 58 ++++++++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 1 deletion(-) diff --git a/scripts/langindex.json b/scripts/langindex.json index ba9376e82..d345bb292 100644 --- a/scripts/langindex.json +++ b/scripts/langindex.json @@ -1349,6 +1349,7 @@ "core.login.selectsite": "local_moodlemobileapp", "core.login.signupplugindisabled": "local_moodlemobileapp", "core.login.siteaddress": "local_moodlemobileapp", + "core.login.sitehasredirect": "local_moodlemobileapp", "core.login.siteinmaintenance": "local_moodlemobileapp", "core.login.sitepolicynotagreederror": "local_moodlemobileapp", "core.login.siteurl": "local_moodlemobileapp", diff --git a/src/core/login/lang/en.json b/src/core/login/lang/en.json index bb2d4f4a6..ec2e9c845 100644 --- a/src/core/login/lang/en.json +++ b/src/core/login/lang/en.json @@ -71,6 +71,7 @@ "selectsite": "Please select your site:", "signupplugindisabled": "{{$a}} is not enabled.", "siteaddress": "Site address", + "sitehasredirect": "Your site contains at least one HTTP redirect. The app cannot follow redirects, this could be the issue that's preventing the app from connecting to your site.", "siteinmaintenance": "Your site is in maintenance mode", "sitepolicynotagreederror": "Site policy not agreed.", "siteurl": "Site URL", diff --git a/src/providers/sites.ts b/src/providers/sites.ts index ee31c0127..89e8bfe5b 100644 --- a/src/providers/sites.ts +++ b/src/providers/sites.ts @@ -422,7 +422,8 @@ export class CoreSitesProvider { password: password, service: service }, - promise = this.http.post(siteUrl + '/login/token.php', params).timeout(CoreConstants.WS_TIMEOUT).toPromise(); + loginUrl = siteUrl + '/login/token.php', + promise = this.http.post(loginUrl, params).timeout(CoreConstants.WS_TIMEOUT).toPromise(); return promise.then((data: any): any => { if (typeof data == 'undefined') { @@ -431,12 +432,22 @@ export class CoreSitesProvider { if (typeof data.token != 'undefined') { return { token: data.token, siteUrl: siteUrl, privateToken: data.privatetoken }; } else { + if (typeof data.error != 'undefined') { // We only allow one retry (to avoid loops). if (!retry && data.errorcode == 'requirecorrectaccess') { siteUrl = this.urlUtils.addOrRemoveWWW(siteUrl); return this.getUserToken(siteUrl, username, password, service, true); + } else if (data.errorcode == 'missingparam') { + // It seems the server didn't receive all required params, it could be due to a redirect. + return this.utils.checkRedirect(loginUrl).then((redirect) => { + if (redirect) { + return Promise.reject({ error: this.translate.instant('core.login.sitehasredirect') }); + } else { + return Promise.reject({ error: data.error, errorcode: data.errorcode }); + } + }); } else if (typeof data.errorcode != 'undefined') { return Promise.reject({ error: data.error, errorcode: data.errorcode }); } else { diff --git a/src/providers/utils/utils.ts b/src/providers/utils/utils.ts index ad37890cf..85b0bfbd5 100644 --- a/src/providers/utils/utils.ts +++ b/src/providers/utils/utils.ts @@ -26,6 +26,7 @@ import { CoreLoggerProvider } from '../logger'; import { TranslateService } from '@ngx-translate/core'; import { CoreLangProvider } from '../lang'; import { CoreWSProvider, CoreWSError } from '../ws'; +import { CoreConstants } from '@core/constants'; /** * Deferred promise. It's similar to the result of $q.defer() in AngularJS. @@ -182,6 +183,43 @@ export class CoreUtilsProvider { return; } + /** + * Check if a URL has a redirect. + * + * @param {string} url The URL to check. + * @return {Promise} Promise resolved with boolean_ whether there is a redirect. + */ + checkRedirect(url: string): Promise { + if (window.fetch) { + const win = window, // Convert to to be able to use AbortController (not supported by our TS version). + initOptions: any = { + redirect: 'follow' + }; + let controller; + + // Some browsers implement fetch but no AbortController. + if (win.AbortController) { + controller = new win.AbortController(); + initOptions.signal = controller.signal; + } + + return this.timeoutPromise(window.fetch(url, initOptions), CoreConstants.WS_TIMEOUT).then((response: Response) => { + return response.redirected; + }).catch((error) => { + if (error.timeout && controller) { + // Timeout, abort the request. + controller.abort(); + } + + // There was a timeout, cannot determine if there's a redirect. Assume it's false. + return false; + }); + } else { + // Cannot check if there is a redirect, assume it's false. + return Promise.resolve(false); + } + } + /** * Close the InAppBrowser window. * @@ -1082,6 +1120,26 @@ export class CoreUtilsProvider { return result; } + /** + * Set a timeout to a Promise. If the time passes before the Promise is resolved or rejected, it will be automatically + * rejected. + * + * @param {Promise} promise The promise to timeout. + * @param {number} time Number of milliseconds of the timeout. + * @return {Promise} Promise with the timeout. + */ + timeoutPromise(promise: Promise, time: number): Promise { + return new Promise((resolve, reject): void => { + const timeout = setTimeout(() => { + reject({timeout: true}); + }, time); + + promise.then(resolve).catch(reject).finally(() => { + clearTimeout(timeout); + }); + }); + } + /** * Converts locale specific floating point/comma number back to standard PHP float value. * Do NOT try to do any math operations before this conversion on any user submitted floats!