Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Have Authorization for deletion of messages#78

Merged
LeonFLK merged 13 commits intodevelopfrom
lw-message-guard
Sep 15, 2020
Merged

Have Authorization for deletion of messages#78
LeonFLK merged 13 commits intodevelopfrom
lw-message-guard

Conversation

@LeonFLK
Copy link
Contributor

@LeonFLK LeonFLK commented Sep 4, 2020

fixes KILTProtocol/ticket#719

fixes KILTProtocol/ticket#685

Adds header authorization as signature key to the message delete route, to restrict message deletion to receiver of the message.

If you want to delete a specific message with the messageId, supply the with the receiver identity signed messageId inside headers as signature key.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of restricting messages to the in / outbox; however the authentication scheme of signing your address is trivial to circumvent and could encourage bad practice in the worst case. Because we sign the same string every time, anyone intercepting a successful request could again request all of this addresses incoming and outgoing messages. If we're serious about this, we'd need to answer a challenge sent by the server as they do in the HOBA scheme or SCRAM. Of course this would be quite a bit more work... maybe there is already a next.js implementation for that out there? BTW are we following HTTP authentication specs here or are we being creative? (I'd love to see some integration tests to understand what a request for this would look like).
The scheme may be more secure when signing a unique message id for deletion, which is a one-off operation. Re the implementation, I'd propose you add a findById method to the message service, then handle the authentication stuff on the controller - my intuition tells me that's where those kinds of things belong.

@rflechtner
Copy link
Contributor

Signing a date header with the current time as proposed here may be a low-effort solution that is a bit more secure?

@LeonFLK
Copy link
Contributor Author

LeonFLK commented Sep 9, 2020

We don't care about inbox and outbox, we only care about authorizing the delete of a message.
For now we are going to stick to this approach, and are going to change this when we adopt DIDcomm or other means of authorization and security.

Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just throw out the yarn.lock changes pls! And maybe have another look at the error string with which we respond to unresolvable signatures.

@LeonFLK LeonFLK changed the title Have basic Authorization for retrieval and deletion of messages Have Authorization for deletion of messages Sep 15, 2020
@LeonFLK LeonFLK dismissed rflechtner’s stale review September 15, 2020 12:25

Requested changes implemented

@LeonFLK LeonFLK merged commit 04fae9c into develop Sep 15, 2020
@rflechtner rflechtner deleted the lw-message-guard branch September 15, 2020 13:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants