Standardized in-app authentication #16916
Conversation
Jenkins BuildsClick to see older builds (80)
|
J-Son89
force-pushed
the
standard-auth-process
branch
2 times, most recently
from
ee587a2
to
bc834e3
Compare
J-Son89
requested review from
ajayesivan,
smohamedjavid,
codemaster115,
FFFra and
ibrkhalil
J-Son89
force-pushed
the
standard-auth-process
branch
from
bc834e3
to
89e20fa
Compare
| @@ -87,12 +87,6 @@ | |||
| small-height (:track-height constants/small-dimensions)] | |||
| (h/has-style mock {:height small-height}))) | |||
|
|
|||
| (h/test "render with the correct customization-color" | |||
There was a problem hiding this comment.
testing styles is brittle, in my opinion it will be better to add unit tests on the custom color functionality directly rather than on components using it.
I will look to add this in a separate set of work
J-Son89
force-pushed
the
standard-auth-process
branch
from
89e20fa
to
3ab5de2
Compare
|
|
||
| }) |
There was a problem hiding this comment.
Move the closing brackets to line 25
| {:margin-top 12 | ||
| :background-color colors/white-opa-5 | ||
| :border-radius 20 | ||
| :flex 1 |
| [qr-code-viewer/qr-code-view 331 @code] | ||
| ] |
There was a problem hiding this comment.
Move the closing brackets to previous line
| :fallback-button-label (i18n/label :t/reveal-sync-code)}]]) | ||
| ] | ||
|
|
||
| ] | ||
|
|
| "Options | ||
| - `on-complete` Callback called when the sliding is complete | ||
| - `disabled?` Boolean that disables the button | ||
| (_and gestures_) | ||
| - `size` `:small`/`:large` | ||
| - `size` `40`/`48` |
There was a problem hiding this comment.
Why moving away from :small :large?
Also I would not be passing around 40/48, they look a lot like magic numbers (I am personally against passing keywords as well, I'd const everything so there's some safety), but especially with numbers, I'd const them somewhere (withing the component itself makes sense, since if you are using the component, you are requiring that namespace), but that's a larger discussion I take, maybe @J-Son89 @ilmotta @erikseppanen @flexsurfer have an opinion? (apologies if it's been discussed already)
TLDR:
[slidebutton.view/view {:size 40}] vs [slidebutton/view {:size slidebutton/size-small} (or slidebutton/size-40)
(I am for the latter, which is what we do for colors etc)
There was a problem hiding this comment.
Thanks @cammellos, I believe this is still an open discussion. I agree with you here and like this suggestion the most 👍
There was a problem hiding this comment.
@cammellos, this is a recurring discussion in PRs.
My opinion has always been that magical design values should be avoided. Tomorrow a designer can change their mind and then we'd need to go through an error-prone process to update all magical values. Sometimes, these magical values also hurt readability because the developer needs to constantly memorize whatever they mean.
The deeper discussion to me is related to using semantic values versus magical values. For example, we're full of 50 and 60 numbers in the code, colors are hardcoded willy-nilly, shadows too use numbers, and so on. One argument in favor of using magical design values is that they eliminate this translation layer between code and Figma, which can help reduce UI bugs.
The code is also a symptom of the way designers use Figma. I've once integrated with a design system that avoided magical values like the plague, and that in turn positively translated to the code. I don't believe our design team will be able to make such drastic changes in Figma, so realistically, it'll be probably up to us to change the code independently.
No discussion oriented towards achieving consensus has been done in our team, I guess it's about time :)
There was a problem hiding this comment.
Yep, I think this could do with some finalised decision. There could also be a set of sizes in figma foundations similar to typography etc so using :small :medium are consistent across components and designs.
There was a problem hiding this comment.
I also would vote for using keywords/constants instead of numbers. More up for keywords (:size-40) to have a finite list of options when using within cond.
cc @ulisesmac, since we had a similar discussion regarding size for context tags component
There was a problem hiding this comment.
I'll set up a vote in the mobile discord and we can record the decision
There was a problem hiding this comment.
Updated to use: :size/s-40 and :size/s-48
| [utils.security.core :as security] | ||
| [status-im2.common.standard-authentication.enter-password.style :as style] | ||
| [quo2.theme :as quo.theme])) | ||
|
|
|
|
||
| (defn- view-internal | ||
| [{:keys [theme on-enter-password button-label]}] | ||
| (let [entered-password (atom "") |
There was a problem hiding this comment.
This atom should be defined outside the Reagent renderer.
There was a problem hiding this comment.
sorry, I'm a bit confused what you mean here. Could you clarify with a code snippet example? 🙏
There was a problem hiding this comment.
Jamie, Icaro wants to say that this atom will be recreated on each rerender since this is in the render function. To put it outside of it, just use a form-2 component:
(defn view [initial-props]
(let [your-atom (atom "")] ; outside render fn
(fn [props] ; this is the render function
[:<>] ; hiccup code
)
))| :icon :i/info} | ||
| error] | ||
| [rn/pressable | ||
| {:active-opacity 1 |
There was a problem hiding this comment.
What is this prop active-opacity? It seems to be a leftover of the Touchable API.
There was a problem hiding this comment.
yeah, seems I missed some pieces on this pr. It was an external candidates initially - will remove 👍
| :error? (seq error) | ||
| :accessibility-label :password-input | ||
| :label (i18n/label :t/profile-password) | ||
| :on-change-text (fn [password] |
There was a problem hiding this comment.
This function argument is shadowing the binding password. It's a bit dangerous/easy to introduce a bug. The on-change-text function can actually be defined outside the renderer, in the form-2 let because the function only relies on entered-password.
There was a problem hiding this comment.
sure, I'll change to user-password or something like that 👍
There was a problem hiding this comment.
actually, I've noticed that this input is also used on initial login page? I will extract these pieces out into a common connected component and see if I can address this in both places
There was a problem hiding this comment.
Makes sense, thanks @J-Son89
| :placeholder (i18n/label :t/type-your-password) | ||
| :auto-focus true | ||
| :show-cancel false | ||
| :error? (seq error) |
There was a problem hiding this comment.
Slight improvement: seq returns a sequence, which is more expensive to diff than a boolean and will affect reagent's ability to optimize quo/input. So when passing props to components that expect booleans, it's almost always better to pass a true boolean type. It's obviously fine to use seq in lots of other places, my comment is specifically about passing boolean props to components.
| (on-auth-success) | ||
| (js/console.log "response" response)) | ||
| :on-fail (fn [error] | ||
| (js/console.log "Authentication Failed. Error:" error) |
There was a problem hiding this comment.
console.log should be replaced by our logging library.
There was a problem hiding this comment.
this is timbre, right? will adjust 👍
| on-auth-fail | ||
| size | ||
| theme] | ||
| :or {on-auth-success #() on-auth-fail #()}}] |
There was a problem hiding this comment.
It's a bit weird to define empty anonymous functions like this. This might be a sign that the code using these functions in authorize is not checking for function existence, which is usually recommended, otherwise it's too easy to have an exception.
In the authorize function, we can change it to check the callback functions exist before calling them. The pattern is (when on-auth-success (on-auth-success)). Also, the default destructuring doesn't protect the code from the caller of view explicitly passing nil as the calllback (which can be surprisingly easy to happen in Clojure).
| :or {on-auth-success #() on-auth-fail #()}}] | ||
| (let [reset-slider? (reagent/atom false) | ||
| on-close #(reset! reset-slider? true)] | ||
| (fn [] |
There was a problem hiding this comment.
You need to define the renderer arguments here, otherwise if they change the component won't re-render.
| (reset! code connection-string))) | ||
| clock (fn [] | ||
| (if (pos? (- code-valid-for-ms | ||
| (- (* 1000 (js/Math.ceil (/ (datetime/timestamp) 1000))) |
There was a problem hiding this comment.
This repeated rounding mechanism in the code should ideally be in utils.number. You can try the utils.number/naive-round and see if it helps. The rounding mechanism is different, but you can round up to a certain number of decimal places too. The code would look like (utils.number/naive-round (datetime/timestamp) 3)
There was a problem hiding this comment.
I will take a look, but slightly outside the scope of this pr as this code was there from before, albeit I am the one who added it 🙃
| (rf/dispatch [:set-in [:profile/login :key-uid] key-uid]) | ||
| (rf/dispatch | ||
| [:syncing/get-connection-string-for-bootstrapping-another-device | ||
| password set-code]))] | ||
| (fn [] |
There was a problem hiding this comment.
There was a problem hiding this comment.
but the outer function here also has no params? 🤔
There was a problem hiding this comment.
You are 100% correct. My bad :)
J-Son89
force-pushed
the
standard-auth-process
branch
from
3ab5de2
to
f9f3a7f
Compare
| (on-valid-connection-string response) | ||
| (rf/dispatch [:syncing/update-role constants/local-pairing-role-sender]) | ||
| (rf/dispatch [:hide-bottom-sheet])) | ||
| {:db (update db |
There was a problem hiding this comment.
@ilmotta @cammellos I am trying to get the error message show on the input when the user puts in a wrong password.
in the case of syncing setup this seems to be the right place but this approach isn't working.
Any thoughts how I can/should do this? 🤔
| @@ -0,0 +1,6 @@ | |||
| (ns status-im2.common.standard-authentication.forgot-password-doc.style) | |||
|
|
|||
| (def forget-password-doc-container {:margin-right 16}) | |||
There was a problem hiding this comment.
this doesn't make sense, the name is longer than the style map itself, forget-password is redundant in the name, and by guidelines it's allowed to still have short styles in the view
There was a problem hiding this comment.
yep, I just copied this code over. These long names are a symptom that the code should have been initially in its own namespace. Anyway, you're right, I will adjust it 👌
J-Son89
force-pushed
the
standard-auth-process
branch
2 times, most recently
from
679f062
to
50625c5
Compare
flexsurfer
changed the title
| (defn view | ||
| [{:keys [shell?]}] | ||
| [quo/documentation-drawers | ||
| {:title (i18n/label :t/forgot-your-password-info-title) | ||
| :shell? shell?} | ||
| [rn/view | ||
| {:style style/container} | ||
| [quo/text {:size :paragraph-2} (i18n/label :t/forgot-your-password-info-description)] | ||
|
|
||
| [rn/view {:style style/step-container} | ||
| [quo/step {:in-blur-view? shell?} 1] | ||
| [rn/view | ||
| {:style style/step-content} | ||
| [quo/text {:size :paragraph-2 :weight :semi-bold} | ||
| (i18n/label :t/forgot-your-password-info-remove-app)] | ||
| [quo/text {:size :paragraph-2} (i18n/label :t/forgot-your-password-info-remove-app-description)]]] | ||
|
|
||
| [rn/view {:style style/step-container} | ||
| [quo/step {:in-blur-view? shell?} 2] | ||
| [rn/view | ||
| {:style style/step-content} | ||
| [quo/text {:size :paragraph-2 :weight :semi-bold} | ||
| (i18n/label :t/forgot-your-password-info-reinstall-app)] | ||
| [quo/text {:size :paragraph-2} | ||
| (i18n/label :t/forgot-your-password-info-reinstall-app-description)]]] | ||
|
|
||
| [rn/view {:style style/step-container} | ||
| [quo/step {:in-blur-view? shell?} 3] | ||
| [rn/view | ||
| {:style style/step-content} | ||
| [rn/view | ||
| {:style style/step-title} | ||
| [quo/text {:size :paragraph-2} (str (i18n/label :t/sign-up) " ")] | ||
| [quo/text {:size :paragraph-2 :weight :semi-bold} | ||
| (i18n/label :t/forgot-your-password-info-signup-with-key)]] | ||
| [quo/text {:size :paragraph-2} | ||
| (i18n/label :t/forgot-your-password-info-signup-with-key-description)]]] | ||
|
|
||
| [rn/view {:style style/step-container} | ||
| [quo/step {:in-blur-view? shell?} 4] | ||
| [rn/view | ||
| {:style style/step-content} | ||
| [quo/text {:size :paragraph-2 :weight :semi-bold} | ||
| (i18n/label :t/forgot-your-password-info-create-new-password)] | ||
| [quo/text {:size :paragraph-2} | ||
| (i18n/label :t/forgot-your-password-info-create-new-password-description)]]]]]) |
There was a problem hiding this comment.
Can we break up this component a little? Also I see some repeated code style/step-container is repeated 4 times. Probably can be extracted to a one defined component
There was a problem hiding this comment.
I just moved this code to a common location. The main focus of this pr is to enable a standard authentication process in the pr. I am happy to make adjustments but would prefer for pieces like this to do in a follow up.
| :size :default | ||
| :icon :i/info} | ||
| error-message] | ||
| [rn/touchable-opacity |
| set-code (fn [connection-string] | ||
| (when (sync-utils/valid-connection-string? connection-string) | ||
| (reset! timestamp (* 1000 | ||
| (js/Math.ceil (/ (datetime/timestamp) | ||
| 1000)))) | ||
| (reset! delay 1000) | ||
| (reset! code connection-string))) | ||
| clock (fn [] | ||
| (if (pos? (- code-valid-for-ms | ||
| (- (* 1000 | ||
| (js/Math.ceil (/ (datetime/timestamp) 1000))) | ||
| @timestamp))) | ||
| (swap! valid-for-ms (fn [_] | ||
| (- code-valid-for-ms | ||
| (- (* 1000 | ||
| (js/Math.ceil | ||
| (/ (datetime/timestamp) 1000))) | ||
| @timestamp)))) | ||
| (reset! delay nil))) |
There was a problem hiding this comment.
This part is hard to read and understand. Maybe extract to utils file and break it up into smaller functions if possible
There was a problem hiding this comment.
sure, this was previously here so I will create a follow up to address this. Also it seems this code will be better as being dispatched as it will break the need for so many nested callbacks
| :track-text (i18n/label :t/slide-to-reveal-code) | ||
| :customization-color customization-color | ||
| :on-enter-password on-enter-password | ||
| :biometric-auth? false |
There was a problem hiding this comment.
Quick question: Do we support biometric authentication?
There was a problem hiding this comment.
not yet, but we will! :) (at least for standardized auth anyway)
| (rf/defn verify-database-password | ||
| {:events [:profile.login/verify-database-password]} | ||
| [{:keys [db] :as cofx} entered-password] | ||
| (let [hashed-password (-> entered-password | ||
| security/safe-unmask-data | ||
| ethereum/sha3) | ||
| key-uid (get-in db [:profile/login :key-uid]) | ||
| valid? (native-module/verify-database-password | ||
| key-uid | ||
| hashed-password | ||
| (fn [response] | ||
| (let [valid? (string/blank? (.-error (js/JSON.parse response)))] | ||
| (rf/dispatch [:profile.login/verified-database-password valid?]))))])) |
There was a problem hiding this comment.
| (rf/defn verify-database-password | |
| {:events [:profile.login/verify-database-password]} | |
| [{:keys [db] :as cofx} entered-password] | |
| (let [hashed-password (-> entered-password | |
| security/safe-unmask-data | |
| ethereum/sha3) | |
| key-uid (get-in db [:profile/login :key-uid]) | |
| valid? (native-module/verify-database-password | |
| key-uid | |
| hashed-password | |
| (fn [response] | |
| (let [valid? (string/blank? (.-error (js/JSON.parse response)))] | |
| (rf/dispatch [:profile.login/verified-database-password valid?]))))])) | |
| (rf/defn verify-database-password | |
| {:events [:profile.login/verify-database-password]} | |
| [{:keys [db]} entered-password] | |
| (let [hashed-password (-> entered-password | |
| security/safe-unmask-data | |
| ethereum/sha3) | |
| key-uid (get-in db [:profile/login :key-uid]) | |
| callback (fn [response] | |
| (let [valid? (string/blank? (.-error (js/JSON.parse response)))] | |
| (rf/dispatch [:profile.login/verified-database-password valid?])))] | |
| (native-module/verify-database-password | |
| key-uid | |
| hashed-password | |
| callback))) |
There was a problem hiding this comment.
will adjust, thanks @smohamedjavid!
| :profile/login | ||
| #(-> % | ||
| (dissoc :processing) | ||
| (assoc :error "Invalid password")))})) |
There was a problem hiding this comment.
I believe we should use (i18n/label :t/oops-wrong-password)
There was a problem hiding this comment.
sure, I can make that adjustment. This is just the error message passed to the db here. There is some weird hack on the display of actually showing that. Actually I wonder are we okay with adding i18n messages to the db? @cammellos is this okay for us or should we keep it english only in the db? 🤔
There was a problem hiding this comment.
Just raised that comment from a UX standpoint. Because we can't display English if a user uses the app in a different language. 👍
(There are quite a few usages of i18n in reframe event handlers in our codebase. Not sure if it's allowed)
There was a problem hiding this comment.
yep, this is currently how the password error message is working 😶
I just moved that code but I believe this should have a stronger mechanism - and I think better as you are saying the error message ideally is just set appropriately where the error originates.
There was a problem hiding this comment.
Maybe the best is to treat the translated text as part of the view layer, and the translation keys the data that can be persisted in the app db. This way, the db layer stays translation agnostic. Not a big deal though, but seems better, also facilitates unit testing.
|
|
||
|
|
| @@ -82,17 +81,11 @@ | |||
| (h/has-style track-mock {:opacity constants/disable-opacity}))) | |||
|
|
|||
| (h/test "render the small button" | |||
| (h/render [slide-button/view (assoc default-props :size :small)]) | |||
| (h/render [slide-button/view (assoc default-props :size :size/s-40)]) | |||
There was a problem hiding this comment.
@J-Son89, quick question, is the decision to use a qualified key to describe sizes captured somewhere in our guidelines? Or is this just a single example and there's no clear recommendation yet?
There was a problem hiding this comment.
the decision was agreed upon within internal communications channels. It should be added to the docs. I guess we wanted to use it once or twice to make sure we have it right before adding it to the docs. I'll see if someone can add it (if not I will 👍 )
btw, Ulises made a good point and I was curious of the difference, why not :size-40 instead of :size/s-40? does the / really give us some benefit?
| :icon-left :i/reveal | ||
| :disabled? (or (not sign-in-enabled?) processing) | ||
| :on-press (fn [] | ||
| (rf/dispatch [:set-in [:profile/login :key-uid] key-uid]) |
There was a problem hiding this comment.
Just a story...
I've seen this set-in pattern in more than one re-frame project as a way of helping reduce re-frame boilerplate. Numerous times I've seen it causing UI glitches because the UI would dispatch multiple set-in events in succession and they would end up in different rendering batches in the re-frame queue. The more reliable solution is to mutate the app db alongside other effects in the same event handler, like an atomic operation.
The re-frame docs explicitly tell us re-frame can't guarantee the order of events between different dispatches, so this is also another problem of using this set-in pattern, some devs will incorrectly assume the setter happens first.
The set-in pattern also tends to couple the UI with the app db shape because the UI now needs to know how the data is structured behind the scenes. Some re-frame projects even go as far as using a get-in subscription, as yet again, a way to make accessing the app db more convenient from the UI. And things don't end there, some projects create other "wrapper events" around the core app db atom, like assoc-in, update-in, and so on. All bad for maintainability in my experience.
End of story :)
FYI: @ulisesmac, @cammellos, @flexsurfer, @erikseppanen, @smohamedjavid, @J-Son89
There was a problem hiding this comment.
Thanks @ilmotta, if there is some guidance on what is best to do I will be happy to implement. :)
There was a problem hiding this comment.
I feel this is part of a much bigger discussion and it would be good if we could organise some initiative to address this and agree on solid and consistent approach 👍
There was a problem hiding this comment.
At this stage I just wanted to raise awareness @J-Son89 and I agree with you, it's not something for your PR alone to change or decide.
The following is a known post about some patterns seen in re-frame apps. Note that some ideas existed in redux apps before re-frame was a thing, so maybe finding guidance on redux can be more fruitful.
There was a problem hiding this comment.
yes :set-in is a legacy code, we even have TODO
;;TODO :replace by named events
(rf/defn set-event
{:events [:set]}
[{:keys [db]} k v]
{:db (assoc db k v)})
;;TODO :replace by named events
(rf/defn set-once-event
{:events [:set-once]}
[{:keys [db]} k v]
(when-not (get db k)
{:db (assoc db k v)}))
;;TODO :replace by named events
(rf/defn set-in-event
{:events [:set-in]}
[{:keys [db]} path v]
{:db (assoc-in db path v)})but it should be more visible i guess, so devs won't use it
J-Son89
force-pushed
the
standard-auth-process
branch
from
50625c5
to
fcbcc52
Compare
qfrank
force-pushed
the
standard-auth-process
branch
from
5f2479e
to
a03b4e3
Compare
done @pavloburykh |
77% of end-end tests have passedFailed tests (10)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (33)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Thank you for your work @J-Son89 @qfrank! PR is ready for design review. NOTE for design review:The following issues are know and will be fixed in separately: ISSUE 1 Biometric auth is not implementedcomment form @J-Son89
ISSUE 2 Biometric icon appears when sliding to the endcomment form @J-Son89
ISSUE 3 Placeholder text partially remains visible for a moment behind the slidercomment form @J-Son89
|
J-Son89
force-pushed
the
standard-auth-process
branch
from
a03b4e3
to
e9d4a83
Compare
J-Son89
force-pushed
the
standard-auth-process
branch
from
e9d4a83
to
8bf40f8
Compare
49% of end-end tests have passedFailed tests (22)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (21)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
@J-Son89 e2e fails are not PR related. Ready for merge. |
* chore: move password input to connected component --------- Co-authored-by: Jamie Caprani <jamiecaprani@gmail.com> Co-authored-by: frank <lovefree103@gmail.com>
* chore: move password input to connected component --------- Co-authored-by: Jamie Caprani <jamiecaprani@gmail.com> Co-authored-by: frank <lovefree103@gmail.com>
fixes #15570
Designs: https://www.figma.com/file/V6nlpAWIf2e1XU8RJ9yQPe/Syncing-for-Mobile?type=design&node-id=1144-131149&mode=design&t=PDU0zL2HgIjUkEwX-0
Other use Designs: https://www.figma.com/file/HKncH4wnDwZDAhc4AryK8Y/Wallet-for-Mobile?type=design&node-id=1741-374592&mode=design&t=4WPq9m1hep9SXcv3-0
this PR was started by an external contributor, now maintained by @J-Son89
Summary
Adds Standardised In App authentication and uses this in Syncing Flows.
Caveats - the design for this is not 100% correct as the Slide Button is missing some variants and so a follow up issue will be created to implement this and update the designs.
I would consider this pr there to add the functionality mostly and design is 90% complete and will be finished in follow up issues as the changes go beyond the scope of this pr as they are related to issues with individual design system components.
Steps to test
Syncing > Slide to reveal > Enter Password