Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Optionally allow messages without MDC #130

Merged
merged 4 commits into from
Sep 30, 2022
Merged

Optionally allow messages without MDC #130

merged 4 commits into from
Sep 30, 2022

Conversation

aksdb
Copy link
Contributor

@aksdb aksdb commented Sep 17, 2022

Here is my proposal on how to approach #129.

@twiss
Copy link
Member

twiss commented Sep 29, 2022

Sorry for the delay! This looks much better.

A few nitpicks: I would name the config option for example InsecureAllowUnauthenticatedMessages, that's a bit more descriptive (for those who don't know what an MDC is), and also makes it clear that the option is insecure. And then the function can just be AllowUnauthenticatedMessages, for example. And the error message could be changed to "Message is not authenticated" (as the "not supported" part is no longer really true after this PR).

@aksdb
Copy link
Contributor Author

aksdb commented Sep 30, 2022

Sorry for the delay! This looks much better.

A few nitpicks: I would name the config option for example InsecureAllowUnauthenticatedMessages, that's a bit more descriptive (for those who don't know what an MDC is), and also makes it clear that the option is insecure. And then the function can just be AllowUnauthenticatedMessages, for example. And the error message could be changed to "Message is not authenticated" (as the "not supported" part is no longer really true after this PR).

Good points. Applied them all.

Copy link
Member

@twiss twiss left a comment

Choose a reason for hiding this comment

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

👍 Thanks!

@twiss twiss merged commit c6815a8 into ProtonMail:main Sep 30, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants