[#935] Auth with recovery code by tuliocasagrande · Pull Request #1049 · pixelated/pixelated-user-agent

tuliocasagrande · 2017-04-06T17:09:49Z

No description provided.

deniscostadsc

We think it's a good idea to change the HTTP status to a more meaningful.

deniscostadsc · 2017-04-06T19:47:02Z

service/test/unit/resources/test_account_recovery_resource.py

-        def assert_successful(success):
-            self.assertEqual(success, 'Done!')
+        def assert_error_response(_):
+            self.assertEqual(500, request.responseCode)


Hey,

@thaissiqueira and me think that in case of validation error the best HTTP status is 400 (bad request), not 500.

deniscostadsc · 2017-04-06T19:47:32Z

service/test/unit/resources/test_account_recovery_resource.py

-        self.assertFalse(self.resource._validate_password('12345678', '1234'))
-
-    def test_validate_password_failed_by_length(self):
-        self.assertFalse(self.resource._validate_password('1234', '1234'))


Why did you remove these tests?

same as above, please have a look at #1047 (comment)

deniscostadsc · 2017-04-06T19:48:31Z

service/test/unit/resources/test_account_recovery_resource.py

        request.method = 'POST'
-        self.resource._handle_post = MagicMock(return_value=defer.succeed(None))
+        request.content = MagicMock()
+        request.content.getvalue.return_value = '{"password": "12345678", "confirmPassword": "12345678"}'


Did you stop mocking POST handler in order to test it?

please have a look at #1047 (comment)

deniscostadsc · 2017-04-07T16:52:43Z

web-ui/src/account_recovery/new_password_form/new_password_form.js

  submitHandler = (event) => {
    event.preventDefault();
    submitForm(event, '/account-recovery', {
+      username: this.props.username,


Did you check if it works if the user adds username with domain?

deniscostadsc · 2017-04-07T16:59:12Z

web-ui/src/account_recovery/page.spec.js

+    pageInstance = page.instance();
+
+    expect(pageInstance.state.username).toEqual('');
+  });


Should it return an error in case of username is empty?

What exactly this test is testing? Is it testing if the code can handle empty querystring or it's just one more test case?

deniscostadsc · 2017-04-07T17:54:55Z

web-ui/src/common/util.js

 export const hasQueryParameter = (param) => {
  const decodedUri = decodeURIComponent(window.location.search.substring(1));
  return !(decodedUri.split('&').indexOf(param) < 0);
 };


These observations are not really from this PR, but does this method still need these handmade parsing after adding that js library to parse querystring?

anikarni · 2017-04-20T18:23:03Z

service/pixelated/account_recovery.py

            code = self._soledad.create_recovery_code()
            response = yield self._bonafide_session.update_recovery_code(code)
+            log.info('recovery code updated')
+            log.info(response)


Is this just for testing purposes? should we be committing logging that we don't know what it could say about the user, or if the formatting breaks, or anything of the kind?

You're right, @anikarni
That was committed by mistake!

anikarni · 2017-04-20T18:27:02Z

service/pixelated/authentication.py


 class Authenticator(object):
-    def __init__(self, leap_provider):
+    def __init__(self, leap_provider, recovery=False):


Is this supposed to be a boolean? Can we either rename the variable to make it clear, or not set it to False?

I changed the variable to recovery_session and it is a boolean indeed.
Do you think it's clearer now, @anikarni ?

anikarni · 2017-04-20T18:28:25Z

service/pixelated/resources/account_recovery_resource.py

+            elif failure.type is UnauthorizedLogin:
+                request.setResponseCode(UNAUTHORIZED)
+            else:
+                request.setResponseCode(INTERNAL_SERVER_ERROR)


Can we map these in a dict or somehting of the kind? There would be a lot of ifs if we continue down this path. We could also resuse it, as this is already used elsewhere.

Also, shouldn't this be used in common with authentication/login errors

hey @anikarni
I extracted this response code flow to the common parent BaseResource and it should be easier for adding new exceptions. I noticed however that the LoginResource doesn't apply these response codes, since its implementation redirects to /login?auth-error, overriding any previous code to 302. I think this refactoring is still valuable though. Please take a look.

anikarni · 2017-04-20T18:29:46Z

service/pixelated/resources/account_recovery_resource.py


+    def _validate_empty_fields(self, username, user_code):
+        if not username or not user_code:
+            raise EmptyFieldsError('The user entered an empty username or empty usercode')


"emtyp user recovery code"

@tayanefernandes

…ticator with @tayanefernandes

@tuliocasagrande

with @tuliocasagrande

@tuliocasagrande

with @tuliocasagrande

@tuliocasagrande

with @tuliocasagrande

@tuliocasagrande

with @tuliocasagrande

deniscostadsc suggested changes Apr 6, 2017

View reviewed changes

deniscostadsc reviewed Apr 7, 2017

View reviewed changes

tayanefernandes changed the title ~~[WIP] [#935] Simplify tests for account recovery~~ [WIP] [#935] Auth with recovery code Apr 7, 2017

FrailWords force-pushed the auth-recovery-code branch 2 times, most recently from 4d7a38a to 60f9cfb Compare April 13, 2017 03:09

FrailWords force-pushed the auth-recovery-code branch from 60f9cfb to e8f2fe6 Compare April 18, 2017 21:24

Tulio Casagrande added 3 commits April 18, 2017 18:33

[#935] Simplify tests for account recovery

4915e6d

[#935] Send username and usercode to back-end

8d5349a

[#935] Extract credentials from the authentication flow

b3365bb

FrailWords force-pushed the auth-recovery-code branch from e8f2fe6 to 20ae5e6 Compare April 18, 2017 21:35

FrailWords changed the title ~~[WIP] [#935] Auth with recovery code~~ [#935] Auth with recovery code Apr 20, 2017

anikarni reviewed Apr 20, 2017

View reviewed changes

Sriram Viswanathan and others added 6 commits April 21, 2017 12:25

[#935] Implements account recovery authenticator which extends Authen…

9e0d0f1

…ticator with @tayanefernandes

[#935] Authentication using account recovery code

c689e8d

with @tuliocasagrande

[#935] Use development branch for bitmask dependency

14db903

with @tuliocasagrande

[#935] Fixes error message in case of auth error

8bee0c0

with @tuliocasagrande

[#935] Shows snackbar error response on new password submit

66d772a

with @tuliocasagrande

[#935] Rename recovery session variable

dbb3c90

tuliocasagrande force-pushed the auth-recovery-code branch from 559d816 to dbb3c90 Compare April 21, 2017 15:30

[#935] Fix invalid recovery code log message

b3bbc05

deniscostadsc self-requested a review April 21, 2017 15:41

Tulio Casagrande added 2 commits April 21, 2017 14:50

[#935] Improve error response flow for authentication

2088174

[#935] Decrease log level of some account recovery exceptions

bfbdd84

Conversation

tuliocasagrande commented Apr 6, 2017

Uh oh!

deniscostadsc 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants