Page MenuHomePhabricator

[lib] validate policies after user login
AbandonedPublic

Authored by kamil on Dec 15 2022, 2:30 AM.
Tags
None
Referenced Files
F3246324: D5875.diff
Thu, Nov 14, 10:52 PM
Unknown Object (File)
Thu, Nov 7, 5:51 AM
Unknown Object (File)
Tue, Nov 5, 7:26 AM
Unknown Object (File)
Mon, Oct 28, 7:19 AM
Unknown Object (File)
Oct 14 2024, 4:29 PM
Unknown Object (File)
Oct 12 2024, 10:11 PM
Unknown Object (File)
Oct 4 2024, 12:54 PM
Unknown Object (File)
Sep 28 2024, 1:35 PM
Subscribers

Details

Reviewers
tomek
Summary

Diff introduces hook to check if there is information about not acknowledged policies in login response and dispatch an action if needed. We care only about logging in manually performed by the user.

Test Plan
  1. Logout user
  2. Set terms policy as not accepted for user in DB
  3. Login
  4. Login action should finish with success and modal should show immediately after it
  5. Test for both web/native

Diff Detail

Repository
rCOMM Comm
Branch
policy-stuff-5
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Dec 15 2022, 2:39 AM
tomek requested changes to this revision.Dec 15 2022, 6:56 AM
tomek added inline comments.
keyserver/src/responders/user-responders.js
294–300

Wondering if this is a good idea to return a lot of data (e.g. message infos) while the terms aren't accepted. It would be complicated to avoid that, but might be better from legal perspective (?) I'm thinking about the case where a user doesn't want to accept the terms but can receive all the new messages just by doing repeated login attempts. @ashoat what do you think?

This revision now requires changes to proceed.Dec 15 2022, 6:56 AM
keyserver/src/responders/user-responders.js
294–300

I don't think we should send any data here in that scenario. Is the reason that we can't we just throw new ServerError because we want to allow the login? I know we moved away from the "force log out" approach, but I'm not sure that means we have to allow new logins.

keyserver/src/responders/user-responders.js
298

I think we should send a response here that has empty objects/array, plus serverTime: 0.

This will work fine for messages, since those are persisted forever on the keyserver. So after the login, when the client connects to the socket, the client will ask for all messages newer than 0, and the keyserver will deliver those messages (with some limit).

However, this won't work well for the data types that are kept updated via UpdateInfo. This is the other checkpointing mechanism in the app (other than the checkpointing for messages), but it's different because we delete UpdateInfos, and we don't have a perfect history of eg. every calendar entry that has ever been created. So if the client asks the keyserver for all UpdateInfos newer than 0, it won't be enough to get the latest state.

Instead, we should force the socket to go into this code branch, where it will be forced to fetch all of that data from scratch. To do that, in addition to returning an empty response here, we should make sure to delete the session (but not the cookie).

To delete the session, we should first introduce a new function to keyserver/src/deleters/session-deleters.js that deletes a specific session based a specific session ID. Then we should call it here.

We might also need to update the viewer update here to have the correct info about the session, ie. unset sessionID and sessionInfo. This will need some changes to the Viewer class, either to make it possible to pass null to setSessionID and setSessionInfo, or maybe a better solution would be a new function eg. clearSession.

kamil added inline comments.
keyserver/src/responders/user-responders.js
298

Not sending any data was a little bit trickier, I described this later in the stack and suggested solutions in diffs: D6092, D6093, and D6094.

Unfortunately after creating the diffs, I found it's even more complicated, I described the problem in: https://phab.comm.dev/D6093#182799