Page MenuHomePhabricator

[keyserver] Create an endpoint that allows disabling links
ClosedPublic

Authored by tomek on May 22 2023, 6:25 AM.
Tags
None
Referenced Files
F3387181: D7906.id26974.diff
Fri, Nov 29, 8:06 AM
Unknown Object (File)
Mon, Nov 25, 8:53 PM
Unknown Object (File)
Sat, Nov 23, 6:57 PM
Unknown Object (File)
Mon, Nov 11, 12:28 PM
Unknown Object (File)
Oct 28 2024, 8:57 AM
Unknown Object (File)
Sep 28 2024, 3:10 PM
Unknown Object (File)
Sep 28 2024, 3:10 PM
Unknown Object (File)
Sep 28 2024, 3:10 PM
Subscribers

Details

Summary

At this point calling the endpoint results in link being deleted. In the future we can consider having a list of disabled links.

Depends on D7885

Test Plan

Tested in combination with the rest of the stack. Confirming disabling a link results in link being deleted.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.May 22 2023, 6:42 AM
kamil requested changes to this revision.May 22 2023, 10:24 AM
kamil added inline comments.
keyserver/src/deleters/link-deleters.js
14 ↗(On Diff #26773)
24–27 ↗(On Diff #26773)

shouldn't we throw ServerError('invalid_parameters'); when no rows was deleted?

or maybe another solution will be returning deleted invite link to inform the client about the result?

This revision now requires changes to proceed.May 22 2023, 10:24 AM
keyserver/src/responders/link-responders.js
93 ↗(On Diff #26773)

Should this be mixed now?

keyserver/src/deleters/link-deleters.js
24–27 ↗(On Diff #26773)

Why should we throw? It is generally a good idea to have more idempotent API: in this case, the intention of a user was for a link to not exist. If that's the case, then great - it doesn't matter if a link wasn't existing before.

A possible scenario where it could happen is when two users try to disable a link at the same time - it's ok that one of the request will finish earlier.

A different corner case is when a link was renamed by one user. Then it's hard to tell how we should handle that. There are a couple of solutions. But that would require some analysis (probably not worth it in the near future).

Specify return type and use mixed as input type

keyserver/src/deleters/link-deleters.js
14 ↗(On Diff #26773)

Don't think that it matters - otherwise Flow would complain as this function is exported.

keyserver/src/responders/link-responders.js
93 ↗(On Diff #26773)

Yes, we should. Created D7944 where the other endpoint from this file is also updated.

kamil added inline comments.
keyserver/src/deleters/link-deleters.js
24–27 ↗(On Diff #26773)

Your explanation makes sense. My intention was to point out that when a user calls this endpoint and nothing changes we probably shouldn't allow the user to call it (as this is something unexpected), but from the scenario you brought it looks like a regular situation, sorry for the confusion

This revision is now accepted and ready to land.May 23 2023, 4:21 PM