Fix: less intrusive authorization (#1172)

* Update db-to-cloud

* Change: refactor sync logic, disallow implicit auth

* Add: better relog message in options page

* read prefs only when `ready`
* show the internal error text in icon tooltip
* show the internal error text in options fully
Co-authored-by: tophf <tophf@gmx.com>

* Update _locales/en/messages.json
Co-authored-by: Enrico Lamperti <910672+elamperti@users.noreply.github.com>

_locales/en/messages.json

@@ -1141,6 +1141,9 @@
   "optionsSyncStatusDisconnected": {
     "message": "Disconnected"
   },
+  "optionsSyncStatusRelogin": {
+    "message": "Session expired, please login again."
+  },
   "paginationCurrent": {
     "message": "Current page",
     "description": "Tooltip for the current page index in search results"

background/sync-manager.js

@@ -63,19 +63,19 @@ const syncMan = (() => {
       return status;
     },
 
-    async login(name = prefs.get('sync.enabled')) {
+    async login(name) {
       if (ready.then) await ready;
+      if (!name) name = prefs.get('sync.enabled');
+      await tokenMan.revokeToken(name);
       try {
         await tokenMan.getToken(name, true);
+        status.login = true;
       } catch (err) {
-        if (/Authorization page could not be loaded/i.test(err.message)) {
+        status.login = false;
-          // FIXME: Chrome always fails at the first login so we try again
-          await tokenMan.getToken(name);
-        }
         throw err;
+      } finally {
+        emitStatusChange();
       }
-      status.login = true;
-      emitStatusChange();
     },
 
     async put(...args) {
@@ -88,29 +88,32 @@ const syncMan = (() => {
     async start(name, fromPref = false) {
       if (ready.then) await ready;
       if (!ctrl) await initController();
+
       if (currentDrive) return;
       currentDrive = getDrive(name);
       ctrl.use(currentDrive);
+
       status.state = STATES.connecting;
       status.currentDriveName = currentDrive.name;
-      status.login = true;
       emitStatusChange();
-      try {
+
-        if (!fromPref) {
+      if (fromPref) {
-          await syncMan.login(name).catch(handle401Error);
+        status.login = true;
-        }
+      } else {
-        await syncMan.syncNow();
+        try {
-        status.errorMessage = null;
+          await syncMan.login(name);
-        lastError = null;
+        } catch (err) {
-      } catch (err) {
-        status.errorMessage = err.message;
-        lastError = err;
-        // FIXME: should we move this logic to options.js?
-        if (!fromPref) {
           console.error(err);
+          status.errorMessage = err.message;
+          lastError = err;
+          emitStatusChange();
           return syncMan.stop();
         }
       }
+
+      await ctrl.init();
+
+      await syncMan.syncNow(name);
       prefs.set('sync.enabled', name);
       status.state = STATES.connected;
       schedule(SYNC_INTERVAL);
@@ -124,7 +127,7 @@ const syncMan = (() => {
       status.state = STATES.disconnecting;
       emitStatusChange();
       try {
-        await ctrl.stop();
+        await ctrl.uninit();
         await tokenMan.revokeToken(currentDrive.name);
         await chromeLocal.remove(STORAGE_KEY + currentDrive.name);
       } catch (e) {}
@@ -138,14 +141,21 @@ const syncMan = (() => {
 
     async syncNow() {
       if (ready.then) await ready;
-      if (!currentDrive) throw new Error('cannot sync when disconnected');
+      if (!currentDrive || !status.login) {
+        console.warn('cannot sync when disconnected');
+        return;
+      }
       try {
-        await (ctrl.isInit() ? ctrl.syncNow() : ctrl.start()).catch(handle401Error);
+        await ctrl.syncNow();
         status.errorMessage = null;
         lastError = null;
       } catch (err) {
         status.errorMessage = err.message;
         lastError = err;
+
+        if (isGrantError(err)) {
+          status.login = false;
+        }
       }
       emitStatusChange();
     },
@@ -192,21 +202,6 @@ const syncMan = (() => {
     });
   }
 
-  async function handle401Error(err) {
-    let authError = false;
-    if (err.code === 401) {
-      await tokenMan.revokeToken(currentDrive.name).catch(console.error);
-      authError = true;
-    } else if (/User interaction required|Requires user interaction/i.test(err.message)) {
-      authError = true;
-    }
-    if (authError) {
-      status.login = false;
-      emitStatusChange();
-    }
-    return Promise.reject(err);
-  }
-
   function emitStatusChange() {
     msg.broadcastExtension({method: 'syncStatusUpdate', status});
     iconMan.overrideBadge(getErrorBadge());
@@ -223,11 +218,17 @@ const syncMan = (() => {
   }
 
   function getErrorBadge() {
-    if (status.state === STATES.connected && lastError && !isNetworkError(lastError)) {
+    if (status.state === STATES.connected &&
+        (!status.login || lastError && !isNetworkError(lastError))) {
       return {
         text: 'x',
         color: '#F00',
-        title: isGrantError(lastError) ? 'syncErrorRelogin' : 'syncError',
+        title: !status.login ? 'syncErrorRelogin' : `${
+          chrome.i18n.getMessage('syncError')
+        }\n---------------------\n${
+          // splitting to limit each line length
+          lastError.message.replace(/.{60,}?\s(?=.{30,})/g, '$&\n')
+        }`,
       };
     }
   }

background/token-manager.js

@@ -75,13 +75,12 @@ const tokenMan = (() => {
           return obj[k.TOKEN];
         }
         if (obj[k.REFRESH]) {
-          try {
+          return refreshToken(name, k, obj);
-            return await refreshToken(name, k, obj);
-          } catch (err) {
-            if (err.code !== 401) throw err;
-          }
         }
       }
+      if (!interactive) {
+        throw new Error(`Invalid token: ${name}`);
+      }
       return authUser(name, k, interactive);
     },
 

options/options.css

@@ -398,12 +398,10 @@ html:not(.firefox):not(.opera) #updates {
 }
 
 .sync-status {
+  width: 0; /* together with flex-grow makes it reuse the current width */
   flex-grow: 1;
   padding-right: 8px;
   box-sizing: border-box;
-  white-space: nowrap;
-  overflow: hidden;
-  text-overflow: ellipsis;
 }
 .sync-status::first-letter {
   text-transform: uppercase;

options/options.js

@@ -147,7 +147,7 @@ document.onclick = e => {
       [elCloud, isDisconnected],
       [elStart, isDisconnected && elCloud.value !== 'none'],
       [elStop, isConnected && !status.syncing],
-      [elSyncNow, isConnected && !status.syncing],
+      [elSyncNow, isConnected && !status.syncing && status.login],
     ]) {
       el.disabled = !enable;
     }
@@ -156,19 +156,22 @@ document.onclick = e => {
   }
 
   function getStatusText() {
-    let res;
     if (status.syncing) {
       const {phase, loaded, total} = status.progress || {};
-      res = phase
+      return phase
         ? t(`optionsSyncStatus${capitalize(phase)}`, [loaded + 1, total], false) ||
           `${phase} ${loaded} / ${total}`
         : t('optionsSyncStatusSyncing');
-    } else {
-      const {state, errorMessage, STATES} = status;
-      res = (state === STATES.connected || state === STATES.disconnected) && errorMessage ||
-        t(`optionsSyncStatus${capitalize(state)}`, null, false) || state;
     }
-    return res;
+
+    const {state, errorMessage, STATES} = status;
+    if (errorMessage && (state === STATES.connected || state === STATES.disconnected)) {
+      return errorMessage;
+    }
+    if (state === STATES.connected && !status.login) {
+      return t('optionsSyncStatusRelogin');
+    }
+    return t(`optionsSyncStatus${capitalize(state)}`, null, false) || state;
   }
 })();
 

package-lock.json

@@ -10,7 +10,7 @@
       "license": "GPL-3.0-only",
       "dependencies": {
         "codemirror": "5.59.2",
-        "db-to-cloud": "^0.4.5",
+        "db-to-cloud": "^0.6.0",
         "jsonlint": "^1.6.3",
         "less-bundle": "github:openstyles/less-bundle#v0.1.0",
         "lz-string-unsafe": "^1.4.4-fork-1",
@@ -2772,12 +2772,15 @@
       }
     },
     "node_modules/db-to-cloud": {
-      "version": "0.4.5",
+      "version": "0.6.0",
-      "resolved": "https://registry.npmjs.org/db-to-cloud/-/db-to-cloud-0.4.5.tgz",
+      "resolved": "https://registry.npmjs.org/db-to-cloud/-/db-to-cloud-0.6.0.tgz",
-      "integrity": "sha512-3E5eYVIlZmX0ZRgSZ3WJF+lxs8eCFOJWruw8GLHbKDGK5tIZ13Bxsge+eFXbYBQUidzW7y3xuxD8MdpjDLY7eQ==",
+      "integrity": "sha512-AbvxpU+fA3Fsdzu0OxL+cVPS9HwM6DzXFDg00WIQ3YeMkWs5saMXpiXMfISlkpBUwm5Cbr4W7cfhYszu38BzSw==",
       "dependencies": {
         "@eight04/read-write-lock": "^0.1.0",
         "universal-base64": "^2.1.0"
+      },
+      "engines": {
+        "node": ">=8"
       }
     },
     "node_modules/debounce": {
@@ -14299,9 +14302,9 @@
       }
     },
     "db-to-cloud": {
-      "version": "0.4.5",
+      "version": "0.6.0",
-      "resolved": "https://registry.npmjs.org/db-to-cloud/-/db-to-cloud-0.4.5.tgz",
+      "resolved": "https://registry.npmjs.org/db-to-cloud/-/db-to-cloud-0.6.0.tgz",
-      "integrity": "sha512-3E5eYVIlZmX0ZRgSZ3WJF+lxs8eCFOJWruw8GLHbKDGK5tIZ13Bxsge+eFXbYBQUidzW7y3xuxD8MdpjDLY7eQ==",
+      "integrity": "sha512-AbvxpU+fA3Fsdzu0OxL+cVPS9HwM6DzXFDg00WIQ3YeMkWs5saMXpiXMfISlkpBUwm5Cbr4W7cfhYszu38BzSw==",
       "requires": {
         "@eight04/read-write-lock": "^0.1.0",
         "universal-base64": "^2.1.0"

package.json

@@ -7,7 +7,7 @@
   "author": "Stylus Team",
   "dependencies": {
     "codemirror": "5.59.2",
-    "db-to-cloud": "^0.4.5",
+    "db-to-cloud": "^0.6.0",
     "jsonlint": "^1.6.3",
     "less-bundle": "github:openstyles/less-bundle#v0.1.0",
     "lz-string-unsafe": "^1.4.4-fork-1",

vendor/db-to-cloud/README.md

@@ -1,4 +1,4 @@
-## db-to-cloud v0.4.5
+## db-to-cloud v0.6.0
 
 Following files are copied from npm (node_modules):