Paths

Table of Contentst

-
native/profile/
-
profile/
3/5
add-keyserver.react.js

[lib/native] add keyserver url validation logic and error handling to the add keyserver screen
ClosedPublic
Actions

Authored by ginsu on Oct 31 2023, 10:12 PM.

Details

Reviewers

inka
rohan
michal

Commits

rCOMMe250e5ccff7b: [lib/native] add keyserver url validation logic and error handling to the add…

Summary

This diff adds keyserver url validation logic to the onPressSave callback in the add keyserver screen. We also now display an error message if for any reason the keyserver url inputed by the user is not valid

Linear task: https://linear.app/comm/issue/ENG-4917/update-onpress-of-goadd-button-in-customservermodal-to-add-keyserver

Depends on D9666

Test Plan

Please see the demo video below

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Oct 31 2023, 10:12 PM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptOct 31 2023, 10:12 PM

ginsu edited the summary of this revision. (Show Details)Oct 31 2023, 10:14 PM

ginsu added a parent revision: D9666: [lib/native] introduce useIsKeyserverURLValid hook.

ginsu added reviewers: inka, rohan, michal.

ginsu edited the test plan for this revision. (Show Details)Oct 31 2023, 10:25 PM

ginsu attached a referenced file: F846696: Screen Recording 2023-11-01 at 1.24.30 AM.mov. (Show Details)

ginsu added inline comments.Oct 31 2023, 10:32 PM

lib/shared/keyserver-utils.js
18 ↗	(On Diff #32584)	We need to add this to the `serverCallParamOverride` otherwise when we attempt to call the get version endpoint from the add keyserver screen we will enter this if condition in the `callServerEndpoint` function and the get version endpoint will not correctly validate if the new keyserver url is correct or not. https://github.com/CommE2E/comm/blob/master/lib/utils/call-server-endpoint.js#L105-L125 When we call the get version endpoint from the keyserver selection in the registration flow the `connectionStatus` is `connecting` as well

Harbormaster completed remote builds in B23638: Diff 32584.Oct 31 2023, 10:43 PM

ginsu requested review of this revision.Oct 31 2023, 10:43 PM

ashoat added inline comments.Nov 1 2023, 10:39 AM

lib/shared/keyserver-utils.js
18 ↗	(On Diff #32584)	This is a legitimate issue you've discovered, but you're papering over it here. When we're overriding `urlPrefix`, we almost certainly never want to use an established socket, since that socket won't respect the overriden `urlPrefix`. Instead of making this change, can you submit a separate diff that skips over the condition you linked when `urlPrefix` is being overriden?

ginsu mentioned this in D9673: [lib] correctly skip using established socket when urlPrefix is overriden.Nov 2 2023, 1:05 AM

ginsu added a child revision: D9673: [lib] correctly skip using established socket when urlPrefix is overriden.Nov 2 2023, 1:06 AM

ginsu added inline comments.

lib/shared/keyserver-utils.js
18 ↗	(On Diff #32584)	D9673

ashoat added inline comments.Nov 2 2023, 7:01 AM

lib/shared/keyserver-utils.js
18 ↗	(On Diff #32584)	Accepted! After landing that diff, can you rebase this diff on it and confirm that this change isn't necessary anymore?

inka accepted this revision.Nov 7 2023, 4:38 AM

This revision is now accepted and ready to land.Nov 7 2023, 4:38 AM

rebase before landing

Harbormaster completed remote builds in B24732: Diff 34190.Dec 4 2023, 8:23 AM

rebase one more time

This revision was landed with ongoing or failed builds.Dec 4 2023, 10:31 PM

Closed by commit rCOMMe250e5ccff7b: [lib/native] add keyserver url validation logic and error handling to the add… (authored by ginsu). · Explain Why

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMMe250e5ccff7b: [lib/native] add keyserver url validation logic and error handling to the add….

Harbormaster completed remote builds in B24784: Diff 34247.Dec 4 2023, 10:51 PM

Revision Contents
Changeset List

Path

Size

native/

profile/

add-keyserver.react.js

34 lines

Diff 34190

View Options

native/profile/add-keyserver.react.js

// @flow		// @flow

import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';		import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';
import { isSQLiteSupported } from './utils/db-utils.js';		import { isDesktopSafari, isSQLiteSupported } from './utils/db-utils.js';
import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';		import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';
import type { AppState } from '../redux/redux-setup.js';		import type { AppState } from '../redux/redux-setup.js';
import {		import {
workerRequestMessageTypes,		workerRequestMessageTypes,
type WorkerRequestMessage,		type WorkerRequestMessage,
type WorkerResponseMessage,		type WorkerResponseMessage,
} from '../types/worker-types.js';		} from '../types/worker-types.js';

Show All 16 Lines	class DatabaseModule {
status: DatabaseStatus;		status: DatabaseStatus;

constructor() {		constructor() {
const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous		const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous
? undefined		? undefined
: preloadedState.currentUserInfo?.id;		: preloadedState.currentUserInfo?.id;
const isSupported = isSQLiteSupported(currentLoggedInUserID);		const isSupported = isSQLiteSupported(currentLoggedInUserID);

if (!isSupported) {		if (!isSupported \|\| isDesktopSafari()) {
		tomekUnsubmitted Not Done Inline Actions Can we have a const instead of a function that tells if it is Safari? We can avoid having to detect the browser multiple times, as this fact doesn't change through the lifetime of the app. tomek: Can we have a const instead of a function that tells if it is Safari? We can avoid having to…
		kamilAuthorUnsubmitted Done Inline Actions good point, thanks kamil: good point, thanks
this.status = databaseStatuses.notSupported;		this.status = databaseStatuses.notSupported;
		kamilAuthorUnsubmitted Done Inline Actions Naming might be confusing, but at this point database is not supported (we don't have a key to it), it will be supported after main thread will provide encryption key kamil: Naming might be confusing, but at this point database is not supported (we don't have a key to…
		tomekUnsubmitted Not Done Inline Actions Do we change the status later? tomek: Do we change the status later?
		kamilAuthorUnsubmitted Done Inline Actions yes, in `initDBForLoggedInUser()` kamil: yes, in `initDBForLoggedInUser()`
} else {		} else {
this.init();		this.init();
}		}
}		}

init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {		init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {
this.status = databaseStatuses.initInProgress;		this.status = databaseStatuses.initInProgress;
this.worker = new SharedWorker(DATABASE_WORKER_PATH);		this.worker = new SharedWorker(DATABASE_WORKER_PATH);
▲ Show 20 Lines • Show All 77 Lines • Show Last 20 Lines

[lib/native] add keyserver url validation logic and error handling to the add keyserver screenClosedPublicActions