Details

Reviewers

inka
• przemek
michal
ashoat
atul

Commits

rCOMM0389fd22a3c2: [lib] validate policies after user login

Summary

Follow up to: https://phab.comm.dev/D5875#inline-39619.

Diff introduces a hook to check if there is information about not acknowledged policies in login response and dispatch an action if needed.
Additionally, it removes data from the response to not share data with user until acceptance.

Probably I'll wait with landing this until most of the SIWE feature will be ready to test end-to-end how this change works with login using wallet.

Test Plan

Logout user
Set terms policy as not accepted for user in DB
Login with credentials and separately using SIWE
Login action should finish with success and the modal should show immediately after it. Data about user messages, threads, calendar, etc. should not be present in the login response.
Test for both web/native

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Dec 29 2022, 4:24 AM

kamil held this revision as a draft.

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 29 2022, 4:24 AM

Harbormaster failed remote builds in B14906: Diff 20323!Dec 29 2022, 4:28 AM

kamil added a parent revision: D6093: [keyserver] disable socket connection when policies are not accepted.Dec 29 2022, 4:29 AM

kamil removed a parent revision: D6093: [keyserver] disable socket connection when policies are not accepted.

kamil added a child revision: D6093: [keyserver] disable socket connection when policies are not accepted.Dec 29 2022, 4:32 AM

kamil added a parent revision: D6091: [keyserver] move logic to fetch not acknowledged policies to separate function.

fix TrucationStatuses type

Harbormaster failed remote builds in B14911: Diff 20328!Dec 29 2022, 4:44 AM

kamil edited the summary of this revision. (Show Details)Dec 29 2022, 4:55 AM

kamil edited the test plan for this revision. (Show Details)

kamil published this revision for review.Dec 29 2022, 5:23 AM

kamil mentioned this in D5875: [lib] validate policies after user login.

ashoat added a reviewer: ashoat.Dec 29 2022, 8:31 AM

We care only about logging in manually performed by the user.

Can you remind me why that is?

lib/hooks/log-in-call.js
11 ↗	(On Diff #20328)	It's weird that we're wrapping a function that calls `dispatch` with `dispatchActionPromise`. Instead, can we rely on the actions dispatched by `dispatchActionPromise`? Eg. `LOG_IN_SUCCESS`'s payload can be modified in include the `result.notAcknowledgedPolicies` data, and we can use that action to do whatever you were doing with the new action you introduced

This revision now requires changes to proceed.Dec 29 2022, 9:10 AM

rely on LOG_IN_SUCCESS action

In D6092#183060, @ashoat wrote:

We care only about logging in manually performed by the user.

Can you remind me why that is?

That was a bad assumption from my side, sorry.

Right now when we rely on LOG_IN_SUCCESS we care about any log-in action - and if there are no accepted policies we save information in redux.

EDIT: tested for SIWE

Harbormaster completed remote builds in B15293: Diff 20855.Jan 12 2023, 5:59 AM

ashoat requested changes to this revision.Jan 12 2023, 7:50 AM

ashoat added a reviewer: atul.

ashoat added inline comments.

keyserver/src/responders/user-responders.js
230 ↗	(On Diff #20855)	Why are we awaiting this sequentially? This is bad for performance. We should never await sequentially unless the data flow forces us to. Did you notice there was a `Promise.all` directly above this line? Try to understand why that is there, and how we use `Promise.all` in the codebase
lib/actions/user-actions.js
150 ↗	(On Diff #20855)	I think we need to make the same changes for the SIWE responder
lib/reducers/policies-reducer.js
20 ↗	(On Diff #20855)	I think we need to consider other types here, such as `siweAuthActionTypes` (cc @atul)
lib/types/account-types.js
122–134 ↗	(On Diff #20855)	Can we update this object and make everything `$ReadOnly`? Whenever we touch something, we should try to update it to the latest "best practice"

This revision now requires changes to proceed.Jan 12 2023, 7:50 AM

wrap promise inside Promise.all, add notAcknowledgedPolicies to SIWE responder, update LogInResponse fields to be read only

kamil added inline comments.Jan 20 2023, 5:38 AM

keyserver/src/responders/user-responders.js
286 ↗	(On Diff #21143)	this change was forced by https://phab.comm.dev/D6092?id=20855#inline-41857
230 ↗	(On Diff #20855)	My bad, fixed
lib/actions/user-actions.js
150 ↗	(On Diff #20855)	Done, tested and after SIWE login there are only empty arrays/object sent from keyserver
lib/reducers/policies-reducer.js
20 ↗	(On Diff #20855)	Good call, I think it needs to be done. Added and tested, after the SIWE login success this code is executed
lib/types/account-types.js
122–134 ↗	(On Diff #20855)	sure, updated

Harbormaster completed remote builds in B15499: Diff 21143.Jan 20 2023, 5:47 AM

This is close!! Some minor comments / questions below. Feel free to re-request review if you're not sure about the setNewSession question

keyserver/src/responders/user-responders.js
233 ↗	(On Diff #21143)	This will need to be changed before the next release. Can you make sure that happens, eg. take the lead on coordinating with @atul about it?
251–253 ↗	(On Diff #21143)	Are you sure we should skip this line? Do we need to set `serverTime` to 0?
286 ↗	(On Diff #21143)	We should avoid setting `rawEntryInfos` if it doesn't exist. Try this: let objWithoutField = { // stuff here }; if (newFieldToAdd) { objectWithoutField = { ...objectWithoutField newFieldToAdd, }; }

This revision is now accepted and ready to land.Jan 20 2023, 10:48 AM

avoid setting field if it not exists

re-requesting review to confirm my understanding of setNewSession is correct

keyserver/src/responders/user-responders.js
233 ↗	(On Diff #21143)	Sure! Task to make sure: https://linear.app/comm/issue/ENG-2481/set-mincodeversion-for-policies-after-release
251–253 ↗	(On Diff #21143)	I think we can skip this, we are unable to open the socket connection when policies are not accepted (D6093), so after accepting new session should be created (client has `serverTime: 0` so initializeSession should handle this and as an effect make sure this code branch will execute. If there is no socket connection user should be logged in again (D6250) so also propper session will be created.
286 ↗	(On Diff #21143)	Can you say why is that? I know there are some differences e.g. when it comes to using `hasOwnProperty()` but in this particular scenario, I think an empty array will make a change, but not setting/setting as undefined not really. I am not saying I don't agree (I updated code), just curious about the reasoning.

Harbormaster completed remote builds in B15643: Diff 21325.Jan 25 2023, 9:49 AM

Okay, seems reasonable

keyserver/src/responders/user-responders.js
286 ↗	(On Diff #21143)	It's not a big deal, I'd just prefer to avoid sending something across the wire if it's not needed

This revision is now accepted and ready to land.Jan 25 2023, 12:44 PM

rebase before landing, base policy fetchers on viewer.id instead of entire object

Harbormaster completed remote builds in B15656: Diff 21349.Jan 26 2023, 4:22 AM

Closed by commit rCOMM0389fd22a3c2: [lib] validate policies after user login (authored by kamil). · Explain WhyJan 26 2023, 4:29 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM0389fd22a3c2: [lib] validate policies after user login.

[lib] validate policies after user login
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 21353

keyserver/src/endpoints.js

keyserver/src/fetchers/policy-acknowledgment-fetchers.js

keyserver/src/responders/user-responders.js

keyserver/src/utils/validation-utils.js

lib/actions/siwe-actions.js

lib/actions/user-actions.js

lib/facts/policies.js

lib/reducers/policies-reducer.js

lib/types/account-types.js

[lib] validate policies after user loginClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 21353

keyserver/src/endpoints.js

keyserver/src/fetchers/policy-acknowledgment-fetchers.js

keyserver/src/responders/user-responders.js

keyserver/src/utils/validation-utils.js

lib/actions/siwe-actions.js

lib/actions/user-actions.js

lib/facts/policies.js

lib/reducers/policies-reducer.js

lib/types/account-types.js

[lib] validate policies after user login
ClosedPublic
Actions

Revision Contents
Changeset List