Fix/auth page improvements #502

henriquecbuss · 2021-03-31T01:29:23Z

What issue does this PR close

Closes #501

Changes Proposed ( a list of new changes introduced by this PR)

A lot of the changes in this PR are transparent to the user.

What the users can see:

Guest pages (Login and Register) now use the same Feedback system as LoggedIn pages (the red/green bar at the top of the screen).
The Feedback styles have been fixed so the bar sticks to the top once the user scrolls past it, so it's always visible.
The Login page has a better flow. If an error happens, the user doesn't need to reload the page to try again.
For browsers that support the clipboard API (for example, chrome for desktop), there is now a "Paste" button so users can paste in their 12 words in the Login page.

What the developers can see:

The Auth module is much smaller and easier to understand. It's now only concerned about logged in users, and can be in a WithoutPrivateKey or WithPrivateKey state.
The logic and view of the Auth module that dealt with non-logged in users is now in the Login module, since that was the only page that used it.
The View.Input module allows for more flexibility and customization on counting strategies, counter styles, error styles, HTML elements inside the input area, and using different HTML elements as input (such as input or textarea).
The Pin module is now a component with its own model/view/update, so it can be used both by Guest and LoggedIn without much code repetition.
Much more consistent logging when dealing with ports and JS.
Some extra logs through the authentication/login process.

How to test ( a list of instructions on how to test this PR)

This deals with authentication, so it's important to be thorough!

The main affected area is the login page, so try logging in with valid and invalid data, check if the errors are easily seen on any screen size (mainly that they stick to the top of the screen if you've scrolled down enough). Make sure you can still register a new user as well.

This also deals with the PIN modal, so try doing something that requires that, and see if it behaves accordingly.

Also check if the feedback bar at the top of the screen works as expected (is always visible even after scrolling past it) on the pages where you need to be logged in to see.

src/elm/Auth.elm

lucca65

Just some commenting on it! Keep up bro, you are doing good! 🏃‍♂️ 🔥

src/elm/View/Form/Input.elm

lucca65

On safari for Mac this happens when I click the button

Login also fails when I try to run it. But its weird because I've got no error in any requests, nor any console logs

public/translations/cat-CAT.json

lucca65 · 2021-04-07T20:21:56Z

public/translations/en-US.json

@@ -104,6 +104,10 @@
    "invalidSymbol": "Invalid symbol.",
    "unknown": "Uoh. Something wrong happened.",
    "accountNotFound": "Account not found. Are you sure you created your account?",
+    "accountDoesNotCorrespond": "This private key does not correspond to logged in account. Please logout to use a different account.",


If we ever got to this state, we cannot do nothing about it, the session is doomed, maybe we can show a modal displaying this message and force a logout?

Honestly, I don't even think it's possible to get to this state, but it was there before and I couldn't figure out how to test it, so I just kept it, but yes, we can do something like that.

Also, does it still make sense to get multiple account_names for the same public key/user (there was a LoginWithMultipleAccounts state before that I removed, because it wasn't being generated)? Can we be sure that the correct account (if there is one) will always be at the first position in the list?

Yes, this case is actually impossible.
For it to happen, the user must be logged in (have their info on localStorage), but this port is only called from the Login page (which means the user is not logged in). But, if this ever happens, I thinks it's safe to just ignore it: it would be the same as the user logging out and logging back in, with a different account (we can make sure all of the data a logged in user can have in localStorage is deleted every time on login, just to be safe)

src/elm/Session/LoggedIn.elm

lucca65 · 2021-04-07T20:36:53Z

src/elm/Session/LoggedIn.elm

-initModel : Shared -> Auth.Model -> Eos.Name -> Symbol -> String -> Model
-initModel shared authModel accountName selectedCommunity authToken =
+initModel : Shared -> Maybe String -> Eos.Name -> Symbol -> String -> Model
+initModel shared maybePrivateKey_ accountName selectedCommunity authToken =


we could introduce a type for private key instead of using a String

I actually introduced a type alias PrivateKey = String later on and forgot to change stuff to use it (will do so). I don't think we need a type PrivateKey for this though, it would just mean more wrapping/unwrapping, and I can't see many benefits we get from that

i can see only one:

having an easy way to validate private keys from elm. that's something we don't do today, we only check if the length for the word list is correct. but there are actually reserved words and forgotten characters

in any way my suggestion was just to keep this opaque types pattern we've been using

I went to search for something, and found this type that already exists. Should we use that?

Just to be sure, by private key you mean the hashed analogue of the passphrase?

Does it make sense to validate either the passphrase (12 words) (other than the word count) or the private key? They're both generated automatically, not from the user 🤔

I went to search for something, and found this type that already exists. Should we use that?
I think so yeah, in any case we can have a different function that only takes model, this way you don't need to wrap, unwrap every time!

Just to be sure, by private key you mean the hashed analogue of the passphrase?

Does it make sense to validate either the passphrase (12 words) (other than the word count) or the private key? They're both generated automatically, not from the user 🤔

Actually the 12 words is a mnemonic that serves as base (with other information to generate the private key)

Actually the 12 words is a mnemonic that serves as base (with other information to generate the private key)

Ok, but does it make sense to validate any of these? We're dealing with

12 words (generated for the user, doesn't really make sense to validate)

Private key (generated for the user, doesn't really make sense to validate)

PIN (just a sequence of digits, not much to validate other than length and being only digits)

The only data we get from the user is from registration (name, username, email, etc), but I haven't gone through that in this PR

12 words (generated for the user, doesn't really make sense to validate)
although its generated for they user have to type it on the text area, or paste it to login.

Private key (generated for the user, doesn't really make sense to validate)
+1

PIN (just a sequence of digits, not much to validate other than length and being only digits)
+1 Pin validation that we already have is enough

src/elm/Session/LoggedIn.elm

src/elm/View/Feedback.elm

src/index.js

henriquecbuss · 2021-04-08T18:51:44Z

@lucca65 I suspect your issue with pasting might be related to permissions. I've added some handling for that, so it's clearer to the user what's going on.
I'm not sure about not being able to log in, maybe it's somehow related to the clipboard again? 🤔

Anyway, I can't test on Safari/Mac, so can you try again please?

lucca65

Still don't work on safari for iOS and have this strange behavior on Safari for Mac.

henriquecbuss · 2021-04-09T16:01:44Z

Huh, that's weird. Nothing shows up as Feedback or on the console?

By the way, did you mean to approve this?

lucca65 · 2021-04-09T16:07:02Z

yes! i approved the pr

lucca65 · 2021-04-09T16:07:30Z

something must be up with netlify again

henriquecbuss · 2021-04-09T16:25:31Z

Oh yeah, auth isn't working on netlify deploys - we still don't have GRAPHQL_SECRET defined, so it sends an empty string as the password and fails authentication. I've deployed this on staging.

@heltonlr can you test this out on staging?
It's mainly the login page flow. There's a button to paste from clipboard (when the browser supports it. note: you need to use https for this to work/show up), better error handling, etc. Also try doing stuff that needs you to be authenticated and that asks for your PIN

heltonlr

Wasn't possible to login. This error appears
@NeoVier

I am using Google Chrome on desktop, I created an account and tried to login with the 12 words. In some moments the error appears on 12 words screen and other times on PIN screen...

henriquecbuss added 10 commits March 30, 2021 19:41

Use View.Feedback in Guest and LoggedIn

d992de4

Remove PaymentHistory page

70b9041

Remove unused states

24a2852

Simplify Auth.Status

76694c5

Fix elm-review errors

ad66f69

Simplify Auth.Msg

dbc47d1

Better error viewing for modals

8548f0b

Add paste button

f6d1971

Improve usability

968446c

Add I18N to auth failed message

5e96370

lucca65 reviewed Mar 31, 2021

View reviewed changes

src/elm/Auth.elm Outdated Show resolved Hide resolved

henriquecbuss added 9 commits April 5, 2021 16:14

Fix feedback to be sticky on loggedIn

db419e7

Move Auth.Unauthenticated stuff to Login

3ddccd1

WIP: Remove Unauthenticated from Auth

10c6475

WIP: Complete sign in process

1056747

WIP: Separate PIN into its own module

a930c42

Rename isAuth -> hasPrivateKey

1c0e86a

Move signIn mutation into Guest.initLoggingIn

84dc51e

WIP: Use View.Form.Input on Login.elm

ed1320d

Enforce passphrase length, some logging and bug fixing

13e18c3

lucca65 reviewed Apr 6, 2021

View reviewed changes

src/elm/View/Form/Input.elm Outdated Show resolved Hide resolved

henriquecbuss added 9 commits April 6, 2021 19:50

Fix clipboard bug

2f12b82

WIP: Minor tweaks, remove unnecessary stuff from Auth

0925ecd

Remove unnecessary route

3277ffb

Refactor auth process on the JS side

d6bef99

Merge branch 'master' into fix/auth-page-improvements

2174fc3

Improve logging overall

368cb09

Make Register.elm use Feedback, minor tweaks

6da9ee4

Add more logging on Elm

7f7783e

Remove unused imports

f5c52eb

henriquecbuss marked this pull request as ready for review April 7, 2021 19:42

henriquecbuss requested a review from lucca65 April 7, 2021 19:42

lucca65 requested changes Apr 7, 2021

View reviewed changes

henriquecbuss added 4 commits April 7, 2021 21:39

Fix some pointers from review

49f1745

Fix z-index bug

79621c6

Detect permission state for clipboard, add more logging

83678aa

Remove login with different key case

e00984f

Remove unused translations

a9391b7

henriquecbuss requested a review from lucca65 April 9, 2021 01:13

lucca65 previously approved these changes Apr 9, 2021

View reviewed changes

heltonlr reviewed Apr 10, 2021

View reviewed changes

Add impossible error logging to javascript

8eef992

henriquecbuss dismissed lucca65’s stale review via 8eef992 April 10, 2021 14:46

heltonlr approved these changes Apr 10, 2021

View reviewed changes

henriquecbuss merged commit c8cf262 into master Apr 10, 2021

henriquecbuss deleted the fix/auth-page-improvements branch April 10, 2021 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/auth page improvements #502

Fix/auth page improvements #502

henriquecbuss commented Mar 31, 2021 •

edited

Loading

lucca65 left a comment

lucca65 left a comment

lucca65 Apr 7, 2021

henriquecbuss Apr 7, 2021

henriquecbuss Apr 8, 2021

lucca65 Apr 7, 2021

henriquecbuss Apr 7, 2021

lucca65 Apr 7, 2021

henriquecbuss Apr 7, 2021

lucca65 Apr 8, 2021

henriquecbuss Apr 8, 2021

lucca65 Apr 9, 2021

henriquecbuss commented Apr 8, 2021

lucca65 left a comment

henriquecbuss commented Apr 9, 2021

lucca65 commented Apr 9, 2021

lucca65 commented Apr 9, 2021

henriquecbuss commented Apr 9, 2021

heltonlr left a comment •

edited

Loading

Fix/auth page improvements #502

Fix/auth page improvements #502

Conversation

henriquecbuss commented Mar 31, 2021 • edited Loading

What issue does this PR close

Changes Proposed ( a list of new changes introduced by this PR)

How to test ( a list of instructions on how to test this PR)

lucca65 left a comment

Choose a reason for hiding this comment

lucca65 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

henriquecbuss commented Apr 8, 2021

lucca65 left a comment

Choose a reason for hiding this comment

henriquecbuss commented Apr 9, 2021

lucca65 commented Apr 9, 2021

lucca65 commented Apr 9, 2021

henriquecbuss commented Apr 9, 2021

heltonlr left a comment • edited Loading

Choose a reason for hiding this comment

henriquecbuss commented Mar 31, 2021 •

edited

Loading

heltonlr left a comment •

edited

Loading