Page MenuHomePhabricator

[lib] Add new olm auth action types and temporary identity login action types
ClosedPublic

Authored by inka on Dec 21 2023, 3:53 AM.
Tags
None
Referenced Files
F3281783: D10424.diff
Sat, Nov 16, 10:13 AM
Unknown Object (File)
Thu, Nov 14, 12:31 PM
Unknown Object (File)
Mon, Nov 11, 8:39 AM
Unknown Object (File)
Fri, Nov 8, 1:06 AM
Unknown Object (File)
Thu, Nov 7, 7:11 PM
Unknown Object (File)
Sun, Nov 3, 12:21 PM
Unknown Object (File)
Fri, Nov 1, 7:37 PM
Unknown Object (File)
Fri, Oct 25, 7:38 PM
Subscribers

Details

Summary

issue: ENG-6018 and ENG-6015
We want to add temporary, fake login action type for identity login - this is because @varun will be calling the real identity login before I'm done with the refactor
We also want to add a keyserver auth action type. This will be an action called to login / register on the ks, using the new endpoint which verifies the user with identity.

The payloads will be updated. KeyserverAuthResult I think will stay the same, it differs from LogInResult only in not having currentUserInfo, since it doesn't make sense for the keyserver to send the user their user info - they will have already gotten it from identity.

I will update payloads for identity login in next diffs, as I figure out what is needed. Probably some changes to it will also be introdeuced by @varun later on

Test Plan

flow

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

@varun I thought about the name for keyserverAuthActionTypes - I remember there were a lot of different variations with words "identity", "olm", "auth", "login" when we were discussing all of this
I talked to @tomek, since he will be working on ENG-6081, and he noticed that:

  1. using identity will be the default in the future, so it doesn't necessarily make sense to put it in the name, it will also be confusing if a keyserver endpoint has "identity" in the name
  2. olm is an implementation detail
  3. auth makes more sense than login, because it is also a register endpoint

And I think that from client's perspective it's important to distinguish between identity and keyserver calls, so authActionTypes would be too arbitrary
So we think keyserverAuthActionTypes make the most sense. But I wanted to ask your opinion, since you have the most context

inka requested review of this revision.Dec 21 2023, 5:00 AM

A little confused why we need to introduce a "fake" login type. Why not introduce the real one, and have @varun start using it once he gets to it?

A little confused why we need to introduce a "fake" login type. Why not introduce the real one, and have @varun start using it once he gets to it?

@varun needs to call identity login before I'm done with this refactor. From what I understand, we want to be calling identity login/logout to check if it's working properly, without triggering changes in redux at first. @varun and I discussed this and together decided to create this fake action

Adding @varun as blocking because we have some repetition between this diff and D10490

From what I understand, we want to be calling identity login/logout to check if it's working properly, without triggering changes in redux at first.

Seems like we're introducing a "real" Redux action here, just we aren't adding any logic to process it to the reducers.

Seems like we're introducing a "real" Redux action here, just we aren't adding any logic to process it to the reducers.

But I need to implement the logic in reducers, before we are ready to reduce the actual action which we will already be dispatching

Understood – thanks for clarifying. So we'll have two actions – one that is being dispatched by @varun's code (the "real" action) and one that is being reduced by your code (the "fake" action). After things are ready, we'll replace all instances of the "fake" action with the "real" action. Does that sound right?

keyserverAuthActionTypes makes sense to me. i'll update D10490 and the rest of my stack to use that terminology

varun requested changes to this revision.Jan 4 2024, 12:08 AM

The payloads will be updated. KeyserverAuthResult I think will stay the same, it differs from LogInResult only in not having currentUserInfo, since it doesn't make sense for the keyserver to send the user their user info - they will have already gotten it from identity.

I agree it's unnecessary for the keyserver to send the user their id and username, but right now the identity service doesn't manage avatars or notif settings. I think we should either stick with LoginResult and return the user ID and username even though they're redundant OR modify KeyserverAuthResult to return only the avatar data and notif settings from currentUserInfo.

export type LoggedInUserInfo = {
  +id: string,
  +username: string,
  +settings?: DefaultNotificationPayload,
  +avatar?: ?ClientAvatar,
};
This revision now requires changes to proceed.Jan 4 2024, 12:08 AM

Understood – thanks for clarifying. So we'll have two actions – one that is being dispatched by @varun's code (the "real" action) and one that is being reduced by your code (the "fake" action). After things are ready, we'll replace all instances of the "fake" action with the "real" action. Does that sound right?

Yes, exactly!

keyserverAuthActionTypes makes sense to me. i'll update D10490 and the rest of my stack to use that terminology

Great, thank you!

I agree it's unnecessary for the keyserver to send the user their id and username, but right now the identity service doesn't manage avatars or notif settings. I think we should either stick with LoginResult and return the user ID and username even though they're redundant OR modify KeyserverAuthResult to return only the avatar data and notif settings from currentUserInfo.

According to this discussion avatars and notif settings will be handled by backup and tunnelbroker. But I don't think that's ready yet, so you are right.

We will just need to remember when updating the endpoint that some clients expect this data

Bring back currentUserInfo

But we should make all fields that use responses[ashoatKeyserverID] as optional, since the action may not call Ashoat's keyserver at all (See D10490)

This revision is now accepted and ready to land.Jan 5 2024, 9:57 AM

Rebase - use KeyserverAuthInfo

Fix - use KeyserverAuthResult