[keyserver] Introduce validateOutput function
ClosedPublic
Actions

Authored by michal on May 4 2023, 3:56 AM.

Details

Reviewers

kamil
tomek

Commits

rCOMM5298ae2c9991: [keyserver] Introduce validateOutput function

Summary

Introduces validateOutput function that validates and (right now blocked with a flag) converts the ids of the responses. Also introduces a pair of functions convertServerIDsToClientIDs (used in validateOutput and in tests) and convertClientIDsToServerIDs (only used in tests right now, will be used in the future diff for the input conversion).

Depends on D7710

Test Plan

yarn jest
Test if the app works fine after adding validateOutput to responders in the later diffs

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.May 4 2023, 3:56 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptMay 4 2023, 3:56 AM

Harbormaster completed remote builds in B19035: Diff 26066.May 4 2023, 4:13 AM

michal requested review of this revision.May 4 2023, 4:13 AM

michal added inline comments.May 4 2023, 5:31 AM

keyserver/src/utils/validation-utils.js
36 ↗	(On Diff #26066)	I've set it to `256` (ashoat user id) based on this comment: Is 00001 supposed to be the keyserver ID? This doesn't make sense, as 1 is the ID of a community, not a keyserver. We could use the user ID as the keyserver ID maybe... not entirely sure this is a good idea, but it seems reasonable. My user ID is 256 but I'm not sure why community and keyserver id's would conflict?

michal added a child revision: D7712: [keyserver] Start validating responders output.May 4 2023, 7:51 AM

tomek accepted this revision.May 5 2023, 12:30 AM

tomek added inline comments.

keyserver/src/utils/validation-utils.js
36 ↗	(On Diff #26066)	Each keyserver has to have a unique id. Also, each user can have up to one keyserver, so each keyserver can be identified by user id instead of introducing a new id. but I'm not sure why community and keyserver id's would conflict? It's not a conflict. Using `00001` would mean that we're introducing a new id and somehow have to make sure that they are globally unique. Using userID solves the issue without any additional work.
69–72 ↗	(On Diff #26066)	Is it expected that this will happen? Or is it a serious error state?
73 ↗	(On Diff #26066)	We can use string template literal here

This revision is now accepted and ready to land.May 5 2023, 12:30 AM

Use string template

keyserver/src/utils/validation-utils.js
36 ↗	(On Diff #26066)	Thanks for explaining!
69–72 ↗	(On Diff #26066)	It's not expected and would mean that the server has wrong IDs in the database. We can recover from this so that's why only log a warning here. I could change it to throw an error if you think that's better.

tomek added inline comments.May 5 2023, 3:33 AM

keyserver/src/utils/validation-utils.js
69–72 ↗	(On Diff #26066)	It's hard to say what we should do in this case. It might mean that we have a serious bug in our code. For me, throwing here doesn't sound like an overkill - continuing might make things worse. Also, can we somehow include more debug info so that we know which id is invalid? If we decide to throw, we have to make sure that it is handled in the same way as e.g. invalid params.

Harbormaster completed remote builds in B19059: Diff 26093.May 5 2023, 3:43 AM

Changed the output validation from server error to a warning. This will be temporary, but with this we can safely check for validator <-> flow-types/runtime-objects mismatches without breaking clients. We can change it back once we activate the conversion logic. Included the id in the error message of serverID -> clientID conversion.

For me, throwing here doesn't sound like an overkill - continuing might make things worse

A warning here will probably indicate an error in some input conversion (because the keyserver db will have some id with the prefix). And throwing here won't stop any further modifications of the database because they would have already happened.

If we decide to throw, we have to make sure that it is handled in the same way as e.g. invalid params.

Can you explain more? I couldn't find a place where we handle the invalid params in some other way.

michal added inline comments.May 8 2023, 1:20 AM

keyserver/src/utils/validation-utils.js
46 ↗	(On Diff #26150)	This is going to display a human-readable name if it was provided when creating a validator or just display the validator definition if they don't have a name.