multi: add ability to persist password on UI login #1119

martonp · 2021-07-02T20:23:45Z

This PR adds a checkbox to the login page that gives the user the option to persist their password for the session. When selected, all the password fields, other than disable account, will no longer be displayed.

When the checkbox is selected, the password is encrypted with a random key, and the encrypted password is stored in a cache in core. The key to decrypt the password is put into a cookie. Whenever a request that requires a password is made with an empty password field, the webserver uses the key from the cookie to retrieve the password from core.

Closes #946.

buck54321

First pass. Looks good conceptually, though I've suggested some architectural changes.

client/webserver/api.go

client/webserver/site/src/html/forms.tmpl

client/webserver/site/src/html/wallets.tmpl

client/core/core.go

client/webserver/site/src/js/settings.js

client/webserver/webserver.go

client/core/core.go

client/webserver/site/src/html/register.tmpl

martonp · 2021-07-15T19:11:01Z

client/webserver/webserver_test.go

@@ -396,7 +403,9 @@ func TestAPIWithdraw(t *testing.T) {
 		return resp.OK
 	}

-	body = &withdrawForm{}
+	body = &withdrawForm{


This is needed because resolvePass will return an error if this is an empty string.

martonp · 2021-07-15T19:43:29Z

@buck54321 This is ready for another review.

buck54321

Partial review. Lemme know what you think about these notes. I promise to review faster now.

client/webserver/http.go

client/webserver/site/src/html/forms.tmpl

client/webserver/site/src/html/login.tmpl

client/webserver/webserver.go

client/webserver/webserver_test.go

martonp · 2021-07-28T21:51:24Z

@buck54321 It's ready for another look.

In the registration workflow (where the page is not reloaded between forms) State.passwordIsCached is being used. I kept using the template arguments on the other pages though since it makes the code much more simple. There is always a page reload when going from the login/registration pages to every other page.

chappjc · 2021-08-05T02:26:26Z

@martonp Please rebase and resolve when you get a chance.

chappjc · 2021-08-05T19:00:23Z

@martonp Please try to avoid merging from master to get the branch up-to-date. Instead, rebase on master.

It is tricky to get rid of the merge commit when you synchronized with master @ 16da72f. I'm squashing this branch down to a single commit with 16da72f as it's base like:

git checkout rememberPW
git reset --hard 16da72f5e3af049db315400c68b87d7957c15bfa
git merge --squash HEAD@{1}
git commit -m "Add ability to remember password at login."

Then it can be rebased on master a bit more easily since it's just one commit now (git rebase master).

Although I see that the conflicts are gonna be tricky to resolve, particularly in forms.js, so thank you.

buck54321

Looking really good. I want to give it one more look, but it's testing well. Just a couple of little things for now.

buck54321 · 2021-08-07T15:00:17Z

client/webserver/api.go

+	passwordIsCached := s.isPasswordCached(r)
+	s.deauth()


Maybe switch the order of these two lines so that the comment is right above the line it applies to.

If I do that then I won't know if the password was cached before I called deauth. I can put the comment between the two lines.

buck54321 · 2021-08-07T15:16:54Z

client/webserver/site/src/html/settings.tmpl

-      <div{{if not .UserInfo.Authed}} class="d-hide"{{end}}>
+      <div {{if not .UserInfo.Authed}}class="d-hide"{{end}}>


Now, it will be <div > instead of <div>. What's the purpose? (one other place too)

This is a mistake.. it was red in my IDE the previous way, and I thought they were equivalent.

buck54321 · 2021-08-07T15:21:38Z

client/webserver/site/src/js/forms.js

+    this.fields.unlockErr.textContent = msg
+    Doc.show(this.fields.unlockErr)


this.setError(msg)

buck54321 · 2021-08-07T21:18:20Z

client/webserver/site/src/html/login.tmpl

+          <label for="rememberPass" class="pl-1 mb-1">Remember my password</label>
+          <input type="checkbox" id="rememberPass">


To be consistent with other forms, put the input before the label, and add class="form-check-input" to the checkbox. I think you'll need to wrap everything in a class="pl-4" div too, but not certain.

Also looks like you need some padding between the password field and the remember-password input.

Looks way better now.

buck54321 · 2021-08-14T19:40:49Z

client/webserver/site/src/js/wallets.js

+   */
+  async openWallet (assetID) {
+    if (!State.passwordIsCached()) {
+      this.showOpen.bind(this)(assetID)


Don't need to .bind(this) here.

buck54321 · 2021-08-14T19:40:56Z

client/webserver/site/src/js/wallets.js

+      if (app.checkResponse(res)) {
+        this.openWalletSuccess.bind(this)()
+      } else {
+        this.showOpen.bind(this)(assetID, `Error opening wallet: ${res.msg}`)


Don't need to .bind(this) here.

buck54321 · 2021-08-14T19:52:50Z

client/webserver/webserver.go

+	cookie, err := r.Cookie(pwKeyCK)
+	switch {
+	case err == nil:
+		sessionKey, err := hex.DecodeString(cookie.Value)
+		if err != nil {
+			return nil, err
+		}
+		return sessionKey, nil
+	default:
+		return nil, err
+	}


We actually want to see theses errors. How about this?

switch { case err != nil: ... case errors.Is(err, http.ErrNoCookie): return nil, nil default: return nil, err }

and then in getCachedPasswordUsingRequest, you return nil, errNoCachedPW on the condition pwKeyBlob == nil, but not err != nil

buck54321 · 2021-08-14T19:58:27Z

client/webserver/webserver.go

+// getCachedPassword takes authToken passed to and the key returned from
+// cacheAppPassword to retrieve and decrypt the app password.


// getCachedPassword retrieves the cached password for the user identified by authToken and // presenting the specified key in their cookies.

buck54321 · 2021-08-14T20:10:32Z

client/webserver/webserver.go

+
+	crypter, err := encrypt.Deserialize(key, cachedPassword.SerializedCrypter)
+	if err != nil {
+		return nil, fmt.Errorf("error deserializing crypter: %v", err)


Might as well %w.

buck54321 · 2021-08-14T20:10:37Z

client/webserver/webserver.go

+
+	pw, err := crypter.Decrypt(cachedPassword.EncryptedPass)
+	if err != nil {
+		return nil, fmt.Errorf("error decrypting password: %v", err)


Might as well %w.

buck54321 · 2021-08-14T20:24:35Z

client/webserver/webserver.go

+	srv             *http.Server
+	html            *templates
+	indent          bool
+	mtx             sync.RWMutex


We should really rename this to authMtx or something, shouldn't we?

buck54321 · 2021-08-14T20:33:09Z

client/webserver/api.go

+	pass, err := s.resolvePass(form.Pass, r)
+	if err != nil {
+		s.writeAPIError(w, fmt.Errorf("password error: %v", err))
+		return
+	}
+	coin, err := s.core.Withdraw(pass, form.AssetID, form.Value, form.Address)


We should always require the password to be entered manually for withdraws.

buck54321 · 2021-08-14T20:39:00Z

client/webserver/site/src/html/wallets.tmpl

+      <div class="fs16 px-4 text-center {{if $passwordIsCached}}d-hide{{end}}">Authorize the withdraw with your app password.</div>
+      <div class="d-flex px-4 mt-3 {{if $passwordIsCached}}justify-content-end{{end}}">
+        <div class="col-12 p-0 {{if $passwordIsCached}}d-hide{{end}}">


Should always require password for withdraws.

martonp · 2021-08-16T17:45:11Z

@buck54321 I agree with all the comments, and I've updated them all.

buck54321

Please increment the cache busters in bodybuilder.js, and this is good with me.

chappjc · 2021-08-18T14:36:03Z

client/webserver/webserver.go

+	srv             *http.Server
+	html            *templates
+	indent          bool
+	authMtx         sync.RWMutex


Would prefer a newline to segregate these guarded fields with their mutex. Shoulda been that way previously, so might as well change it now.

chappjc · 2021-08-18T14:41:50Z

client/webserver/webserver.go

+	if err != nil {
+		return nil, err
+	}
+	if pwKeyBlob == nil && err == nil {


err is guaranteed to be nil here, so this half of the condition is redundant.

chappjc · 2021-08-18T14:52:04Z

client/webserver/api.go

@@ -398,13 +438,8 @@ func (s *WebServer) apiLogout(w http.ResponseWriter, r *http.Request) {
 	// sessions to login again.


Pls update comments at deauth call sites to reflect it's expanded role w.r.t. cached passwords as well as the auth tokens.

chappjc · 2021-08-18T15:10:17Z

client/webserver/webserver.go

+	authTokens      map[string]bool
+	cachedPasswords map[string]*cachedPassword


The keys for these maps are the same, right (both auth tokens)? Don't worry about refactoring at this stage, but it suggests a single map would be adequate if the logic around access to the map were updated to reflect new semantics (found would indicate known auth token regardless of value stored including nil, while non-nil value would indicate a cached password). Rather than change anything, I just request a simple trailing comment on this delc line to doco the map key

cachedPasswords map[string]*cachedPassword // cached passwords keyed by auth token

Yeah that makes sense, I'll do another PR for this.

buck54321 requested changes Jul 10, 2021

View reviewed changes

martonp commented Jul 15, 2021

View reviewed changes

client/webserver/site/src/html/register.tmpl Outdated Show resolved Hide resolved

martonp commented Jul 15, 2021

View reviewed changes

buck54321 requested changes Jul 26, 2021

View reviewed changes

chappjc added UI labels Aug 2, 2021

chappjc added this to the 0.3 milestone Aug 3, 2021

chappjc self-requested a review August 5, 2021 02:25

Add ability to remember password on UI login.

78b22ff

martonp force-pushed the rememberPW branch 2 times, most recently from 0ccc8f3 to 78b22ff Compare August 6, 2021 19:12

martonp mentioned this pull request Aug 6, 2021

ui: Add table showing available markets when registering for dex. #1135

Merged

buck54321 reviewed Aug 7, 2021

View reviewed changes

Update.

7967fc5

buck54321 requested changes Aug 14, 2021

View reviewed changes

Updates based on comments.

fdd28eb

buck54321 approved these changes Aug 17, 2021

View reviewed changes

chappjc requested changes Aug 18, 2021

View reviewed changes

martonp added 2 commits August 18, 2021 14:04

Update.

b679355

Update cachebusters.

0044605

chappjc approved these changes Aug 19, 2021

View reviewed changes

chappjc merged commit 7a0c387 into decred:master Aug 19, 2021

martonp deleted the rememberPW branch December 20, 2022 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: add ability to persist password on UI login #1119

multi: add ability to persist password on UI login #1119

martonp commented Jul 2, 2021

buck54321 left a comment

martonp Jul 15, 2021

martonp commented Jul 15, 2021

buck54321 left a comment

martonp commented Jul 28, 2021

chappjc commented Aug 5, 2021

chappjc commented Aug 5, 2021 •

edited

buck54321 left a comment

buck54321 Aug 7, 2021

martonp Aug 9, 2021

buck54321 Aug 7, 2021

martonp Aug 9, 2021

buck54321 Aug 7, 2021

buck54321 Aug 7, 2021

martonp Aug 9, 2021

buck54321 Aug 14, 2021

buck54321 Aug 14, 2021

buck54321 Aug 14, 2021

buck54321 Aug 14, 2021

buck54321 Aug 14, 2021

buck54321 Aug 14, 2021

buck54321 Aug 14, 2021

buck54321 Aug 14, 2021

buck54321 Aug 14, 2021

martonp commented Aug 16, 2021

buck54321 left a comment

chappjc Aug 18, 2021

chappjc Aug 18, 2021

chappjc Aug 18, 2021

chappjc Aug 18, 2021

martonp Aug 18, 2021

		<div{{if not .UserInfo.Authed}} class="d-hide"{{end}}>
		<div {{if not .UserInfo.Authed}}class="d-hide"{{end}}>

		this.fields.unlockErr.textContent = msg
		Doc.show(this.fields.unlockErr)

		<label for="rememberPass" class="pl-1 mb-1">Remember my password</label>
		<input type="checkbox" id="rememberPass">

		// getCachedPassword takes authToken passed to and the key returned from
		// cacheAppPassword to retrieve and decrypt the app password.

		@@ -398,13 +438,8 @@ func (s WebServer) apiLogout(w http.ResponseWriter, r http.Request) {
		// sessions to login again.

		authTokens map[string]bool
		cachedPasswords map[string]*cachedPassword

multi: add ability to persist password on UI login #1119

multi: add ability to persist password on UI login #1119

Conversation

martonp commented Jul 2, 2021

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented Jul 15, 2021

buck54321 left a comment

Choose a reason for hiding this comment

martonp commented Jul 28, 2021

chappjc commented Aug 5, 2021

chappjc commented Aug 5, 2021 • edited

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented Aug 16, 2021

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Aug 5, 2021 •

edited