From 4593d0a9b107081bcfe0e44f9f5ce79f378c6ad5 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Mon, 14 Mar 2016 15:14:08 -0700 Subject: [PATCH] fix(config): Move account tokens to system keychain Summary: This diff also adds an account version number to the config so that the AccountStore can tell whether it should reload accounts (depending on whether it was the instance making tthe changes.) This diff also fixes a tiny issue where un-opened composers threw an exception if you changed accounts. Test Plan: New tests Reviewers: evan, drew, juan Reviewed By: juan Subscribers: juan Differential Revision: https://phab.nylas.com/D2726 --- build/resources/linux/debian/control.in | 3 +- build/resources/linux/redhat/nylas.spec.in | 4 +- .../composer/lib/composer-view.cjsx | 1 + package.json | 1 + spec/stores/account-store-spec.coffee | 102 ++++++++++++------ src/flux/actions.coffee | 2 +- src/flux/stores/account-store.coffee | 76 +++++++------ 7 files changed, 120 insertions(+), 69 deletions(-) diff --git a/build/resources/linux/debian/control.in b/build/resources/linux/debian/control.in index fb623507a..18ba612e6 100644 --- a/build/resources/linux/debian/control.in +++ b/build/resources/linux/debian/control.in @@ -1,7 +1,6 @@ Package: <%= name %> Version: <%= version %> -Depends: git, gconf2, gconf-service, libgtk2.0-0, libudev0 | libudev1, libgcrypt11 | libgcrypt20, libnotify4, libxtst6, libnss3, python, gvfs-bin, xdg-utils -Suggests: libgnome-keyring0, gir1.2-gnomekeyring-1.0 +Depends: libgnome-keyring0, gir1.2-gnomekeyring-1.0, git, gconf2, gconf-service, libgtk2.0-0, libudev0 | libudev1, libgcrypt11 | libgcrypt20, libnotify4, libxtst6, libnss3, python, gvfs-bin, xdg-utils Section: <%= section %> Priority: optional Architecture: <%= arch %> diff --git a/build/resources/linux/redhat/nylas.spec.in b/build/resources/linux/redhat/nylas.spec.in index d4a228063..3302012d5 100644 --- a/build/resources/linux/redhat/nylas.spec.in +++ b/build/resources/linux/redhat/nylas.spec.in @@ -2,10 +2,12 @@ Name: <%= name %> Version: <%= version %> Release: 0.1%{?dist} Summary: <%= description %> -License: Proprietary +License: GPLv3 URL: https://nylas.com/N1 AutoReqProv: no # Avoid libchromiumcontent.so missing dependency +requires: libgnome-keyring0, gir1.2-gnomekeyring-1.0 + %description <%= description %> diff --git a/internal_packages/composer/lib/composer-view.cjsx b/internal_packages/composer/lib/composer-view.cjsx index e9fb78244..6237c98a1 100644 --- a/internal_packages/composer/lib/composer-view.cjsx +++ b/internal_packages/composer/lib/composer-view.cjsx @@ -561,6 +561,7 @@ class ComposerView extends React.Component # When the account store changes, the From field may or may not still # be in scope. We need to make sure to update our enabled fields. _onAccountStoreChanged: => + return unless @_proxy accounts = @_getAccountsForSend() enabledFields = if @_shouldShowFromField(@_proxy?.draft()) @state.enabledFields.concat [Fields.From] diff --git a/package.json b/package.json index b75d44a4e..6af5680fb 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,7 @@ "jasmine-tagged": "^1.1.2", "jquery": "^2.1.1", "juice": "^1.4", + "keytar": "^3.0.0", "less-cache": "0.21", "marked": "^0.3", "mkdirp": "^0.5", diff --git a/spec/stores/account-store-spec.coffee b/spec/stores/account-store-spec.coffee index 5709b9a43..e32ea1529 100644 --- a/spec/stores/account-store-spec.coffee +++ b/spec/stores/account-store-spec.coffee @@ -1,4 +1,5 @@ _ = require 'underscore' +keytar = require 'keytar' AccountStore = require '../../src/flux/stores/account-store' Account = require '../../src/flux/models/account' Actions = require '../../src/flux/actions' @@ -7,37 +8,71 @@ describe "AccountStore", -> beforeEach -> @instance = null @constructor = AccountStore.constructor + @keys = {} + spyOn(keytar, 'getPassword').andCallFake (service, account) => + @keys[account] + spyOn(keytar, 'deletePassword').andCallFake (service, account) => + delete @keys[account] + spyOn(keytar, 'replacePassword').andCallFake (service, account, pass) => + @keys[account] = pass afterEach -> @instance.stopListeningToAll() - it "should initialize using data saved in config", -> - accounts = - [{ - "id": "123", - "client_id" : 'local-4f9d476a-c173', - "server_id" : '123', - "email_address":"bengotow@gmail.com", - "object":"account" - "organization_unit": "label" - },{ - "id": "1234", - "client_id" : 'local-4f9d476a-c175', - "server_id" : '1234', - "email_address":"ben@nylas.com", - "object":"account" - "organization_unit": "label" - }] + describe "initialization", -> + beforeEach -> + @configTokens = null + @configVersion = 1 + @configAccounts = + [{ + "id": "A", + "client_id" : 'local-4f9d476a-c173', + "server_id" : 'A', + "email_address":"bengotow@gmail.com", + "object":"account" + "organization_unit": "label" + },{ + "id": "B", + "client_id" : 'local-4f9d476a-c175', + "server_id" : 'B', + "email_address":"ben@nylas.com", + "object":"account" + "organization_unit": "label" + }] - spyOn(NylasEnv.config, 'get').andCallFake (key) -> - return accounts if key is 'nylas.accounts' - return null - @instance = new @constructor + spyOn(NylasEnv.config, 'set') + spyOn(NylasEnv.config, 'get').andCallFake (key) => + return @configAccounts if key is 'nylas.accounts' + return @configVersion if key is 'nylas.accountsVersion' + return @configTokens if key is 'nylas.accountTokens' + return null - expect(@instance.accounts()).toEqual([ - (new Account).fromJSON(accounts[0]), - (new Account).fromJSON(accounts[1]) - ]) + it "should initialize the accounts and version from config", -> + @instance = new @constructor + expect(@instance._version).toEqual(@configVersion) + expect(@instance.accounts()).toEqual([ + (new Account).fromJSON(@configAccounts[0]), + (new Account).fromJSON(@configAccounts[1]) + ]) + + it "should initialize tokens from config, if present, save them to keytar, and remove them from config", -> + @configTokens = {'A': 'A-TOKEN'} + @instance = new @constructor + expect(@instance.tokenForAccountId('A')).toEqual('A-TOKEN') + expect(@instance.tokenForAccountId('B')).toEqual(undefined) + expect(keytar.replacePassword).toHaveBeenCalledWith('Nylas', 'bengotow@gmail.com', 'A-TOKEN') + expect(NylasEnv.config.set).toHaveBeenCalledWith('nylas.accountTokens', null) + + it "should initialize tokens from keytar", -> + @configTokens = null + jasmine.unspy(keytar, 'getPassword') + spyOn(keytar, 'getPassword').andCallFake (service, account) => + return 'A-TOKEN' if account is 'bengotow@gmail.com' + return 'B-TOKEN' if account is 'ben@nylas.com' + return null + @instance = new @constructor + expect(@instance.tokenForAccountId('A')).toEqual('A-TOKEN') + expect(@instance.tokenForAccountId('B')).toEqual('B-TOKEN') describe "accountForEmail", -> beforeEach -> @@ -58,33 +93,32 @@ describe "AccountStore", -> beforeEach -> spyOn(NylasEnv.config, "set") @json = - "id": "1234", + "id": "B", "client_id" : 'local-4f9d476a-c175', - "server_id" : '1234', + "server_id" : 'B', "email_address":"ben@nylas.com", "provider":"gmail", "object":"account" - "auth_token": "auth-123" + "auth_token": "B-NEW-TOKEN" "organization_unit": "label" @instance = new @constructor spyOn(Actions, 'focusDefaultMailboxPerspectiveForAccounts') spyOn(@instance, "trigger") @instance.addAccountFromJSON(@json) - it "sets the tokens", -> - expect(@instance._tokens["1234"]).toBe "auth-123" + it "saves the token to keytar and to the loaded tokens cache", -> + expect(@instance._tokens["B"]).toBe("B-NEW-TOKEN") + expect(keytar.replacePassword).toHaveBeenCalledWith("Nylas", "ben@nylas.com", "B-NEW-TOKEN") - it "sets the accounts", -> + it "saves the account to the accounts cache and saves", -> account = (new Account).fromJSON(@json) expect(@instance._accounts.length).toBe 1 expect(@instance._accounts[0]).toEqual account - - it "saves the config", -> expect(NylasEnv.config.save).toHaveBeenCalled() expect(NylasEnv.config.set.calls.length).toBe 2 it "selects the account", -> - expect(Actions.focusDefaultMailboxPerspectiveForAccounts).toHaveBeenCalledWith(["1234"]) + expect(Actions.focusDefaultMailboxPerspectiveForAccounts).toHaveBeenCalledWith(["B"]) it "triggers", -> expect(@instance.trigger).toHaveBeenCalled() diff --git a/src/flux/actions.coffee b/src/flux/actions.coffee index 2f39b49a7..fa667efb4 100644 --- a/src/flux/actions.coffee +++ b/src/flux/actions.coffee @@ -171,7 +171,7 @@ class Actions Actions.updateAccount(account.id, {accountName: 'new'}) ``` ### - @updateAccount: ActionScopeGlobal + @updateAccount: ActionScopeWindow ### Public: Re-order the provided account in the account list. diff --git a/src/flux/stores/account-store.coffee b/src/flux/stores/account-store.coffee index 448658320..68ec849e7 100644 --- a/src/flux/stores/account-store.coffee +++ b/src/flux/stores/account-store.coffee @@ -4,10 +4,12 @@ Actions = require '../actions' Account = require '../models/account' Utils = require '../models/utils' DatabaseStore = require './database-store' +keytar = require 'keytar' - -saveObjectsKey = "nylas.accounts" -saveTokensKey = "nylas.accountTokens" +configAccountsKey = "nylas.accounts" +configVersionKey = "nylas.accountsVersion" +configTokensKey = "nylas.accountTokens" +keytarServiceName = 'Nylas' ### Public: The AccountStore listens to changes to the available accounts in @@ -18,31 +20,45 @@ Section: Stores class AccountStore extends NylasStore constructor: -> - @_load() + @_loadAccounts() @listenTo Actions.removeAccount, @_onRemoveAccount @listenTo Actions.updateAccount, @_onUpdateAccount @listenTo Actions.reorderAccount, @_onReorderAccount - @_caches = {} + NylasEnv.config.onDidChange configVersionKey, (change) => + # If we already have this version of the accounts config, it means we + # are the ones who saved the change, and we don't need to reload. + return if @_version / 1 is change.newValue / 1 + oldAccountIds = _.pluck(@_accounts, 'id') + @_loadAccounts() + newAccountIds = _.pluck(@_accounts, 'id') + newAccountIds = _.without(newAccountIds, oldAccountIds) - NylasEnv.config.onDidChange saveTokensKey, (change) => - updatedTokens = change.newValue - return if _.isEqual(updatedTokens, @_tokens) - newAccountIds = _.keys(_.omit(updatedTokens, _.keys(@_tokens))) - @_load() if newAccountIds.length > 0 Actions.focusDefaultMailboxPerspectiveForAccounts([newAccountIds[0]]) - if NylasEnv.isComposerWindow() or NylasEnv.isWorkWindow() - NylasEnv.config.onDidChange saveObjectsKey, => @_load() - - _load: => + _loadAccounts: => @_caches = {} + @_version = NylasEnv.config.get(configVersionKey) || 0 + @_accounts = [] - for json in NylasEnv.config.get(saveObjectsKey) || [] + for json in NylasEnv.config.get(configAccountsKey) || [] @_accounts.push((new Account).fromJSON(json)) - @_tokens = NylasEnv.config.get(saveTokensKey) || {} + # Load tokens using the old config method and save them into the keychain + oldTokens = NylasEnv.config.get(configTokensKey) + if oldTokens + for key, val of oldTokens + account = @accountForId(key) + continue unless account + keytar.replacePassword(keytarServiceName, account.emailAddress, val) + NylasEnv.config.set(configTokensKey, null) + + # Load tokens using the new keytar method + @_tokens = {} + for account in @_accounts + @_tokens[account.id] = keytar.getPassword(keytarServiceName, account.emailAddress) + @_trigger() _trigger: -> @@ -54,9 +70,11 @@ class AccountStore extends NylasStore @trigger() _save: => - NylasEnv.config.set(saveObjectsKey, @_accounts) - NylasEnv.config.set(saveTokensKey, @_tokens) + @_version += 1 + NylasEnv.config.set(configVersionKey, @_version) + NylasEnv.config.set(configAccountsKey, @_accounts) NylasEnv.config.save() + @_trigger() # Inbound Events @@ -68,23 +86,21 @@ class AccountStore extends NylasStore account = _.extend(account, updated) @_caches = {} @_accounts[idx] = account - NylasEnv.config.set(saveObjectsKey, @_accounts) - @_trigger() + @_save() _onRemoveAccount: (id) => - idx = _.findIndex @_accounts, (a) -> a.id is id - return if idx is -1 + account = _.findWhere(@_accounts, {id}) + return unless account + keytar.deletePassword(keytarServiceName, account.emailAddress) - delete @_tokens[id] @_caches = {} - @_accounts.splice(idx, 1) + @_accounts = _.without(@_accounts, account) @_save() if @_accounts.length is 0 ipc = require('electron').ipcRenderer ipc.send('command', 'application:reset-config-and-relaunch') else - @_trigger() Actions.focusDefaultMailboxPerspectiveForAccounts(@_accounts) _onReorderAccount: (id, newIdx) => @@ -95,7 +111,6 @@ class AccountStore extends NylasStore @_accounts.splice(existingIdx, 1) @_accounts.splice(newIdx, 0, account) @_save() - @_trigger() addAccountFromJSON: (json) => if not json.email_address or not json.provider @@ -103,10 +118,11 @@ class AccountStore extends NylasStore console.log JSON.stringify(json) throw new Error("Returned account data is invalid") - @_load() - @_tokens[json.id] = json.auth_token + @_loadAccounts() + + @_tokens[json.id] = json.auth_token + keytar.replacePassword(keytarServiceName, json.email_address, json.auth_token) - @_caches = {} existingIdx = _.findIndex @_accounts, (a) -> a.id is json.id if existingIdx is -1 account = (new Account).fromJSON(json) @@ -116,8 +132,6 @@ class AccountStore extends NylasStore account.fromJSON(json) @_save() - - @_trigger() Actions.focusDefaultMailboxPerspectiveForAccounts([account.id]) # Exposed Data