Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing user registration concurrency issues #2157

Closed
wants to merge 3 commits into from
Closed

Fixing user registration concurrency issues #2157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c6dd62
Fixing user registration concurrency issues
Dec 8, 2023
8be5986
Fixup
Dec 8, 2023
5fe1834
Fixup
Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions packages/backend/src/nest/connections-manager/connections-manager.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,11 +318,25 @@ export class ConnectionsManagerService extends EventEmitter implements OnModuleI
await this.launchCommunity(payload)
this.logger(`Created and launched community ${payload.id}`)
this.serverIoProvider.io.emit(SocketActionTypes.NEW_COMMUNITY, { id: payload.id })

if (!payload.ownerCsr || !payload.permsData) {
throw new Error("Owner CSR and PermsData required to create community!")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would just show "abnormal backend termination" js error in application. Maybe it should be a return instead and emitError?

}
// In launchCommunity, we initialize the StorageService and also
// the RegistrationService for the first time. And then now, once
// those are ready, we register the owner's certificate.
const cert = await this.registrationService.registerOwnerCertificate({ communityId: payload.id, userCsr: payload.ownerCsr, permsData: payload.permsData })
if (payload.certs) {
// Hacking, perhaps make certs.certificate optional
payload.certs.certificate = cert || ''
}
await this.localDbService.put(LocalDBKeys.COMMUNITY, payload)
}

public async launchCommunity(payload: InitCommunityPayload) {
this.logger('Launching community: peers:', payload.peers)
this.communityState = ServiceState.LAUNCHING

// Perhaps we should call this data something else, since we already have a Community type.
// It seems like InitCommunityPayload is a mix of various connection metadata.
const communityData: InitCommunityPayload = await this.localDbService.get(LocalDBKeys.COMMUNITY)
Expand Down Expand Up @@ -424,6 +438,7 @@ export class ConnectionsManagerService extends EventEmitter implements OnModuleI
this.serverIoProvider.io.emit(SocketActionTypes.PEER_DISCONNECTED, payload)
})
await this.storageService.init(_peerId)
await this.registrationService.init(this.storageService)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a recommended way to inject dependencies.

Please add StorageModule in Registration Module:
imports: [StorageModule]
And in RegistartionService add:
private readonly storageService: StorageService

this.logger('storage initialized')
}

Expand All @@ -444,10 +459,6 @@ export class ConnectionsManagerService extends EventEmitter implements OnModuleI
this.registrationService.on(RegistrationEvents.ERROR, payload => {
emitError(this.serverIoProvider.io, payload)
})
this.registrationService.on(RegistrationEvents.NEW_USER, async payload => {
await this.storageService?.saveCertificate(payload)
})

this.registrationService.on(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, payload => {
if (payload.id) {
this.storageService.resolveCsrReplicatedPromise(payload.id)
Expand Down
72 changes: 72 additions & 0 deletions packages/backend/src/nest/registration/registration.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import { issueCertificate, extractPendingCsrs } from './registration.functions'
import { jest } from '@jest/globals'
import { createTmpDir } from '../common/utils'
import { RegistrationEvents } from './registration.types'
import { EventEmitter } from 'events'
import { CertificatesStore } from '../storage/certificates/certificates.store'
import { create, IPFS } from 'ipfs-core'
import OrbitDB from 'orbit-db'
import { TestConfig } from '../const'

describe('RegistrationService', () => {
let module: TestingModule
Expand Down Expand Up @@ -180,4 +185,71 @@ describe('RegistrationService', () => {

expect(eventSpy).toHaveBeenCalledTimes(3)
})

const createOrbitDbInstance = async () => {
const ipfs: IPFS = await create()
// @ts-ignore
const orbitdb = await OrbitDB.createInstance(ipfs, {
directory: TestConfig.ORBIT_DB_DIR,
})

return { orbitdb, ipfs }
}

it('race', async () => {
let { orbitdb, ipfs } = await createOrbitDbInstance()
let store = new CertificatesStore(orbitdb)
let emitter = new EventEmitter()
await store.init(emitter)

registrationService.permsData = permsData
// @ts-ignore
registrationService.storageService = { certificatesStore: store }

registrationService.onModuleInit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing await


const userCsr = await createUserCsr({
nickname: 'alice',
commonName: 'nqnw4kc4c77fb47lk52m5l57h4tcxceo7ymxekfn7yh5m66t4jv2olad.onion',
peerId: 'Qmf3ySkYqLET9xtAtDzvAr5Pp3egK1H3C5iJAZm1SpLEp6',
dmPublicKey: 'testdmPublicKey',
signAlg: configCrypto.signAlg,
hashAlg: configCrypto.hashAlg,
})

const userCsr2 = await createUserCsr({
nickname: 'alice',
commonName: 'nnnnnnc4c77fb47lk52m5l57h4tcxceo7ymxekfn7yh5m66t4jv2olad.onion',
peerId: 'QmffffffqLET9xtAtDzvAr5Pp3egK1H3C5iJAZm1SpLEp6',
dmPublicKey: 'testdmPublicKey',
signAlg: configCrypto.signAlg,
hashAlg: configCrypto.hashAlg,
})

const certificates: string[] = []
let calls = 0

registrationService.on('new', (p: any) => {
console.log(p)
calls++
})

registrationService.emit(
RegistrationEvents.REGISTER_USER_CERTIFICATE,
{ csrs: [userCsr.userCsr], certificates: [], id: '1' }
)

registrationService.emit(
RegistrationEvents.REGISTER_USER_CERTIFICATE,
{ csrs: [userCsr2.userCsr], certificates: [], id: '2' }
)

await new Promise(r => setTimeout(r, 10000))

await store.close()
await orbitdb.stop()
await ipfs.stop()

expect(calls).toEqual(1)
})
})
118 changes: 91 additions & 27 deletions packages/backend/src/nest/registration/registration.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,17 @@ import { EventEmitter } from 'events'
import { extractPendingCsrs, issueCertificate } from './registration.functions'
import { ErrorCodes, ErrorMessages, PermsData, RegisterOwnerCertificatePayload, SocketActionTypes } from '@quiet/types'
import { RegistrationEvents } from './registration.types'
import { StorageService } from '../storage/storage.service'
import Logger from '../common/logger'

@Injectable()
export class RegistrationService extends EventEmitter implements OnModuleInit {
private readonly logger = Logger(RegistrationService.name)
public certificates: string[] = []
private _permsData: PermsData
private storageService: StorageService
private registrationEvents: { csrs: string[], id?: string }[] = []
private registrationEventInProgress: boolean

constructor() {
super()
Expand All @@ -18,32 +22,15 @@ export class RegistrationService extends EventEmitter implements OnModuleInit {
onModuleInit() {
this.on(
RegistrationEvents.REGISTER_USER_CERTIFICATE,
async (payload: { csrs: string[]; certificates: string[]; id: string }) => {
await this.issueCertificates(payload)
(payload: { csrs: string[]; certificates: string[]; id: string }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should still be async function

this.registrationEvents.push({ csrs: payload.csrs, id: payload.id })
this.tryRegisterNextUserCertificates()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tryRegisterNextUserCertificates() is async function so it needs to be awaited. Otherwise it's a fire-and-forget, which is a very bad idea

I think something may be broken again with our eslint. Do you not get an error on this line? It should complain about missing await. If you did want to actually fire-and-forget you should write it as

void this.tryRegisterNextUserCertificates()

but this is asually a very bad idea and rarely the behaviour you want

Copy link
Author

@leblowl leblowl Dec 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think

new Promise(something).then(another),

is equivalent to

test = async () => {
  await something
  another
}

and where the Promise body, something is entirely synchronous. I think it's only the promise callback, another, which is added to the event loop and is asynchronous. So if there is no line after an await, I don't think the await does anything. I don't think the async keyword actually makes the function async. It's still synchronous unless there is a promise callback or I/O callback or timer. I've been reading a bit more about how JS works internally and posted a lot of links in the Slack internal channel. I also wrote some interested tests over here: develop...race

I was confused myself and so I started diving into it a bit more. I don't have the whole picture and am likely still confused about many things, but this is my latest understanding

A situation like this I think is a little bit different:

test = async () => {
  await something
}

test2 = async () => {
  await test
  another
}

Where the solo await in test does matter. But I don't think EventEmitter uses await at all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if there is no line after an await, I don't think the await does anything. I don't think the async keyword actually makes the function async

It's actually the opposite.

If you don't await this means that that the outer Promise will fire immediately. If you await the inner Promise the outer promise will wait for the inner one.
Even more importantly, if inner Promise throws a failure:
a) with await the outer Promise will be a failuer
b) without await the outer Promise will succeed and the failure from inner Promise will hit the generic error handler of the process

This second point is the main reason for always awaiting stuff. Without it, error handling is a nightmare.

}
)
}

private async issueCertificates(payload: { csrs: string[]; certificates: string[]; id?: string }) {
// Lack of permsData means that we are not the owner of the
// community in the official model of the app, however anyone can
// modify the source code, put malicious permsData here, issue
// false certificates and try to trick other users. To prevent
// that, peers verify that anything that is written to the
// certificate store is signed by the owner.
if (!this._permsData) {
if (payload.id) this.emit(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, { id: payload.id })
return
}
const pendingCsrs = await extractPendingCsrs(payload)

await Promise.all(
pendingCsrs.map(async csr => {
await this.registerUserCertificate(csr)
})
)

if (payload.id) this.emit(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, { id: payload.id })
public init(storageService: StorageService) {
this.storageService = storageService
}

public set permsData(perms: PermsData) {
Expand All @@ -53,15 +40,26 @@ export class RegistrationService extends EventEmitter implements OnModuleInit {
}
}

public async registerOwnerCertificate(payload: RegisterOwnerCertificatePayload): Promise<void> {
public async registerOwnerCertificate(payload: RegisterOwnerCertificatePayload): Promise<string | undefined> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string | undefined is annoying type to work with

For functions that return stuff conditionally it's generally recommended to go with string | null

if (!this.storageService) {
throw new Error("Storage Service must be initialized before the Registration Service")
}

// FIXME: We should resolve problems with events order and we should set permsData only on LAUNCH_REGISTRART socket event in connectionsManager.
this._permsData = payload.permsData
const result = await issueCertificate(payload.userCsr.userCsr, this._permsData)
if (result?.cert) {
await this.storageService.certificatesStore.addCertificate(result.cert)
// Not sure if this is necessary
const certs = await this.storageService.certificatesStore.loadAllCertificates()
if (!certs.includes(result.cert)) {
throw new Error("Cert wasn't added to CertificateStore correctly")
}
this.emit(SocketActionTypes.SAVED_OWNER_CERTIFICATE, {
communityId: payload.communityId,
network: { certificate: result.cert },
})
return result?.cert
} else {
this.emit(SocketActionTypes.ERROR, {
type: SocketActionTypes.REGISTRAR,
Expand All @@ -72,10 +70,76 @@ export class RegistrationService extends EventEmitter implements OnModuleInit {
}
}

public async registerUserCertificate(csr: string): Promise<void> {
const result = await issueCertificate(csr, this._permsData)
if (result?.cert) {
this.emit(RegistrationEvents.NEW_USER, { certificate: result.cert })
public async tryRegisterNextUserCertificates() {
if (!this.registrationEventInProgress) {
console.log("Registering next certificate")
this.registrationEventInProgress = true
const next = this.registrationEvents.shift()
if (next) {
await this.registerUserCertificates(next)
}
this.registrationEventInProgress = false

if (this.registrationEvents.length !== 0) {
setTimeout(this.tryRegisterNextUserCertificates.bind(this), 0)
}
}
}

// Apparently, JS will run each function to completion. So assuming
// we do not have multiple threads, this function should run to
// completion before it is called again. Because we take the CSRs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is inaccurate

JS will run the function synchronously until the first await statement.
It's a cool excercise to look up the actual implementation of this in C-node.
What happens underneath is that await is actually almost synonymous to yield. Async functions are really generators that "generate" Promises. Whenever Promise is returned, the handler applies to it's then() callback the logic that effectively calls next(resultOfPreviousPromise) on the generator.
If you think about it in these terms it becomes obvious what will and what will not get interrupted

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, this comment is outdated

// and then get the latest view of certificates, filter CSRs and
// then update certificates in a single function, we should never
// issue a CSR that is contained in the certificates list, at least
// in this function... as long as there is no other code that
// updates the certificates list. Something else to consider is that
// because this function is called asynchronously, there may be
// several invocations with different CSRs and they may run in an
// unexpected order. We could address that if it's an issue, but I
// think that might only affect the order of CSR registration.
public async registerUserCertificates(payload: { csrs: string[]; id?: string }) {
if (!this.storageService) {
throw new Error("Storage Service must be initialized before the Registration Service")
}
console.log("Registering user certificates")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't we refactor everthing to use nest logger for logging? If so, please fix this


// Lack of permsData means that we are not the owner of the
// community in the official model of the app, however anyone can
// modify the source code, put malicious permsData here, issue
// false certificates and try to trick other users. To prevent
// that, peers verify that anything that is written to the
// certificate store is signed by the owner.
if (!this._permsData) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here? Why would we trigger a "finished issuing certificates" event if we are not the owner?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I think I'm mostly reorganizing existing code here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is misleading. AFAIK it's only used for resolving promise in csrReplicatedPromiseMap in storage.

if (payload.id) this.emit(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, { id: payload.id })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to think eslint is completely off in this project. Is it not requesting that you wrap this in brackets?

return
}

console.log("Loading all certificates")
const certificates = await this.storageService.certificatesStore.loadAllCertificates()
console.log("Certificates loaded")

console.log("Extracting pending CSRs")
const pendingCsrs = await extractPendingCsrs({ csrs: payload.csrs, certificates: certificates as string[] })

for (const csr of pendingCsrs) {
console.log("Issuing certificate")
const result = await issueCertificate(csr, this._permsData)
if (result?.cert) {
console.log("Adding certificate")
await this.storageService.certificatesStore.addCertificate(result.cert)
// Not sure if this is necessary
const certs = await this.storageService.certificatesStore.loadAllCertificates()
if (!certs.includes(result.cert)) {
throw new Error("Cert wasn't added to CertificateStore correctly")
}
console.log("Certificate added")
this.emit('new', result?.cert)
} else {
console.log("Not adding certificate")
}
}

if (payload.id) this.emit(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, { id: payload.id })
}
}
24 changes: 20 additions & 4 deletions packages/state-manager/src/sagas/identity/registerCertificate/registerCertificate.saga.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ import { applyEmitParams, type Socket } from '../../../types'
import { type PayloadAction } from '@reduxjs/toolkit'
import { apply, select, put } from 'typed-redux-saga'
import { communitiesSelectors } from '../../communities/communities.selectors'
import { identitySelectors } from '../../identity/identity.selectors'
import { identityActions } from '../identity.slice'
import {
type RegisterOwnerCertificatePayload,
type RegisterUserCertificatePayload,
SocketActionTypes,
type InitCommunityPayload,
} from '@quiet/types'
import { communitiesActions } from '../../communities/communities.slice'

Expand All @@ -22,16 +24,30 @@ export function* registerCertificateSaga(
}

if (currentCommunity.CA?.rootCertString) {
const payload: RegisterOwnerCertificatePayload = {
communityId: action.payload.communityId,
userCsr: action.payload.userCsr,
const identity = yield* select(identitySelectors.selectById(currentCommunity.id))
if (!identity?.userCsr || !currentCommunity?.rootCa) {
console.error("User CSR or root cert missing", identity?.userCsr, currentCommunity?.rootCa)
return
}

const payload: InitCommunityPayload = {
id: currentCommunity.id,
peerId: identity.peerId,
hiddenService: identity.hiddenService,
certs: {
// Hacking, perhaps make certs.certificate optional
certificate: identity.userCertificate || '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call it bikeshedding on my part but || is a logic operator, so there is problem with how it's typed
I highly recommend using ?? instead

key: identity.userCsr.userKey,
CA: [currentCommunity.rootCa],
},
ownerCsr: identity.userCsr,
permsData: {
certificate: currentCommunity.CA.rootCertString,
privKey: currentCommunity.CA.rootKeyString,
},
}

yield* apply(socket, socket.emit, applyEmitParams(SocketActionTypes.REGISTER_OWNER_CERTIFICATE, payload))
yield* apply(socket, socket.emit, applyEmitParams(SocketActionTypes.CREATE_COMMUNITY, payload))
} else {
if (!isUsernameTaken) {
yield* put(communitiesActions.launchCommunity(action.payload.communityId))
Expand Down
23 changes: 1 addition & 22 deletions ...ages/state-manager/src/sagas/identity/savedOwnerCertificate/savedOwnerCertificate.saga.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,5 @@ export function* savedOwnerCertificateSaga(
socket: Socket,
action: PayloadAction<ReturnType<typeof identityActions.savedOwnerCertificate>['payload']>
): Generator {
let communityId: string = action.payload

if (!communityId) {
communityId = yield* select(communitiesSelectors.currentCommunityId)
}

const community = yield* select(communitiesSelectors.selectById(communityId))
const identity = yield* select(identitySelectors.selectById(communityId))
holmesworcester marked this conversation as resolved.
Show resolved Hide resolved
if (!identity?.userCertificate || !identity?.userCsr || !community?.rootCa) return

const payload: InitCommunityPayload = {
id: communityId,
peerId: identity.peerId,
hiddenService: identity.hiddenService,
certs: {
certificate: identity.userCertificate,
key: identity.userCsr.userKey,
CA: [community.rootCa],
},
}

yield* apply(socket, socket.emit, applyEmitParams(SocketActionTypes.CREATE_COMMUNITY, payload))
// TODO: Remove?
}
4 changes: 3 additions & 1 deletion packages/types/src/community.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { type HiddenService, type PeerId, type Identity } from './identity'
import { type HiddenService, type PeerId, type PermsData, type Identity, type UserCsr } from './identity'
import { InvitationPair } from './network'

export interface Community {
Expand Down Expand Up @@ -59,6 +59,8 @@ export interface InitCommunityPayload {
hiddenService: HiddenService
certs?: Certificates
peers?: string[]
ownerCsr?: UserCsr
permsData?: PermsData
}

export interface LaunchRegistrarPayload {
Expand Down
Loading