[keyserver] validate if policies are acknowledged on request
ClosedPublic
Actions

Authored by kamil on Dec 6 2022, 6:15 AM.

Details

Reviewers

tomek
atul

Commits

rCOMM3351f164fadb: [keyserver] validate if policies are acknowledged on request

Summary

Checking acknowledgment before executing responder, on both HTTP and WebSocket endpoints. User will not be able to use the app without accepting policy, because most of the functionalities are disabled (will return an error), we can also disable not only API_RESPONSE socket message, but with current approach we will achieve our goal and maintain keyserver connection under the hood, so after accepting user will be able to use app immediately without waiting for e.g. socket initialization.

Context in ENG-2363.

Test Plan

Check if policies are properly validated on both WebSocket and HTTP request, if any of required acknowledgment is missing client should receive an error.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Dec 6 2022, 6:15 AM

kamil held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptDec 6 2022, 6:15 AM

kamil added a parent revision: D5823: [keyserver] refactor JSON endpoints to require policy acknowledgment.Dec 6 2022, 6:15 AM

kamil added a child revision: D5826: [lib] client side logic for handling policy acknowledgment.Dec 6 2022, 6:23 AM

Harbormaster completed remote builds in B14004: Diff 19177.Dec 6 2022, 6:27 AM

kamil published this revision for review.Dec 6 2022, 6:55 AM

tomek requested changes to this revision.Dec 6 2022, 9:48 AM

tomek added inline comments.

keyserver/src/responders/handlers.js
45–46 ↗	(On Diff #19177)	Ideally we could make a single await for these two, but the fact that the second call requires viewer makes this challenging. Another idea is to include accepted policies inside viewer object and to make fetching policies a part of `fetchViewerForJSONRequest` call. Not sure though what will be the consequences of this. Also it doesn't make sense to spend too much time on it (it would be only slightly more efficient).
keyserver/src/utils/validation-utils.js
170–178 ↗	(On Diff #19177)	This pattern resembles a filter over an array. How about using a `filter` instead?
172 ↗	(On Diff #19177)	In the future it might be a good idea to use a structure that allows constant time access, but with one (or other small number) policy it is more efficient to keep using array.
184 ↗	(On Diff #19177)	We can check code version at the beginning and early exit

This revision now requires changes to proceed.Dec 6 2022, 9:48 AM

add early exit, use filter()

kamil added inline comments.Dec 13 2022, 5:34 AM

keyserver/src/responders/handlers.js
45–46 ↗	(On Diff #19177)	I also think it will be great to find improvement here, but have not only `fetchViewerForJSONRequest` but also `fetchViewerForSocket`, and it'll be not that easy to make single await there, and they're called in different places in codebase so we will probably not gain much efficiency with increasing complexity a lot