Page MenuHomePhabricator
Differential D10492

[keyserver] processOlmAccountCreation
ClosedPublic
Actions

Authored by varun on Dec 29 2023, 8:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 29, 9:46 PM
Unknown Object (File)
Fri, Mar 28, 9:24 AM
Unknown Object (File)
Sun, Mar 23, 12:58 AM
Unknown Object (File)
Wed, Mar 19, 2:02 AM
Unknown Object (File)
Tue, Mar 18, 8:37 AM
Unknown Object (File)
Tue, Mar 18, 8:37 AM
Unknown Object (File)
Tue, Mar 18, 8:37 AM
Unknown Object (File)
Tue, Mar 18, 8:37 AM
View All Files
Subscribers
tomek

Details

Reviewers
ashoat
atul
Commits
rCOMM26a1b6eddb80: [keyserver] processOlmAccountCreation
Summary

similar to SIWE account creation logic. differences:

  • signedIdentityKeysBlob must be present
  • we use the UserIdentifier type to get the username (and eth address if present)
  • we don't send a reserved username request since this user has already registered with identity

not deduping because we eventually want to deprecate the siwe account creation function anyway

Depends on D10491

Test Plan

tested in next diff by calling the olm auth responder and confirming that the above listed changes all work as expected

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Dec 29 2023, 8:43 AM
Herald added a subscriber: tomek. · View Herald TranscriptDec 29 2023, 8:43 AM
varun added a child revision: D10493: [keyserver] olm auth responder.Dec 29 2023, 8:48 AM
varun published this revision for review.Dec 29 2023, 8:51 AM
Harbormaster completed remote builds in B25403: Diff 35101.Dec 29 2023, 9:01 AM
ashoat added inline comments.Dec 29 2023, 10:59 AM
keyserver/src/creators/account-creator.js
323 ↗(On Diff #35101)

We seem to be allowing the user to pick any username here. Instead, we should ask the identity service for the username, to prevent the user from picking a username that doesn't match the identity service's records for the given userID

ashoat requested changes to this revision.Dec 29 2023, 11:07 AM

not deduping because we eventually want to deprecate the siwe account creation function anyway

I don't love this reasoning. This code will be around for a while with duplication. It seems like at least some of the shared code can be factored out here, especially the stuff at the end. Can you try to factor it out?

This revision now requires changes to proceed.Dec 29 2023, 11:07 AM
varun updated this revision to Diff 35327.Jan 5 2024, 10:00 PM

checking delta between two account creation functions

Harbormaster completed remote builds in B25561: Diff 35327.Jan 5 2024, 10:24 PM
varun added inline comments.Jan 5 2024, 10:34 PM
keyserver/src/creators/account-creator.js
323 ↗(On Diff #35101)

the caller of this function will pass the username retrieved from the identity service

varun updated this revision to Diff 35330.Jan 5 2024, 10:35 PM

dedup

Harbormaster completed remote builds in B25564: Diff 35330.Jan 5 2024, 10:59 PM
varun added inline comments.Jan 5 2024, 11:16 PM
keyserver/src/creators/account-creator.js
15 ↗(On Diff #35330)

not sure what happened here but i'll make sure to add the .js suffix back

300–302 ↗(On Diff #35330)

not sure if we need to block on setNewSession but that's what we were doing in the existing code so i haven't changed it

ashoat accepted this revision.Jan 6 2024, 1:29 PM
ashoat added inline comments.
keyserver/src/creators/account-creator.js
300–302 ↗(On Diff #35330)

setNewSession mutates viewer, so yeah probably best to await on it

305 ↗(On Diff #35330)

You could just pass the viewer in only, and use viewer.userID

lib/types/account-types.js
175–178 ↗(On Diff #35330)

To be honest, I don't think there's much of a point to this new type. It could be collapsed into ProcessOLMAccountCreationRequest to remove a layer of indirection

This revision is now accepted and ready to land.Jan 6 2024, 1:29 PM
Closed by commit rCOMM26a1b6eddb80: [keyserver] processOlmAccountCreation (authored by varun). · Explain WhyJan 9 2024, 9:31 PM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMM26a1b6eddb80: [keyserver] processOlmAccountCreation.

Revision Contents
Changeset List

PathSize
keyserver/
src/
creators/
94 lines
lib/
types/
5 lines

Diff 35327

View Options

keyserver/src/creators/account-creator.js

// @flow // @flow
import { addCommunityActionType } from '../actions/community-actions.js'; import { addCommunityActionType } from '../actions/community-actions.js';
import { siweAuthActionTypes } from '../actions/siwe-actions.js';
import {
logInActionTypes,
keyserverRegisterActionTypes,
} from '../actions/user-actions.js';
import { import {
communityStoreOpsHandlers, communityStoreOpsHandlers,
type CommunityStoreOperation, type CommunityStoreOperation,
type ReplaceCommunityOperation, type ReplaceCommunityOperation,
type RemoveAllCommunitiesOperation,
} from '../ops/community-store-ops.js'; } from '../ops/community-store-ops.js';
import type { CommunityStore } from '../types/community-types.js'; import type {
CommunityStore,
CommunityInfo,
} from '../types/community-types.js';
import { defaultEnabledApps } from '../types/enabled-apps.js';
import type { BaseAction } from '../types/redux-types'; import type { BaseAction } from '../types/redux-types';
import type { RawThreadInfos } from '../types/thread-types';
import { threadTypeIsCommunityRoot } from '../types/thread-types-enum.js';
const { processStoreOperations: processStoreOps } = communityStoreOpsHandlers; const { processStoreOperations: processStoreOps } = communityStoreOpsHandlers;
export default function reduceCommunityStore( export default function reduceCommunityStore(
state: CommunityStore, state: CommunityStore,
action: BaseAction, action: BaseAction,
newThreadInfos: RawThreadInfos,
): { ): {
communityStore: CommunityStore, communityStore: CommunityStore,
communityStoreOperations: $ReadOnlyArray<CommunityStoreOperation>, communityStoreOperations: $ReadOnlyArray<CommunityStoreOperation>,
} { } {
if (action.type === addCommunityActionType) { if (
action.type === logInActionTypes.success ||
action.type === siweAuthActionTypes.success ||
action.type === keyserverRegisterActionTypes.success
) {
kamilUnsubmitted
Not Done
Inline Actions

wondering, shouldn't we also add action.type === fullStateSyncActionType?

kamil: wondering, shouldn't we also add `action.type === fullStateSyncActionType`?
ginsuAuthorUnsubmitted
Done
Inline Actions

I spoke to @atul about this and from what I understood we don't need this type since the community store is not synced with the keyserver

ginsu: I spoke to @atul about this and from what I understood we don't need this type since the…
kamilUnsubmitted
Not Done
Inline Actions

But based on the code below, communities are computed based on newThreadInfos which are synced - see here (cc. @atul)

I am afraid that on fullStateSyncActionType we will have inconsistency between threads (threadTypeIsCommunityRoot) and communities in this store

kamil: But based on the code below, communities are computed based on `newThreadInfos` which are…
const removeAllOpertation: RemoveAllCommunitiesOperation = {
type: 'remove_all_communities',
};
const replaceOperations = Object.keys(newThreadInfos)
.filter(threadID =>
threadTypeIsCommunityRoot(newThreadInfos[threadID].type),
)
.map(threadID => {
const communityInfo: CommunityInfo = {
enabledApps: defaultEnabledApps,
};
return {
type: 'replace_community',
payload: {
id: threadID,
communityInfo,
},
};
});
atulUnsubmitted
Not Done
Inline Actions

You could do something like following:

const replaceOperations = Object.keys(newThreadInfos).reduce((acc, threadID) => {
  if (threadTypeIsCommunityRoot(newThreadInfos[threadID].type)) {
    const communityInfo = {
      enabledApps: defaultEnabledApps,
    };
    acc.push({
      type: 'replace_community',
      payload: {
        id: threadID,
        communityInfo,
      },
    });
  }
  return acc;
}, []);

(generated with ChatGPT)

that lets us avoid an iteration, but probably doesn't make a difference

atul: You could do something like following: ```lang=js const replaceOperations = Object.keys…
const commmunityStoreOperations = [
removeAllOpertation,
...replaceOperations,
];
return {
communityStore: processStoreOps(state, commmunityStoreOperations),
communityStoreOperations: commmunityStoreOperations,
};
} else if (action.type === addCommunityActionType) {
const replaceOperation: ReplaceCommunityOperation = { const replaceOperation: ReplaceCommunityOperation = {
type: 'replace_community', type: 'replace_community',
payload: { payload: {
id: action.payload.id, id: action.payload.id,
communityInfo: { communityInfo: {
...action.payload.newCommunityInfo, ...action.payload.newCommunityInfo,
}, },
}, },
Show All 13 Lines
View Options

lib/types/account-types.js

// @flow // @flow
import { addCommunityActionType } from '../actions/community-actions.js'; import { addCommunityActionType } from '../actions/community-actions.js';
import { siweAuthActionTypes } from '../actions/siwe-actions.js';
import {
logInActionTypes,
keyserverRegisterActionTypes,
} from '../actions/user-actions.js';
import { import {
communityStoreOpsHandlers, communityStoreOpsHandlers,
type CommunityStoreOperation, type CommunityStoreOperation,
type ReplaceCommunityOperation, type ReplaceCommunityOperation,
type RemoveAllCommunitiesOperation,
} from '../ops/community-store-ops.js'; } from '../ops/community-store-ops.js';
import type { CommunityStore } from '../types/community-types.js'; import type {
CommunityStore,
CommunityInfo,
} from '../types/community-types.js';
import { defaultEnabledApps } from '../types/enabled-apps.js';
import type { BaseAction } from '../types/redux-types'; import type { BaseAction } from '../types/redux-types';
import type { RawThreadInfos } from '../types/thread-types';
import { threadTypeIsCommunityRoot } from '../types/thread-types-enum.js';
const { processStoreOperations: processStoreOps } = communityStoreOpsHandlers; const { processStoreOperations: processStoreOps } = communityStoreOpsHandlers;
export default function reduceCommunityStore( export default function reduceCommunityStore(
state: CommunityStore, state: CommunityStore,
action: BaseAction, action: BaseAction,
newThreadInfos: RawThreadInfos,
): { ): {
communityStore: CommunityStore, communityStore: CommunityStore,
communityStoreOperations: $ReadOnlyArray<CommunityStoreOperation>, communityStoreOperations: $ReadOnlyArray<CommunityStoreOperation>,
} { } {
if (action.type === addCommunityActionType) { if (
action.type === logInActionTypes.success ||
action.type === siweAuthActionTypes.success ||
action.type === keyserverRegisterActionTypes.success
) {
kamilUnsubmitted
Not Done
Inline Actions

wondering, shouldn't we also add action.type === fullStateSyncActionType?

kamil: wondering, shouldn't we also add `action.type === fullStateSyncActionType`?
ginsuAuthorUnsubmitted
Done
Inline Actions

I spoke to @atul about this and from what I understood we don't need this type since the community store is not synced with the keyserver

ginsu: I spoke to @atul about this and from what I understood we don't need this type since the…
kamilUnsubmitted
Not Done
Inline Actions

But based on the code below, communities are computed based on newThreadInfos which are synced - see here (cc. @atul)

I am afraid that on fullStateSyncActionType we will have inconsistency between threads (threadTypeIsCommunityRoot) and communities in this store

kamil: But based on the code below, communities are computed based on `newThreadInfos` which are…
const removeAllOpertation: RemoveAllCommunitiesOperation = {
type: 'remove_all_communities',
};
const replaceOperations = Object.keys(newThreadInfos)
.filter(threadID =>
threadTypeIsCommunityRoot(newThreadInfos[threadID].type),
)
.map(threadID => {
const communityInfo: CommunityInfo = {
enabledApps: defaultEnabledApps,
};
return {
type: 'replace_community',
payload: {
id: threadID,
communityInfo,
},
};
});
atulUnsubmitted
Not Done
Inline Actions

You could do something like following:

const replaceOperations = Object.keys(newThreadInfos).reduce((acc, threadID) => {
  if (threadTypeIsCommunityRoot(newThreadInfos[threadID].type)) {
    const communityInfo = {
      enabledApps: defaultEnabledApps,
    };
    acc.push({
      type: 'replace_community',
      payload: {
        id: threadID,
        communityInfo,
      },
    });
  }
  return acc;
}, []);

(generated with ChatGPT)

that lets us avoid an iteration, but probably doesn't make a difference

atul: You could do something like following: ```lang=js const replaceOperations = Object.keys…
const commmunityStoreOperations = [
removeAllOpertation,
...replaceOperations,
];
return {
communityStore: processStoreOps(state, commmunityStoreOperations),
communityStoreOperations: commmunityStoreOperations,
};
} else if (action.type === addCommunityActionType) {
const replaceOperation: ReplaceCommunityOperation = { const replaceOperation: ReplaceCommunityOperation = {
type: 'replace_community', type: 'replace_community',
payload: { payload: {
id: action.payload.id, id: action.payload.id,
communityInfo: { communityInfo: {
...action.payload.newCommunityInfo, ...action.payload.newCommunityInfo,
}, },
}, },
Show All 13 Lines