Page MenuHomePhabricator

Soften requirements for password when deleting thread
ClosedPublic

Authored by marcin on Dec 29 2022, 6:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 8:15 AM
Unknown Object (File)
Tue, Nov 5, 4:26 AM
Unknown Object (File)
Tue, Nov 5, 4:26 AM
Unknown Object (File)
Tue, Nov 5, 1:41 AM
Unknown Object (File)
Sun, Oct 27, 9:21 PM
Unknown Object (File)
Sun, Oct 27, 9:21 PM
Unknown Object (File)
Sun, Oct 27, 9:21 PM
Unknown Object (File)
Sun, Oct 27, 9:21 PM
Subscribers

Details

Summary

This differential makes account password an optional parameter of thread deletion request

Test Plan

Build and lanynch the app

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 29 2022, 6:49 AM
Harbormaster failed remote builds in B14921: Diff 20338!
marcin added reviewers: tomek, atul.
This revision is now accepted and ready to land.Dec 29 2022, 6:57 AM
keyserver/src/responders/thread-responders.js
46 ↗(On Diff #20338)

We can consider completely removing this parameter for newer clients

keyserver/src/responders/thread-responders.js
46 ↗(On Diff #20338)

I also thought about that, but I was strictly following this requirement: https://linear.app/comm/issue/ENG-2541#comment-a48dff3b. It explicitly advices to keep it as optional parameter even though we should ignore it altogether.

keyserver/src/responders/thread-responders.js
46 ↗(On Diff #20338)

Hmm... I think @tomek is referring to the client side, not the keyserver side. He's saying we shouldn't pass that data up from newer clients, right?

keyserver/src/responders/thread-responders.js
46 ↗(On Diff #20338)

My idea here was to have two different validators, one with and one without password, and choose them based on viewer's code version.

keyserver/src/responders/thread-responders.js
46 ↗(On Diff #20338)

What is the benefit of this approach? Moreover the choice between validators based on code version, should still be restricted to non-SIWE accounts (SWIE accounts shouldn't have to provide password regardless of code version). As far as I can see this approach introduces noticeable complexity, but I can't see benefits.
Additionally, I am really sorry for not starting this discussion before landing this differential - I was in a rush to land this stack to meet the deadline, but still should have at least share my opinion before landing.

keyserver/src/responders/thread-responders.js
46 ↗(On Diff #20338)

Overall we should be precise about declaring our API. In theory we could declare the requests to be inexact, accept any object, and take from it whatever interests us and ignore anything else. But that will degrade maintainability.

So the maintainability is probably the most important benefit here. If we declare that all the clients above some code version aren't supposed to send this parameter, we could ultimately delete it after bumping minimal supported version.

Also, when we are precise when declaring the API, we can safely implement it without guessing. In the future someone may start wondering why this is an optional prop and when clients send it. Some searching though blame will clarify the issue, but it is less convenient that stating it explicitly.

On the other hand having different validators for different code versions also complicates the code.

Also SIWE increases the complexity, so maybe it's better to not change this.