[keyserver] Introduce convertObject function
ClosedPublic
Actions

Authored by michal on Apr 18 2023, 8:33 AM.

Details

Reviewers

kamil
inka
tomek
ashoat

Commits

rCOMM1371c52cec71: [keyserver] Introduce convertObject function

Summary

Introdices convertObject function that implements the visitor pattern and uses it to implement sanitizeInput. It will used in the later diffs for implementing the id conversion function.

sanitizeInput should still redact passwords where it previously redacted them but it will now redact them in more places, e.g. lists, non-string optionals. Previously added tests succeed, and I have also added tests fot the new cases.

Test Plan

Run yarn jest

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Apr 18 2023, 8:33 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptApr 18 2023, 8:33 AM

michal added a parent revision: D7487: [keyserver] Add password sanitization tests.Apr 18 2023, 8:34 AM

Harbormaster completed remote builds in B18455: Diff 25273.Apr 18 2023, 8:51 AM

michal requested review of this revision.Apr 18 2023, 8:51 AM

ashoat mentioned this in D7487: [keyserver] Add password sanitization tests.Apr 18 2023, 9:29 AM

ashoat added inline comments.Apr 18 2023, 9:33 AM

keyserver/src/utils/validation-utils.js
22–26 ↗	(On Diff #25273)	Types looks significantly improved!! Wondering if this should be typed with a generic similar to how you typed `sanitizeInput`?
147 ↗	(On Diff #25273)	Why is the `: any` necessary here?
159–160 ↗	(On Diff #25273)	Nit: I think it would be better to do `return (conversionFunction(input): any);` here... `$FlowExpectedError` is meant for unit tests (eg. like this). There is also `$FlowFixMe`, but all of these are basically aliases for `any` Separately – can you talk through what you tried to avoid an `any` type here?

Looks great! (requesting changes because addressing the comments might change things significantly)

keyserver/src/utils/validation-utils.js
201–203 ↗	(On Diff #25273)	Why do we have to use lodash?

This revision now requires changes to proceed.Apr 21 2023, 1:52 AM

Changed mixed to generics, removed lodash _map. Tried to minimize usages of any by using a assertWithValidator function that's a no-op but changes the the data's type after validating it.

keyserver/src/utils/validation-utils.js
147 ↗	(On Diff #25273)	I'm actually not sure, without it, I'm getting: Cannot instantiate TType because I is not a valid argument of TStructProps I suspect this fixes it because `: any` doesn't allow for the `empty` type. But I've tried to make a minimal repro with but I couldn't get it to fail with `empty` so I'm not sure.

Harbormaster completed remote builds in B18620: Diff 25510.Apr 21 2023, 4:05 AM

ashoat added inline comments.Apr 21 2023, 6:56 AM

keyserver/src/utils/validation-utils.js
147 ↗	(On Diff #25510)	I think we should try to figure this out. I'll try to make some time to take a look at this later today
lib/utils/validation-utils.js
96 ↗	(On Diff #25510)	I think this is a good usage of `any`. We're doing runtime type validation on a type we don't understand; after validating, we have to assert statically that it is of the type that is passed to us. Furthermore, we use `any` in a "safe" way that won't affect other types, by immediately casting the result to `T`

michal added a child revision: D7564: [lib] Move `messageTypes` enum to a separate file.Apr 21 2023, 7:57 AM

ashoat added inline comments.Apr 21 2023, 11:33 AM

keyserver/src/utils/validation-utils.js
147 ↗	(On Diff #25510)	I looked at this closer. I agree there's no way to avoid using `any` here... the core issue is that every type in the `TType` union needs to support its type parameter, but in the `TInterface` case we use `$ObjMap` on the type parameter, which only works on objects. It's possible this will work with the upcoming conditional type supporting coming to Flow. I did try this with my `$If` hack, but I wasn't able to get it working quickly. Instead, I figure it would be better to "constrain" our use of `any`. The best way to use `any` is to cast something to `any` and then immediately cast it to something else, so that the `any` type doesn't cross any boundaries and affect something we don't want it to affect. I think this would be slightly better: https://gist.github.com/Ashoat/45893e511ab9e860e351d08307ec3ef7
159–161 ↗	(On Diff #25510)	Are `TValidator` and `validator` the same? If so, it's confusing that you use both of them here. Should you use `typesToConvert.includes(validator)` and then use `validator` here?

tomek accepted this revision.Apr 25 2023, 2:58 AM

tomek added a reviewer: ashoat.

See my previous review

This revision now requires changes to proceed.Apr 25 2023, 4:21 AM

Introduce changes from the diff in the gist

keyserver/src/utils/validation-utils.js
159–161 ↗	(On Diff #25510)	`validator` is `TType<I>` and `TValidator` is `TType<T>` which allows the `assertWithValidator` to change the type of the `input`. I could change it to just cast `const recastValidator: TType<T> = (validator: any);` if you think that's better.

Harbormaster completed remote builds in B18741: Diff 25679.Apr 25 2023, 8:09 AM

ashoat accepted this revision.Apr 25 2023, 10:21 AM

ashoat added inline comments.

keyserver/src/utils/validation-utils.js
159–161 ↗	(On Diff #25510)	Thanks for explaining. I think the best thing would be to add a code comment explaining what we're doing here

This revision is now accepted and ready to land.Apr 25 2023, 10:21 AM

Add a comment

Harbormaster completed remote builds in B18843: Diff 25812.Apr 27 2023, 4:32 AM

Rebase

Harbormaster completed remote builds in B19022: Diff 26053.May 4 2023, 3:21 AM

Closed by commit rCOMM1371c52cec71: [keyserver] Introduce convertObject function (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyMay 5 2023, 5:46 AM

This revision was automatically updated to reflect the committed changes.

Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMM1371c52cec71: [keyserver] Introduce convertObject function.

michal mentioned this in D8355: [lib] Generate converters from validators.Jul 4 2023, 7:20 AM

inka mentioned this in D12199: [lib] Fix invite links validation.May 23 2024, 6:04 AM

ashoat mentioned this in D12387: [keyserver] Improve logging in checkInputValidator.Jun 11 2024, 6:41 AM

Revision Contents
Changeset List

Path

Size

keyserver/

src/

utils/

validation-utils.js

136 lines

validation-utils.test.js

47 lines

lib/

utils/

validation-utils.js

8 lines

Diff 26053

View Options

keyserver/src/utils/validation-utils.js

Show All 40 Lines	function ManagePublicLinkScreen(props: Props): React.Node {
let errorComponent = null;		let errorComponent = null;
if (error) {		if (error) {
errorComponent = <Text style={styles.error}>{error}</Text>;		errorComponent = <Text style={styles.error}>{error}</Text>;
}		}

const onDisableButtonClick = React.useCallback(() => {		const onDisableButtonClick = React.useCallback(() => {
Alert.alert(		Alert.alert(
'Disable public link',		'Disable public link',
'Are you sure you want to disable your public link? Members who have your community’s public link but have not joined will not able to with the disabled link. \n' +		'Are you sure you want to disable your public link?\n' +
'\n' +		'\n' +
'Other communities may also claim your previous public link url.',		'Other communities will be able to claim the same URL.',
[		[
{		{
text: 'Confirm disable',		text: 'Confirm disable',
style: 'destructive',		style: 'destructive',
onPress: disableInviteLink,		onPress: disableInviteLink,
},		},
{		{
text: 'Cancel',		text: 'Cancel',
Show All 18 Lines	disablePublicLinkButton = (
</View>		</View>
);		);
}		}

return (		return (
<View>		<View>
<View style={styles.section}>		<View style={styles.section}>
<Text style={styles.sectionText}>		<Text style={styles.sectionText}>
Let your community be more accessible with your own unique public		Invite links make it easy for your friends to join your community.
link. By enabling a public link, you are allowing anyone who has your		Anybody who knows your community’s invite link will be able to join
link to join your community.{'\n\n'}		it. Note that if you change your public link’s URL, other communities
Editing your community’s public link allows other communities to claim		will be able to claim the old URL.
		ashoatUnsubmitted Not Done Inline Actions From the screenshot, this looks like too much text in a row. We could try separating out the third sentence into its own paragraph. We could also try showing the three sentences as three bullet points. @ted, any thoughts? ashoat: From the screenshot, this looks like too much text in a row. We could try separating out the…
		tomekAuthorUnsubmitted Done Inline Actions In the original design, the last sentence was separated by an empty line. It probably makes sense to do the same here. tomek: In the original design, the last sentence was separated by an empty line. It probably makes…
your previous URL.
</Text>		</Text>
</View>		</View>
<Text style={styles.sectionTitle}>INVITE URL</Text>		<Text style={styles.sectionTitle}>INVITE URL</Text>
<View style={styles.section}>		<View style={styles.section}>
<View style={styles.inviteLink}>		<View style={styles.inviteLink}>
<Text style={styles.inviteLinkPrefix}>{inviteLinkUrl('')}</Text>		<Text style={styles.inviteLinkPrefix}>{inviteLinkUrl('')}</Text>
<TextInput		<TextInput
style={styles.input}		style={styles.input}
▲ Show 20 Lines • Show All 108 Lines • Show Last 20 Lines

View Options

keyserver/src/utils/validation-utils.test.js

Show All 40 Lines	function ManagePublicLinkScreen(props: Props): React.Node {
let errorComponent = null;		let errorComponent = null;
if (error) {		if (error) {
errorComponent = <Text style={styles.error}>{error}</Text>;		errorComponent = <Text style={styles.error}>{error}</Text>;
}		}

const onDisableButtonClick = React.useCallback(() => {		const onDisableButtonClick = React.useCallback(() => {
Alert.alert(		Alert.alert(
'Disable public link',		'Disable public link',
'Are you sure you want to disable your public link? Members who have your community’s public link but have not joined will not able to with the disabled link. \n' +		'Are you sure you want to disable your public link?\n' +
'\n' +		'\n' +
'Other communities may also claim your previous public link url.',		'Other communities will be able to claim the same URL.',
[		[
{		{
text: 'Confirm disable',		text: 'Confirm disable',
style: 'destructive',		style: 'destructive',
onPress: disableInviteLink,		onPress: disableInviteLink,
},		},
{		{
text: 'Cancel',		text: 'Cancel',
Show All 18 Lines	disablePublicLinkButton = (
</View>		</View>
);		);
}		}

return (		return (
<View>		<View>
<View style={styles.section}>		<View style={styles.section}>
<Text style={styles.sectionText}>		<Text style={styles.sectionText}>
Let your community be more accessible with your own unique public		Invite links make it easy for your friends to join your community.
link. By enabling a public link, you are allowing anyone who has your		Anybody who knows your community’s invite link will be able to join
link to join your community.{'\n\n'}		it. Note that if you change your public link’s URL, other communities
Editing your community’s public link allows other communities to claim		will be able to claim the old URL.
		ashoatUnsubmitted Not Done Inline Actions From the screenshot, this looks like too much text in a row. We could try separating out the third sentence into its own paragraph. We could also try showing the three sentences as three bullet points. @ted, any thoughts? ashoat: From the screenshot, this looks like too much text in a row. We could try separating out the…
		tomekAuthorUnsubmitted Done Inline Actions In the original design, the last sentence was separated by an empty line. It probably makes sense to do the same here. tomek: In the original design, the last sentence was separated by an empty line. It probably makes…
your previous URL.
</Text>		</Text>
</View>		</View>
<Text style={styles.sectionTitle}>INVITE URL</Text>		<Text style={styles.sectionTitle}>INVITE URL</Text>
<View style={styles.section}>		<View style={styles.section}>
<View style={styles.inviteLink}>		<View style={styles.inviteLink}>
<Text style={styles.inviteLinkPrefix}>{inviteLinkUrl('')}</Text>		<Text style={styles.inviteLinkPrefix}>{inviteLinkUrl('')}</Text>
<TextInput		<TextInput
style={styles.input}		style={styles.input}
▲ Show 20 Lines • Show All 108 Lines • Show Last 20 Lines

View Options

lib/utils/validation-utils.js

Show All 40 Lines	function ManagePublicLinkScreen(props: Props): React.Node {
let errorComponent = null;		let errorComponent = null;
if (error) {		if (error) {
errorComponent = <Text style={styles.error}>{error}</Text>;		errorComponent = <Text style={styles.error}>{error}</Text>;
}		}

const onDisableButtonClick = React.useCallback(() => {		const onDisableButtonClick = React.useCallback(() => {
Alert.alert(		Alert.alert(
'Disable public link',		'Disable public link',
'Are you sure you want to disable your public link? Members who have your community’s public link but have not joined will not able to with the disabled link. \n' +		'Are you sure you want to disable your public link?\n' +
'\n' +		'\n' +
'Other communities may also claim your previous public link url.',		'Other communities will be able to claim the same URL.',
[		[
{		{
text: 'Confirm disable',		text: 'Confirm disable',
style: 'destructive',		style: 'destructive',
onPress: disableInviteLink,		onPress: disableInviteLink,
},		},
{		{
text: 'Cancel',		text: 'Cancel',
Show All 18 Lines	disablePublicLinkButton = (
</View>		</View>
);		);
}		}

return (		return (
<View>		<View>
<View style={styles.section}>		<View style={styles.section}>
<Text style={styles.sectionText}>		<Text style={styles.sectionText}>
Let your community be more accessible with your own unique public		Invite links make it easy for your friends to join your community.
link. By enabling a public link, you are allowing anyone who has your		Anybody who knows your community’s invite link will be able to join
link to join your community.{'\n\n'}		it. Note that if you change your public link’s URL, other communities
Editing your community’s public link allows other communities to claim		will be able to claim the old URL.
		ashoatUnsubmitted Not Done Inline Actions From the screenshot, this looks like too much text in a row. We could try separating out the third sentence into its own paragraph. We could also try showing the three sentences as three bullet points. @ted, any thoughts? ashoat: From the screenshot, this looks like too much text in a row. We could try separating out the…
		tomekAuthorUnsubmitted Done Inline Actions In the original design, the last sentence was separated by an empty line. It probably makes sense to do the same here. tomek: In the original design, the last sentence was separated by an empty line. It probably makes…
your previous URL.
</Text>		</Text>
</View>		</View>
<Text style={styles.sectionTitle}>INVITE URL</Text>		<Text style={styles.sectionTitle}>INVITE URL</Text>
<View style={styles.section}>		<View style={styles.section}>
<View style={styles.inviteLink}>		<View style={styles.inviteLink}>
<Text style={styles.inviteLinkPrefix}>{inviteLinkUrl('')}</Text>		<Text style={styles.inviteLinkPrefix}>{inviteLinkUrl('')}</Text>
<TextInput		<TextInput
style={styles.input}		style={styles.input}
▲ Show 20 Lines • Show All 108 Lines • Show Last 20 Lines

[keyserver] Introduce convertObject functionClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 26053

keyserver/src/utils/validation-utils.js

keyserver/src/utils/validation-utils.test.js

lib/utils/validation-utils.js

[keyserver] Introduce convertObject function
ClosedPublic
Actions

Revision Contents
Changeset List