hide metadata of private notes

Johann150 commented

2022-07-17 18:35:59 +00:00

Owner

Analog of https://github.com/misskey-dev/misskey/pull/8641, the history is slightly rewritten to prevent errors that were spotted later which should make for a cleaner commit history.

The hideNote function has been removed. This has the additional advantage that it removes duplicated code for checking the visibility of a note already in isVisibleForMe.

Users will no longer be able to see notes that are not visible to them. The previous behaviour was to show the note just with the text replaced by a message that the note is not visible to the user. The new behaviour is that the server claims that the note does not exist at all if it is not visible to the user.

Users will no longer be able to reply to or renote notes that are not visible to them.

Analog of <https://github.com/misskey-dev/misskey/pull/8641>, the history is slightly rewritten to prevent errors that were spotted later which should make for a cleaner commit history. The `hideNote` function has been removed. This has the additional advantage that it removes duplicated code for checking the visibility of a note already in `isVisibleForMe`. Users will no longer be able to see notes that are not visible to them. The previous behaviour was to show the note just with the text replaced by a message that the note is not visible to the user. The new behaviour is that the server claims that the note does not exist at all if it is not visible to the user. Users will no longer be able to reply to or renote notes that are not visible to them.

Johann150 added this to the (deleted) project 2022-07-17 18:36:04 +00:00

Johann150 commented

2022-07-18 10:52:41 +00:00

Author

Owner

There are some issues with this in combination with the added possibility of using GET requests for the API. For example, hovering over a reaction to a followers-only post does not work any more and returns the error NO_SUCH_NOTE.

The problem is that the GET part of the API does not use/support authentication properly. We would have to introduce a proper Authentication HTTP header probably for that to work correctly unless we want the token in the URL parameters.

Alternatively revert that endpoint back to a POST request. But we would also have to check each API endpoint. (sigh how many times did i already do that for this PR)

There are some issues with this in combination with the added possibility of using GET requests for the API. For example, hovering over a reaction to a followers-only post does not work any more and returns the error `NO_SUCH_NOTE`. The problem is that the GET part of the API does not use/support authentication properly. We would have to introduce a proper `Authentication` HTTP header probably for that to work correctly unless we want the token in the URL parameters. Alternatively revert that endpoint back to a POST request. But we would also have to check each API endpoint. (*sigh* how many times did i already do that for this PR)

Johann150 referenced this pull request

2022-07-18 21:33:11 +00:00

implement bearer token authentication #15

Johann150 added a new dependency 2022-07-18 21:34:41 +00:00

#15 implement bearer token authentication

Johann150 added a new dependency 2022-07-19 19:37:08 +00:00

#8 visibility checks around reported content

Johann150 force-pushed private-notes from a3f8bde92e to 3fe351df6d

2022-07-23 20:28:52 +00:00

Compare

toast commented

2022-07-25 09:59:54 +00:00

Owner

Kind of wonder what the performance penalty is on the overall here. I'm noticing a couple of allSettled promise calls.
I'd imagine it should be fine, but might be a good idea to check in case it actively tanks.
Besides that looks fine!

P.S. Maybe we can start the "shared code" package between client/backend/etc and put errors into it? That way we could have something like errors.AUTH_ERROR instead of UUIDs...

Kind of wonder what the performance penalty is on the overall here. I'm noticing a couple of allSettled promise calls. I'd imagine it should be fine, but might be a good idea to check in case it actively tanks. Besides that looks fine! P.S. Maybe we can start the "shared code" package between client/backend/etc and put errors into it? That way we could have something like `errors.AUTH_ERROR` instead of UUIDs...

Johann150 commented

2022-07-25 15:06:47 +00:00

Author

Owner

Kind of wonder what the performance penalty is on the overall here. I'm noticing a couple of allSettled promise calls.

The reason for the allSettled calls is because the way of determining that a note is not visible to the given user is to throw an error. However, I don't want one invisible note to make all other notes that I am trying to pack become invisibile as well. I assume there are no other rejection conditions in what is essentially transforming data, as there were none dealth with before, and I don't think performance should be impacted. (Not measured though because no idea how to.)

have something like errors.AUTH_ERROR instead of UUIDs...

In this case the explicit goal was to not have a separate error cases for these notes but instead just pretend they do not exist in the first place. But I think even in that case there were multiple errors that essentially said "this note doesnt exist" so that might be something to look into in the future. Out of scope for this PR though I think.

> Kind of wonder what the performance penalty is on the overall here. I'm noticing a couple of `allSettled` promise calls. The reason for the `allSettled` calls is because the way of determining that a note is not visible to the given user is to throw an error. However, I don't want one invisible note to make all other notes that I am trying to pack become invisibile as well. I assume there are no other rejection conditions in what is essentially transforming data, as there were none dealth with before, and I don't *think* performance should be impacted. (Not measured though because no idea how to.) > have something like `errors.AUTH_ERROR` instead of UUIDs... In this case the explicit goal was to not have a separate error cases for these notes but instead just pretend they do not exist in the first place. But I think even in that case there were multiple errors that essentially said "this note doesnt exist" so that might be something to look into in the future. Out of scope for this PR though I think.

👍 1

Johann150 referenced this issue from a commit

2022-07-25 16:16:31 +00:00

Merge: enhance privacy of notes

Johann150 manually merged commit 9ee609d700 into main

2022-07-25 16:18:55 +00:00

Johann150 deleted branch private-notes

2022-07-25 16:19:05 +00:00