Skip to content
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.

Sign up for GitHub

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

[New Feature] Using node-keytar to store github token in safe place #321

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[New Feature] Using node-keytar to store github token in safe place #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d29a65a
using node-keytar to store github token in safe place
khanhicetea Oct 2, 2018
8ed94a8
style
khanhicetea Oct 2, 2018
0dbc718
fix CI
khanhicetea Oct 2, 2018
94534f3
remove cachedImage.status to reduce failure rate on launching
khanhicetea Oct 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .travis.yml
View file
Open in desktop
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
16 changes: 16 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,22 @@ $ git clone git@github.com: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
```

Copy link
Owner

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?

Copy link
Author

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.

Copy link
Owner

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.

Copy link
Author

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)

### 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
Expand Down
127 changes: 76 additions & 51 deletions app/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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))
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
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) {
Expand Down Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use regular Promise?

Copy link
Author

Choose a reason for hiding this comment

The 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 is better ? Thanks !

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')
)
Loading