-
Notifications
You must be signed in to change notification settings - Fork 474
[New Feature] Using node-keytar to store github token in safe place #321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
language: node_js | ||
node_js: | ||
- "8" | ||
before_install: | ||
- sudo apt-get update | ||
- sudo apt-get install -y libsecret-1-dev |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,22 @@ $ git clone [email protected]:hackjutsu/Lepton.git | |
$ cd Lepton && npm i | ||
``` | ||
|
||
#### On Linux | ||
|
||
Currently this library uses `libsecret` so you may need to install it before running `npm install`. | ||
|
||
Depending on your distribution, you will need to run the following command: | ||
|
||
* Debian/Ubuntu: `sudo apt-get install libsecret-1-dev` | ||
* Red Hat-based: `sudo yum install libsecret-devel` | ||
* Arch Linux: `sudo pacman -S libsecret` | ||
|
||
Rebuild `node-keytar` (native node module) | ||
|
||
```bash | ||
$ npm run electron-rebuild | ||
``` | ||
|
||
### Client ID/Secret | ||
[Register your application](https://github.com/settings/applications/new), and put your client id and client secret in `./configs/account.js`. | ||
```js | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,9 @@ import { Provider } from 'react-redux' | |
import { createStore, applyMiddleware } from 'redux' | ||
import thunk from 'redux-thunk' | ||
import electronLocalStorage from 'electron-json-storage-sync' | ||
import { Promise } from 'bluebird' | ||
import keytar from 'keytar' | ||
import Async from 'react-promise' | ||
|
||
import './utilities/vendor/bootstrap/css/bootstrap.css' | ||
import AppContainer from './containers/appContainer' | ||
|
@@ -69,7 +72,8 @@ const localPref = new Store({ | |
const CONFIG_OPTIONS = { | ||
client_id: Account.client_id, | ||
client_secret: Account.client_secret, | ||
scopes: ['gist'] | ||
scopes: ['gist'], | ||
keyChainService: 'Lepton Github Token' | ||
} | ||
|
||
let preSyncSnapshot = { | ||
|
@@ -385,17 +389,18 @@ function initUserSession (token) { | |
token: token, | ||
profile: newProfile.login, | ||
image: newProfile.avatar_url | ||
}) | ||
logger.debug('-----> after updateLocalStorage') | ||
}).then(() => { | ||
logger.debug('-----> after updateLocalStorage') | ||
|
||
logger.debug('-----> before syncLocalPref') | ||
syncLocalPref(newProfile.login) | ||
logger.debug('-----> after syncLocalPref') | ||
logger.debug('-----> before syncLocalPref') | ||
syncLocalPref(newProfile.login) | ||
logger.debug('-----> after syncLocalPref') | ||
|
||
remote.getCurrentWindow().setTitle(`${ newProfile.login } | Lepton`) // update the app title | ||
remote.getCurrentWindow().setTitle(`${ newProfile.login } | Lepton`) // update the app title | ||
|
||
logger.info('[Dispatch] updateUserSession ACTIVE') | ||
reduxStore.dispatch(updateUserSession({ activeStatus: 'ACTIVE', profile: newProfile })) | ||
logger.info('[Dispatch] updateUserSession ACTIVE') | ||
reduxStore.dispatch(updateUserSession({ activeStatus: 'ACTIVE', profile: newProfile })) | ||
}) | ||
}) | ||
.catch((err) => { | ||
logger.debug('-----> Failure with ' + JSON.stringify(err)) | ||
|
@@ -415,22 +420,28 @@ function initUserSession (token) { | |
|
||
/** Start: Local storage management **/ | ||
function updateLocalStorage (data) { | ||
try { | ||
logger.debug(`-----> Caching token ${data.token}`) | ||
let rst = electronLocalStorage.set('token', data.token) | ||
logger.debug(`-----> [${rst.status}] Cached token ${data.token}`) | ||
|
||
logger.debug(`-----> Caching profile ${data.profile}`) | ||
rst = electronLocalStorage.set('profile', data.profile) | ||
logger.debug(`-----> [${rst.status}] Cached profile ${data.profile}`) | ||
|
||
logger.debug(`-----> Caching image ${data.image}`) | ||
downloadImage(data.image, data.profile) | ||
|
||
logger.debug(`-----> User info is cached.`) | ||
} catch (e) { | ||
logger.error(`-----> Failed to cache user info. ${JSON.stringify(e)}`) | ||
} | ||
return new Promise((resolve, reject) => { | ||
try { | ||
logger.debug(`-----> Caching token ${data.token} in keychain`) | ||
keytar.setPassword(CONFIG_OPTIONS.keyChainService, data.profile, data.token).then(() => { | ||
logger.debug(`-----> Cached token in keychain`) | ||
|
||
logger.debug(`-----> Caching profile ${data.profile}`) | ||
let rst = electronLocalStorage.set('profile', data.profile) | ||
logger.debug(`-----> [${rst.status}] Cached profile ${data.profile}`) | ||
|
||
logger.debug(`-----> Caching image ${data.image}`) | ||
downloadImage(data.image, data.profile) | ||
|
||
logger.debug(`-----> User info is cached.`) | ||
|
||
resolve() | ||
}) | ||
} catch (e) { | ||
logger.error(`-----> Failed to cache user info. ${JSON.stringify(e)}`) | ||
reject(e) | ||
} | ||
}) | ||
} | ||
|
||
function downloadImage (imageUrl, filename) { | ||
|
@@ -461,21 +472,31 @@ function downloadImage (imageUrl, filename) { | |
} | ||
|
||
function getCachedUserInfo () { | ||
logger.debug('-----> Inside getCachedUserInfo') | ||
const cachedProfile = electronLocalStorage.get('profile') | ||
logger.debug(`-----> [${cachedProfile.status}] cachedProfile is ${cachedProfile.data}`) | ||
const cachedToken = electronLocalStorage.get('token') | ||
logger.debug(`-----> [${cachedToken.status}] cachedToken is ${cachedToken.data}`) | ||
|
||
if (cachedProfile.status && cachedToken.status) { | ||
return { | ||
token: cachedToken.data, | ||
profile: cachedProfile.data, | ||
image: electronLocalStorage.get('image').data | ||
return new Promise((resolve, reject) => { | ||
logger.debug('-----> Inside getCachedUserInfo') | ||
const cachedProfile = electronLocalStorage.get('profile') | ||
logger.debug(`-----> [${cachedProfile.status}] cachedProfile is ${cachedProfile.data}`) | ||
const cachedImage = electronLocalStorage.get('image') | ||
logger.debug(`-----> [${cachedImage.status}] cachedImage is ${cachedImage.data}`) | ||
|
||
if (cachedProfile.status && cachedImage.status) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only the profile cache and token cache are required for launching. The image cache is optional. Given there is a relatively higher failure rate for image caching, this check will propagate this higher failure rate to the app launching. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yah, I pushed a commit to remove |
||
keytar.getPassword(CONFIG_OPTIONS.keyChainService, cachedProfile.data).then((token, err) => { | ||
logger.debug(`-----> [${err}] cachedToken is ${token}`) | ||
|
||
if (err) { | ||
return resolve(null) | ||
} | ||
|
||
return resolve({ | ||
token: token, | ||
profile: cachedProfile.data, | ||
image: cachedImage.data | ||
}) | ||
}) | ||
} else { | ||
return resolve(null) | ||
} | ||
} | ||
|
||
return null | ||
}) | ||
} | ||
|
||
function syncLocalPref (userName) { | ||
|
@@ -692,17 +713,21 @@ const reduxStore = createStore( | |
) | ||
|
||
ReactDom.render( | ||
<Provider store={ reduxStore }> | ||
<AppContainer | ||
searchIndex = { SearchIndex } | ||
localPref = { localPref } | ||
updateLocalStorage = { updateLocalStorage } | ||
loggedInUserInfo = { getCachedUserInfo() } | ||
launchAuthWindow = { launchAuthWindow } | ||
reSyncUserGists = { reSyncUserGists } | ||
updateAboutModalStatus = { updateAboutModalStatus } | ||
updateDashboardModalStatus = { updateDashboardModalStatus } | ||
updateActiveGistAfterClicked = { updateActiveGistAfterClicked } /> | ||
</Provider>, | ||
<Async | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use regular There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I'm not good at JS so I don't get it. Could you teach me why using regular |
||
promise={ getCachedUserInfo() } | ||
then = {cachedUserInfo => | ||
<Provider store={ reduxStore }> | ||
<AppContainer | ||
searchIndex = { SearchIndex } | ||
localPref = { localPref } | ||
updateLocalStorage = { updateLocalStorage } | ||
loggedInUserInfo = { cachedUserInfo } | ||
launchAuthWindow = { launchAuthWindow } | ||
reSyncUserGists = { reSyncUserGists } | ||
updateAboutModalStatus = { updateAboutModalStatus } | ||
updateDashboardModalStatus = { updateDashboardModalStatus } | ||
updateActiveGistAfterClicked = { updateActiveGistAfterClicked } /> | ||
</Provider> | ||
} />, | ||
document.getElementById('container') | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid treating Linux as a special case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSes use different things to store secret so
node-keytar
(a native node module) have to deal with this issue.From what I know, we have to rebuild native node modules b/c some pre-build libs won't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining:) I'm hesitant to add extra build steps for particular platforms, especially introducing external dependency by installing prerequisites. This often (significantly) increases the maintenance cost by making it hard to dig out or reproduce bugs, especially when the bug is related to the launch step. Maybe this module is not ready for integration at this moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, you're right. Using this module can increase failure rate and it's not safe as we want (atom/node-keytar#88)