From d07756d68d1b1797507333ee9be34b5d90c65663 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 2 Nov 2015 03:22:37 -0500 Subject: [PATCH] Store API key in login manager, and add temp field in prefs --- .../zotero/preferences/preferences_sync.js | 12 ++- .../zotero/preferences/preferences_sync.xul | 20 +++- chrome/content/zotero/xpcom/sync/syncLocal.js | 94 ++++++++++++------- .../content/zotero/xpcom/sync/syncRunner.js | 55 +++++++++-- test/tests/syncLocalTest.js | 19 ++++ test/tests/syncRunnerTest.js | 32 +++---- 6 files changed, 167 insertions(+), 65 deletions(-) diff --git a/chrome/content/zotero/preferences/preferences_sync.js b/chrome/content/zotero/preferences/preferences_sync.js index 1b5f5fe04..40eeb3bb5 100644 --- a/chrome/content/zotero/preferences/preferences_sync.js +++ b/chrome/content/zotero/preferences/preferences_sync.js @@ -29,11 +29,13 @@ Zotero_Preferences.Sync = { init: function () { this.updateStorageSettings(null, null, true); - document.getElementById('sync-password').value = Zotero.Sync.Server.password; - var pass = Zotero.Sync.Storage.WebDAV.password; - if (pass) { - document.getElementById('storage-password').value = pass; - } + document.getElementById('sync-api-key').value = Zotero.Sync.Data.Local.getAPIKey(); + + // TEMP: Disabled + //var pass = Zotero.Sync.Storage.WebDAV.password; + //if (pass) { + // document.getElementById('storage-password').value = pass; + //} }, updateStorageSettings: function (enabled, protocol, skipWarnings) { diff --git a/chrome/content/zotero/preferences/preferences_sync.xul b/chrome/content/zotero/preferences/preferences_sync.xul index 928757764..8b79a81a4 100644 --- a/chrome/content/zotero/preferences/preferences_sync.xul +++ b/chrome/content/zotero/preferences/preferences_sync.xul @@ -63,6 +63,7 @@ + + + - + + - + tooltiptext="&zotero.preferences.sync.syncFullTextContent.desc;"/>--> + diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 32e8478df..fa435ab44 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -28,7 +28,7 @@ if (!Zotero.Sync.Data) { } Zotero.Sync.Data.Local = { - _loginManagerHost: 'https://api.zotero.org', + _loginManagerHost: 'chrome://zotero', _loginManagerRealm: 'Zotero Web API', _lastSyncTime: null, _lastClassicSyncTime: null, @@ -42,41 +42,25 @@ Zotero.Sync.Data.Local = { getAPIKey: function () { - var apiKey = Zotero.Prefs.get('devAPIKey'); - if (apiKey) { - return apiKey; - } - var loginManager = Components.classes["@mozilla.org/login-manager;1"] - .getService(Components.interfaces.nsILoginManager); - var logins = loginManager.findLogins( - {}, this._loginManagerHost, null, this._loginManagerRealm - ); - // Get API from returned array of nsILoginInfo objects - if (logins.length) { - return logins[0].password; - } - if (!apiKey) { - let username = Zotero.Prefs.get('sync.server.username'); - if (username) { - let password = Zotero.Sync.Data.Local.getLegacyPassword(username); - if (!password) { - return false; - } - throw new Error("Unimplemented"); - // Get API key from server - - // Store API key - - // Remove old logins and username pref - } - } - return apiKey; + Zotero.debug("Getting API key"); + var login = this._getAPIKeyLoginInfo(); + return login ? login.password : ""; }, setAPIKey: function (apiKey) { var loginManager = Components.classes["@mozilla.org/login-manager;1"] .getService(Components.interfaces.nsILoginManager); + + var oldLoginInfo = this._getAPIKeyLoginInfo(); + + // Clear old login + if (oldLoginInfo && (!apiKey || apiKey === "")) { + Zotero.debug("Clearing old API key"); + loginManager.removeLogin(oldLoginInfo); + return; + } + var nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1", Components.interfaces.nsILoginInfo, "init"); var loginInfo = new nsLoginInfo( @@ -85,10 +69,53 @@ Zotero.Sync.Data.Local = { this._loginManagerRealm, 'API Key', apiKey, - "", - "" + '', + '' ); - loginManager.addLogin(loginInfo); + if (!oldLoginInfo) { + Zotero.debug("Setting API key"); + loginManager.addLogin(loginInfo); + } + else { + Zotero.debug("Replacing API key"); + loginManager.modifyLogin(oldLoginInfo, loginInfo); + } + }, + + + /** + * @return {nsILoginInfo|false} + */ + _getAPIKeyLoginInfo: function () { + var loginManager = Components.classes["@mozilla.org/login-manager;1"] + .getService(Components.interfaces.nsILoginManager); + try { + var logins = loginManager.findLogins( + {}, + this._loginManagerHost, + null, + this._loginManagerRealm + ); + } + catch (e) { + Zotero.logError(e); + if (Zotero.isStandalone) { + var msg = Zotero.getString('sync.error.loginManagerCorrupted1', Zotero.appName) + "\n\n" + + Zotero.getString('sync.error.loginManagerCorrupted2', [Zotero.appName, Zotero.appName]); + } + else { + var msg = Zotero.getString('sync.error.loginManagerInaccessible') + "\n\n" + + Zotero.getString('sync.error.checkMasterPassword', Zotero.appName) + "\n\n" + + Zotero.getString('sync.error.corruptedLoginManager', Zotero.appName); + } + var ps = Components.classes["@mozilla.org/embedcomp/prompt-service;1"] + .getService(Components.interfaces.nsIPromptService); + ps.alert(null, Zotero.getString('general.error'), msg); + return false; + } + + // Get API from returned array of nsILoginInfo objects + return logins.length ? logins[0] : false; }, @@ -104,6 +131,7 @@ Zotero.Sync.Data.Local = { var logins = loginManager.findLogins({}, loginManagerHost, null, loginManagerRealm); } catch (e) { + Zotero.logError(e); return ''; } diff --git a/chrome/content/zotero/xpcom/sync/syncRunner.js b/chrome/content/zotero/xpcom/sync/syncRunner.js index a22654ec4..b86204a6e 100644 --- a/chrome/content/zotero/xpcom/sync/syncRunner.js +++ b/chrome/content/zotero/xpcom/sync/syncRunner.js @@ -38,7 +38,10 @@ Zotero.Sync.Runner_Module = function (options = {}) { this.baseURL = options.baseURL || ZOTERO_CONFIG.API_URL; this.apiVersion = options.apiVersion || ZOTERO_CONFIG.API_VERSION; - this.apiKey = options.apiKey || Zotero.Sync.Data.Local.getAPIKey(); + + // Allows tests to set apiKey in options or as property, overriding login manager + var _apiKey = options.apiKey; + Zotero.defineProperty(this, 'apiKey', { set: val => _apiKey = val }); Components.utils.import("resource://zotero/concurrentCaller.js"); this.caller = new ConcurrentCaller(4); @@ -66,11 +69,15 @@ Zotero.Sync.Runner_Module = function (options = {}) { var _errors = []; - this.getAPIClient = function () { + this.getAPIClient = function (options = {}) { + if (!options.apiKey) { + throw new Error("apiKey not provided"); + } + return new Zotero.Sync.APIClient({ baseURL: this.baseURL, apiVersion: this.apiVersion, - apiKey: this.apiKey, + apiKey: options.apiKey, caller: this.caller }); } @@ -112,7 +119,8 @@ Zotero.Sync.Runner_Module = function (options = {}) { // Purge deleted objects so they don't cause sync errors (e.g., long tags) yield Zotero.purgeDataObjects(true); - if (!this.apiKey) { + var apiKey = yield _getAPIKey(); + if (!apiKey) { let msg = "API key not set"; let e = new Zotero.Error(msg, 0, { dialogButtonText: null }) this.updateIcons(e); @@ -129,7 +137,7 @@ Zotero.Sync.Runner_Module = function (options = {}) { this.updateIcons('animate'); try { - let client = this.getAPIClient(); + let client = this.getAPIClient({ apiKey }); let keyInfo = yield this.checkAccess(client, options); if (!keyInfo) { @@ -603,13 +611,19 @@ Zotero.Sync.Runner_Module = function (options = {}) { return false; } + var apiKey = yield _getAPIKey(); + if (!apiKey) { + Zotero.debug("API key not set -- skipping download"); + return false; + } + // TEMP var options = {}; var itemID = item.id; var modeClass = Zotero.Sync.Storage.Local.getClassForLibrary(item.libraryID); var controller = new modeClass({ - apiClient: this.getAPIClient() + apiClient: this.getAPIClient({apiKey }) }); // TODO: verify WebDAV on-demand? @@ -1160,4 +1174,33 @@ Zotero.Sync.Runner_Module = function (options = {}) { _currentLastSyncLabel.value = Zotero.getString('sync.status.lastSync') + " " + msg; _currentLastSyncLabel.hidden = false; } + + + var _getAPIKey = Zotero.Promise.coroutine(function* () { + // Set as .apiKey on Runner in tests + return _apiKey + // Set in login manager + || Zotero.Sync.Data.Local.getAPIKey() + // Fallback to old username/password + || (yield _getAPIKeyFromLogin()); + }) + + + var _getAPIKeyFromLogin = Zotero.Promise.coroutine(function* () { + var apiKey = ""; + let username = Zotero.Prefs.get('sync.server.username'); + if (username) { + let password = Zotero.Sync.Data.Local.getLegacyPassword(username); + if (!password) { + return ""; + } + throw new Error("Unimplemented"); + // Get API key from server + + // Store API key + + // Remove old logins and username pref + } + return apiKey; + }) } diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js index 543ecb6c2..9b824e677 100644 --- a/test/tests/syncLocalTest.js +++ b/test/tests/syncLocalTest.js @@ -1,6 +1,25 @@ "use strict"; describe("Zotero.Sync.Data.Local", function() { + describe("#getAPIKey()/#setAPIKey()", function () { + it("should get and set an API key", function* () { + var apiKey1 = Zotero.Utilities.randomString(24); + var apiKey2 = Zotero.Utilities.randomString(24); + Zotero.Sync.Data.Local.setAPIKey(apiKey1); + assert.equal(Zotero.Sync.Data.Local.getAPIKey(apiKey1), apiKey1); + Zotero.Sync.Data.Local.setAPIKey(apiKey2); + assert.equal(Zotero.Sync.Data.Local.getAPIKey(apiKey2), apiKey2); + }) + + + it("should clear an API key by setting an empty string", function* () { + var apiKey = Zotero.Utilities.randomString(24); + Zotero.Sync.Data.Local.setAPIKey(apiKey); + Zotero.Sync.Data.Local.setAPIKey(""); + assert.strictEqual(Zotero.Sync.Data.Local.getAPIKey(apiKey), ""); + }) + }) + describe("#processSyncCacheForObjectType()", function () { var types = Zotero.DataObjectUtilities.getTypes(); diff --git a/test/tests/syncRunnerTest.js b/test/tests/syncRunnerTest.js index 4e725f618..8318655d4 100644 --- a/test/tests/syncRunnerTest.js +++ b/test/tests/syncRunnerTest.js @@ -109,10 +109,7 @@ describe("Zotero.Sync.Runner", function () { yield Zotero.DB.queryAsync("DELETE FROM settings WHERE setting='account'"); yield Zotero.Users.init(); - var runner = new Zotero.Sync.Runner_Module({ - baseURL: baseURL, - apiKey: apiKey - }); + var runner = new Zotero.Sync.Runner_Module({ baseURL, apiKey }); Components.utils.import("resource://zotero/concurrentCaller.js"); var caller = new ConcurrentCaller(1); @@ -172,7 +169,7 @@ describe("Zotero.Sync.Runner", function () { it("should check key access", function* () { spy = sinon.spy(runner, "checkUser"); setResponse('keyInfo.fullAccess'); - var json = yield runner.checkAccess(runner.getAPIClient()); + var json = yield runner.checkAccess(runner.getAPIClient({ apiKey })); sinon.assert.calledWith(spy, 1, "Username"); var compare = {}; Object.assign(compare, responses.keyInfo.fullAccess.json); @@ -208,7 +205,7 @@ describe("Zotero.Sync.Runner", function () { setResponse('userGroups.groupVersions'); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), false, responses.keyInfo.fullAccess.json + runner.getAPIClient({ apiKey }), false, responses.keyInfo.fullAccess.json ); assert.lengthOf(libraries, 4); assert.sameMembers( @@ -232,13 +229,16 @@ describe("Zotero.Sync.Runner", function () { setResponse('userGroups.groupVersions'); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), false, responses.keyInfo.fullAccess.json, [userLibraryID] + runner.getAPIClient({ apiKey }), + false, + responses.keyInfo.fullAccess.json, + [userLibraryID] ); assert.lengthOf(libraries, 1); assert.sameMembers(libraries, [userLibraryID]); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), + runner.getAPIClient({ apiKey }), false, responses.keyInfo.fullAccess.json, [userLibraryID, publicationsLibraryID] @@ -247,7 +247,7 @@ describe("Zotero.Sync.Runner", function () { assert.sameMembers(libraries, [userLibraryID, publicationsLibraryID]); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), + runner.getAPIClient({ apiKey }), false, responses.keyInfo.fullAccess.json, [group1.libraryID] @@ -275,7 +275,7 @@ describe("Zotero.Sync.Runner", function () { setResponse('groups.ownerGroup'); setResponse('groups.memberGroup'); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), false, responses.keyInfo.fullAccess.json + runner.getAPIClient({ apiKey }), false, responses.keyInfo.fullAccess.json ); assert.lengthOf(libraries, 4); assert.sameMembers( @@ -316,7 +316,7 @@ describe("Zotero.Sync.Runner", function () { setResponse('groups.ownerGroup'); setResponse('groups.memberGroup'); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), + runner.getAPIClient({ apiKey }), false, responses.keyInfo.fullAccess.json, [group1.libraryID, group2.libraryID] @@ -337,7 +337,7 @@ describe("Zotero.Sync.Runner", function () { setResponse('groups.ownerGroup'); setResponse('groups.memberGroup'); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), false, responses.keyInfo.fullAccess.json + runner.getAPIClient({ apiKey }), false, responses.keyInfo.fullAccess.json ); assert.lengthOf(libraries, 4); var groupData1 = responses.groups.ownerGroup; @@ -368,7 +368,7 @@ describe("Zotero.Sync.Runner", function () { assert.include(text, group1.name); }); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), false, responses.keyInfo.fullAccess.json + runner.getAPIClient({ apiKey }), false, responses.keyInfo.fullAccess.json ); assert.lengthOf(libraries, 3); assert.sameMembers(libraries, [userLibraryID, publicationsLibraryID, group2.libraryID]); @@ -386,7 +386,7 @@ describe("Zotero.Sync.Runner", function () { assert.include(text, group.name); }, "extra1"); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), false, responses.keyInfo.fullAccess.json + runner.getAPIClient({ apiKey }), false, responses.keyInfo.fullAccess.json ); assert.lengthOf(libraries, 3); assert.sameMembers(libraries, [userLibraryID, publicationsLibraryID, group.libraryID]); @@ -403,7 +403,7 @@ describe("Zotero.Sync.Runner", function () { assert.include(text, group.name); }, "cancel"); var libraries = yield runner.checkLibraries( - runner.getAPIClient(), false, responses.keyInfo.fullAccess.json + runner.getAPIClient({ apiKey }), false, responses.keyInfo.fullAccess.json ); assert.lengthOf(libraries, 0); assert.isTrue(Zotero.Groups.exists(groupData.json.id)); @@ -632,8 +632,6 @@ describe("Zotero.Sync.Runner", function () { }); yield runner.sync({ - baseURL: baseURL, - apiKey: apiKey, onError: e => { throw e }, });