Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Implement system message for OMEMO status in the conversation #129

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Implement system message for OMEMO status in the conversation #129

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 23, 2018

Fix issue #125
Also add a preference option for it

@ghost
Copy link
Author

ghost commented Oct 23, 2018

I have to re-submit the PR, since I have deleted my forked repository accidentally when Github.com service was failed yesterday. The old closed PR is #126

Result of this PR: when user open a window for IM conversation:

  • If OMEMO enabled, there will be a system message "OMEMO encrypt conversation: Enabled"
  • If OMEMO available, there will have a system message "OMEMO encrypt conversation: Available"

There is a preference option to control whether to enable this function, it is enabled by default.

@cherti
Copy link

cherti commented Mar 4, 2019

I'd love to see this in lurch, it would be very beneficial to the user experience!

@codecov
Copy link

codecov bot commented Sep 21, 2019

Codecov Report

Merging #129 into dev will decrease coverage by 0.18%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #129      +/-   ##
==========================================
- Coverage   13.36%   13.18%   -0.19%     
==========================================
  Files           4        4              
  Lines        2110     2139      +29     
==========================================
  Hits          282      282              
- Misses       1828     1857      +29
Impacted Files Coverage Δ
src/lurch.c 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13cf6d1...ed53028. Read the comment docs.

@ghost ghost changed the title Show system message for OMEMO status in the conversation Implement system message for OMEMO status in the conversation Sep 27, 2019
@codecov-io
Copy link

codecov-io commented Sep 27, 2019

Codecov Report

Merging #129 into dev will decrease coverage by 0.13%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #129      +/-   ##
==========================================
- Coverage   13.36%   13.23%   -0.14%     
==========================================
  Files           4        4              
  Lines        2110     2131      +21     
==========================================
  Hits          282      282              
- Misses       1828     1849      +21
Impacted Files Coverage Δ
src/lurch.c 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13cf6d1...b77dc62. Read the comment docs.

Fix issue #125
And add a preference option
shtrom added a commit to shtrom/Lurch4Adium that referenced this pull request Feb 11, 2020
shtrom added a commit to shtrom/Lurch4Adium that referenced this pull request Feb 11, 2020
shtrom added a commit to shtrom/Lurch4Adium that referenced this pull request Feb 12, 2020
@shtrom
Copy link
Contributor

shtrom commented Jan 5, 2021

@wnereiz seems to have closed their account.

For safekeeping, I have made a copy of that branch here: https://github.com/shtrom/libpurple-lurch/tree/wnereiz-omemo-indication

@Neustradamus
Copy link

Neustradamus commented Feb 12, 2021

What is the status of this PR?

@Neustradamus
Copy link

@gkdr: I wish you a Happy New Year!

What is the status of this PR?

@gkdr
Copy link
Owner

gkdr commented Jan 10, 2022

hi @Neustradamus! thanks, i wish you a happy new year as well!

i think i never knew how to deal with this, as i don't know if it's a good decision UI-wise. the issue says that setting the conversation title is not enough and this would help remedy it, but i'm not sure i agree. one argument could be that it's better in finch, but i am not convinced a lot of people use it.

also, i think the initial reason why i didn't comment on this was that i was developing the signals API (which was finally added in 0.7.0), and now it is the right way to implement this feature. however, it wasn't done yet back then, and now it would probably be more or less a complete rewrite. i could base that rewrite on this commit in order to honor the contribution. however, the user could also just write /lurch status if the title is not clear enough. in fact, the feature would use the same signal as that command.

in the end, i think i hoped the contributor would come back, but i guess that day won't come (if you read this: i'm sorry it turned out like this. i really appreciate your effort!).
what do you think about what i wrote? maybe it's just finally time to close this PR and feature request issue.

@secutloled
Copy link

secutloled commented May 22, 2022

hi @Neustradamus! thanks, i wish you a happy new year as well!

i think i never knew how to deal with this, as i don't know if it's a good decision UI-wise. the issue says that setting the conversation title is not enough and this would help remedy it, but i'm not sure i agree. one argument could be that it's better in finch, but i am not convinced a lot of people use it.

also, i think the initial reason why i didn't comment on this was that i was developing the signals API (which was finally added in 0.7.0), and now it is the right way to implement this feature. however, it wasn't done yet back then, and now it would probably be more or less a complete rewrite. i could base that rewrite on this commit in order to honor the contribution. however, the user could also just write /lurch status if the title is not clear enough. in fact, the feature would use the same signal as that command.

in the end, i think i hoped the contributor would come back, but i guess that day won't come (if you read this: i'm sorry it turned out like this. i really appreciate your effort!). what do you think about what i wrote? maybe it's just finally time to close this PR and feature request issue.

No system message makes me feel insecure even though /lurch status says OMEMO is enabled. I would love to see this update. I use Pidgin.

When I start a private conversation with OTR I still see that lurch is active. Are the messages encrypted twice? With OTR and OMEMO? It's better to use OMEMO without OTR or that doesnt matter?

When I have OMEMO disabled and someone with OMEMO enabled message me I get the error that someone sended me an encrypted message but I can still read it. How does that work? In that same situation using OTR I can't read the message.

Every time when I close or restart Pidgin OMEMO is active without the need of starting it. With OTR I have to restart the private conversation. I have the feeling OMEMO is active but the messages are not encrypted because there is no clear message system.

On linux 'Enabled' doesn't mean something is actually running

edit:
I can verify the message are encrypted with OMEMO using pidgin --debug.

I would still love to have answers on the questions I asked

@gkdr
Copy link
Owner

gkdr commented May 30, 2022

hi @secutloled!

Every time when I close or restart Pidgin OMEMO is active without the need of starting it. With OTR I have to restart the private conversation.

that was actually one of the goals of the underlying signal protocol's designers who did not like the usability of OTR, i.e. having to manually start a session all the time. so like you already verified yourself looking at the messages sent - establishing a session once is enough. 🙂

When I start a private conversation with OTR I still see that lurch is active. Are the messages encrypted twice? With OTR and OMEMO? It's better to use OMEMO without OTR or that doesnt matter?

there is no detection of other plugins, so yes, both plugins will be active then. if it works i don't see why you shouldn't use both plugins, i just don't think it's necessary. the way the OTR plugin works is by using the actual message body for its data, which is why it's protocol-independent (and why people who don't have OTR see weird messages when you try to enable it with them). OMEMO is XMPP protocol specific and is integrated into it. so what i think is happening is that your plaintext message is encrypted using OTR and then put into the message body, which is then encrypted and replaced by OMEMO. and on the receiving end it's the other way round.

When I have OMEMO disabled and someone with OMEMO enabled message me I get the error that someone sended me an encrypted message but I can still read it. How does that work?

when disabling OMEMO, you only disable it on your own client. so the messages you send will not be encrypted. if you at any time used OMEMO with your contact, there exists an active session, which can still be use to decrypt incoming messages. i figured that's more helpful than to ignore the session on purpose, but i see how it can also be confusing.

i hope i was able to answer your questions!

@Fhiss
Copy link

Fhiss commented Jan 29, 2023

No system message makes me feel insecure even though /lurch status says OMEMO is enabled.

I turn on the Debug Window in parallel for control.
"Help" → "Debug Window"

@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 49 lines in your changes missing coverage. Please review.

Project coverage is 13.23%. Comparing base (13cf6d1) to head (b77dc62).
Report is 35 commits behind head on dev.

Files with missing lines Patch % Lines
src/lurch.c 0.00% 49 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #129      +/-   ##
==========================================
- Coverage   13.36%   13.23%   -0.14%     
==========================================
  Files           4        4              
  Lines        2110     2131      +21     
==========================================
  Hits          282      282              
- Misses       1828     1849      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants