Passwords can now be changed (#687) by himanish-star · Pull Request #694 · mozilla/send

himanish-star · 2018-01-03T11:59:57Z

fixes: #687

@ehuggett , Thanks for the help I have added the ability to change password.

However I still face one error and that is as follows

The password does get reset but it fails once with status 401 and then proceeds as per the existing code below. It doesn't affect in any way(as you can see in the gif below that all works well) but still I would like to get rid of this error. Could you suggest how ? At the mean time I am also having a look into this matter

try {
  await sendPassword(file, authKey, rawAuth);
} catch (e) {
  if (e.message === '401' && file.nonce !== e.nonce) {
    await sendPassword(file, authKey, rawAuth);
  } else {
    throw e;
  }
}

himanish-star · 2018-01-03T23:29:35Z

If no password has been set then existingPassword is undefined, this error prevents the password from being set the first time.

Nope this doesn't happen I believe. @ehuggett , you can also see the gif and over there too the password was being set the first time and no error shows. Errors show up only when we try to reset the password. The reset procedure fails once with status 401 and then proceeds with status 200

ehuggett · 2018-01-04T02:05:19Z

Nope this doesn't happen I believe. @ehuggett , you can also see the gif(...)

I get the error when using Firefox Developer Edition 58.0b13 (64-bit) & Firefox ESR 52.5.2 (64-bit)
, Your PR does seem to work in chromium version 63.0.3239.84.

note: the 'reset' button is not well formatted in Firefox either

The reset procedure fails once with status 401 and then proceeds with status 200

I see that now. sendPassword was only updating the stored nonce if the request failed, my changes to have it always update the file nonce seem clean enough so I've opened a PR to have you include them in your branch himanish-star#1 (which should cause it to be included here, so you should test it yourself before merging it).

himanish-star · 2018-01-04T02:18:29Z

@ehuggett , thanks for the help. This issue has really helped me get a deeper understanding of the mozilla/send code base. So I need to correct the logic of existingPassword, got to format the reset Button and got to merge your branch onto mine. Ok I'm working on it. I'll make these changes later on , as I now got to go to college. If there are any other changes you want me to incorporate please do tell me.

shikhar-scs · 2018-01-04T02:21:38Z

Also @ehuggett would you mind reviewing these PRs #695 #686 #681 #679.
I have asked @dannycoates too but he seems to be unavailable since a long time.

dannycoates · 2018-01-04T02:53:14Z

@shikhar-scs @himanish-star I appreciate your enthusiasm, but please chill 😉

I will be able to review your PRs soon. I'm the only current maintainer who can merge them.

himanish-star · 2018-01-04T04:00:21Z

@dannycoates, sorry for the inconvenience 😅

shikhar-scs · 2018-01-04T04:08:17Z

@dannycoates, yeah Sorry for the inconvenience .

himanish-star · 2018-01-05T00:31:50Z

@ehuggett , Now you can have a look at the changes . I have fixed all the bugs and have made the requested changes and I have also merged your branch into my branch

himanish-star · 2018-01-09T04:45:51Z

@dannycoates , shall I resolve the conflicts ?

dannycoates · 2018-01-09T19:30:10Z

@himanish-star yes, you can resolve conflicts, but it will probably be a little while longer before this feature gets reviewed / merged.

himanish-star · 2018-01-10T00:49:12Z

@dannycoates , ok I will do this in a later part of the day

himanish-star · 2018-01-14T14:53:09Z

@dannycoates , I have fixed the conflicts and things are back to normal.

Here is a GIF with the changes

dannycoates

This looks really good @himanish-star! Thanks for your patience. We might want to make a few little tweaks before pushing to production, but this is almost ready to merge.

dannycoates · 2018-01-16T22:22:21Z

app/fileSender.js

+        await sendPassword(file, oldAuthKey, rawAuth);
+      } else {
+        await sendPassword(file, authKey, rawAuth);
+      }


Just a nit on style... I'd prefer something like:

const aKey = existingPassword ? oldAuthKey : authKey; await sendPassword(file, aKey, rawAuth);

dannycoates · 2018-01-16T22:25:21Z

app/templates/share.js

+
+function toggleResetInput(event) {
+  const form = event.target.parentElement.querySelector('form');
+  if (form.style.visibility === 'hidden' || form.style.visibility === '')


Another style nit: no ifs without brackets

dannycoates · 2018-01-16T22:38:05Z

app/fileSender.js

    const xhr = new XMLHttpRequest();
    xhr.onreadystatechange = () => {
      if (xhr.readyState === XMLHttpRequest.DONE) {
+        const nonce = xhr.getResponseHeader('WWW-Authenticate').split(' ')[1];


There are cases where this header is not set (status == 404 for example), so would throw an exception here

himanish-star · 2018-01-17T00:58:23Z

app/templates/share.js

+  function resetPassword(event) {
+    event.preventDefault();
+    const existingPassword = document.querySelector('.passwordOriginal')
+      .innerText;


This is regarding indentation . Shall I do this

const existingPassword = document.querySelector('.passwordOriginal') .innerText;

or this

const existingPassword = document.querySelector('.passwordOriginal').innerText;

In general try to keep it on one line, but our auto-formatter prettier might change it.

himanish-star · 2018-01-17T01:12:04Z

app/fileSender.js

        if (xhr.status === 200) {
          return resolve(xhr.response);
        }
-        if (xhr.status === 401) {


Do you want me to revert this removed code back. If in case the status=401 then the below code would help to reset the file.nonce

if (xhr.status === 401) { const nonce = xhr.getResponseHeader('WWW-Authenticate').split(' ')[1]; file.nonce = nonce; }

dannycoates · 2018-01-17T19:37:35Z

app/templates/share.js

      password: '<pre></pre>'
-    })}</div>`
+    })}
+    <button id="resetButton">Reset</button>


I missed this earlier... The "Reset" text here needs to be localized. We need to add a string to public/locales/en-US/send.ftl and use state.translate like the other strings here. I also think "Change" is a better word than "Reset" in this case.

Yes , I know that it has to be localised. But I thought I would have to do it in a number of different languages . Thus I had left it alone. I'll make the changes soon.

dannycoates · 2018-01-17T19:46:58Z

app/templates/share.js

+        placeholder="${state.translate('unlockInputPlaceholder')}">
+      <input type="submit"
+        id="unlock-reset-btn"
+        class="btn btn-hidden"


Does it work if you set onclick=${toggleResetInput} here instead of with the querySelector below? If so it's the preferred style. (also same with others)

Yes, I know that this is the preferred style, but I had tried to do it this way but it hadn't worked. I'll try again and if it doesn't work , I'll ask you for help 😃

Nope , using onclick=${toggleResetInput} doesn't work 😞. Can you try the same thing out once @dannycoates and I have also made the changes requested

Thanks for checking @himanish-star. I see the reason it doesn't work... using the function form "html()" processes the string differently than the template string form "html``". We need to use the function form here so that the password text gets escaped properly. It's not a big deal.

We need to use the function form here so that the password text gets escaped properly. It's not a big deal.

So you mean we can still write onclick=${toggleResetInput}. Ok, I'll try searching more over the net of how to do this. Meanwhile if you can suggest me of how this is done, then I will surely rectify my mistake

No, no, I was just commenting on why we needed to use html() instead of html``

Your code is fine :)

dannycoates · 2018-01-18T19:45:57Z

Nice work @himanish-star! 💯

himanish-star · 2018-01-19T06:04:32Z

@dannycoates , Thank for merging. Can I take up issue #324 ? 😃

ghost requested review from dannycoates and johngruen January 3, 2018 18:52

himanish-star mentioned this pull request Jan 3, 2018

checkbox now has a hover effect: fixes #635 #685

Merged

himanish-star force-pushed the feature-change-password branch 2 times, most recently from 9b26f2e to 6173e76 Compare January 5, 2018 00:28

himanish-star force-pushed the feature-change-password branch from 1daaf82 to abc930c Compare January 14, 2018 14:52

dannycoates suggested changes Jan 16, 2018

View reviewed changes

himanish-star commented Jan 17, 2018

View reviewed changes

himanish-star force-pushed the feature-change-password branch from abc930c to ef28050 Compare January 17, 2018 01:16

dannycoates suggested changes Jan 17, 2018

View reviewed changes

Passwords can now be reset

fdcc31f

himanish-star force-pushed the feature-change-password branch from ef28050 to fdcc31f Compare January 18, 2018 10:15

himanish-star requested a review from flodolo as a code owner January 18, 2018 10:15

dannycoates approved these changes Jan 18, 2018

View reviewed changes

dannycoates merged commit 9688dde into mozilla:master Jan 18, 2018

Conversation

himanish-star commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

himanish-star commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehuggett commented Jan 4, 2018

Uh oh!

himanish-star commented Jan 4, 2018

Uh oh!

shikhar-scs commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dannycoates commented Jan 4, 2018

Uh oh!

himanish-star commented Jan 4, 2018

Uh oh!

shikhar-scs commented Jan 4, 2018

Uh oh!

himanish-star commented Jan 5, 2018

Uh oh!

himanish-star commented Jan 9, 2018

Uh oh!

dannycoates commented Jan 9, 2018

Uh oh!

himanish-star commented Jan 10, 2018

Uh oh!

himanish-star commented Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dannycoates left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanish-star Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannycoates commented Jan 18, 2018

Uh oh!

himanish-star commented Jan 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

himanish-star commented Jan 3, 2018 •

edited

Loading

himanish-star commented Jan 3, 2018 •

edited

Loading

shikhar-scs commented Jan 4, 2018 •

edited

Loading

himanish-star commented Jan 14, 2018 •

edited

Loading

himanish-star Jan 18, 2018 •

edited

Loading