Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

feat(app): basic password authentication #2860

Closed
wants to merge 12 commits into from

Conversation

mrfelton
Copy link
Member

@mrfelton mrfelton commented Sep 9, 2019

Description:

This PR adds basic password authentication for the app. The password is stored in plain text in the indexed database so it's not a strong protection, however it does provide a useful basic level of protection.

This is currently an opt-in feature that users can enable from the preferences page

Additional things we should consider:

  1. Store password in more secure way (eg, in the keychain)
  2. Require users to enter existing password in order to change password
  3. Enable password to grant access to all wallets
  4. Auto-lock app after a time limit

Motivation and Context:

Fix #1387

How Has This Been Tested?

Manually

Screenshots (if appropriate):

image

image

Types of changes:

Feature

Checklist:

  • My code follows the code style of this project.
  • I have reviewed and updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes where needed.
  • All new and existing tests passed.
  • My commits have been squashed into a concise set of changes.

@mrfelton mrfelton added this to the v0.6.0-beta milestone Sep 9, 2019
@mrfelton mrfelton added scope: security issues that are security related type: enhancement New feature or request labels Sep 9, 2019
@mrfelton mrfelton self-assigned this Sep 9, 2019
@mrfelton mrfelton changed the title [WIP] feat(app): basic password authentication feat(app): basic password authentication Sep 9, 2019
@coveralls
Copy link

coveralls commented Sep 9, 2019

Coverage Status

Coverage increased (+0.2%) to 23.128% when pulling ddabf58 on mrfelton:feat/password into ec0ec28 on LN-Zap:next.

Copy link
Member

@korhaliv korhaliv left a comment

Choose a reason for hiding this comment

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

Some feedback

renderer/components/Settings/SettingsFieldsSecurity.js Outdated Show resolved Hide resolved
renderer/containers/Login/Login.js Outdated Show resolved Hide resolved
renderer/containers/Root.js Outdated Show resolved Hide resolved
renderer/reducers/account.js Outdated Show resolved Hide resolved
renderer/components/Settings/SettingsForm.js Outdated Show resolved Hide resolved
renderer/components/Settings/SettingsPage.js Outdated Show resolved Hide resolved
renderer/components/Login/Login.js Outdated Show resolved Hide resolved
renderer/components/Settings/SettingsPage.js Outdated Show resolved Hide resolved
Copy link
Member

@korhaliv korhaliv left a comment

Choose a reason for hiding this comment

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

Some comments

renderer/components/Login/Login.js Outdated Show resolved Hide resolved
renderer/components/Login/Login.js Outdated Show resolved Hide resolved
renderer/components/Login/Login.js Outdated Show resolved Hide resolved
renderer/components/Settings/SettingsForm.js Show resolved Hide resolved
renderer/components/Settings/SettingsPage.js Outdated Show resolved Hide resolved
@mrfelton
Copy link
Member Author

Updated

@mrfelton mrfelton requested a review from korhaliv September 26, 2019 14:39
@korhaliv korhaliv added the status: needs rebase Issue needs to be rebased on top of the main branch label Sep 26, 2019
@korhaliv korhaliv assigned korhaliv and unassigned mrfelton Oct 1, 2019
@mrfelton
Copy link
Member Author

mrfelton commented Oct 7, 2019

Closing in favour of #2940

@mrfelton mrfelton closed this Oct 7, 2019
@mrfelton mrfelton deleted the feat/password branch October 7, 2019 18:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
scope: security issues that are security related status: needs rebase Issue needs to be rebased on top of the main branch type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants