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

Message carbons are not logged to chat log file #1181

Closed
janteau opened this issue Aug 26, 2019 · 19 comments
Closed

Message carbons are not logged to chat log file #1181

janteau opened this issue Aug 26, 2019 · 19 comments
Assignees
Labels
Milestone

Comments

@janteau
Copy link

@janteau janteau commented Aug 26, 2019

Expected Behavior

In the chat logs messages sent from Profanity along with messages sent by other clients (carbons) should be logged into the log file.

Current Behavior

Chat logs contain messages received and messages sent by the Profanity client, but do not include messages sent by the user's other clients. I am using OMEMO encryption in the chats that I experience this behavior (/omemo log on is enabled).

Possible Solution

Steps to Reproduce (for bugs)

  1. Turn on chat logging.
  2. Log into account through Profanity and another client at the same time.
  3. Send messages from Profanity as well as another client.
  4. Messages sent by other client are present in window.
  5. Inspect chat logs for current conversation.

Context

Environment

Profanity, version 0.7.0

Build information:
XMPP library: libmesode
Desktop notification support: Disabled
OTR support: Disabled
PGP support: Disabled
OMEMO support: Enabled
C plugins: Enabled
Python plugins: Enabled (2.7.15+)
GTK icons: Disabled

System: Ubuntu Linux 18.04

glib2.0 Version: 2.56.4-0ubuntu0.18.04.4

@jubalh jubalh added the improvement label Aug 27, 2019
@jubalh jubalh added this to the 0.8.0 milestone Aug 27, 2019
@jubalh jubalh self-assigned this Sep 3, 2019
@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Sep 3, 2019

Code doesnt log carbons indeed.
Also in carbon case we dont make sure that message->plain is always set.

Will fix this when I have time.

@jubalh jubalh closed this in 09b6fc9 Sep 4, 2019
@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Sep 4, 2019

@janteau please try again with latest master.

Seems like MUC logging is totally broken too. Funny noone noticed it :-)
I'll adress this in a seperate issue.

Logging of carbons (on 1:1) should now work.

@janteau

This comment has been minimized.

Copy link
Author

@janteau janteau commented Sep 6, 2019

@jubalh This still doesn't seem to work for me. I did a git pull from master this evening as you can see from the version number below. Messages sent by me using profanity to users on my roster are logged, but the messages sent from Conversations are not logged (but do show on the screen).

Enabling and disabling OMEMO made no difference.

Version 0.7.0dev.master.09b6fc9a

@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Sep 6, 2019

@mdosch can you also test this once you find time?

@mdosch

This comment has been minimized.

Copy link
Contributor

@mdosch mdosch commented Sep 8, 2019

I just had a quick look at my logfiles:

  • 1-1
    • A 1-1 message (OMEMO encrypted) sent by another client (Conversations) is not showing up, there is even no logfile for that day.
    • A message sent by profanity creates the logfile and shows up in the log
  • MUC
    • A message sent by profanity is appearing in the log twice
    • A message sent by conversations is appearing in the log once
    • MUC PM sent by conversations or profanity don't show up
    • MUC PM received is logged twice (one time as sent by me and one time as received from the contact)

So it seems logging is broken. Any special use cases you like to be tested?

profanity --version
Profanity, version 0.7.0dev.master.09b6fc9a
Copyright (C) 2012 - 2019 James Booth <boothj5web@gmail.com>.
Copyright (C) 2019 Michael Vetter <jubalh@iodoru.org>.
License GPLv3+: GNU GPL version 3 or later <https://www.gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Build information:
XMPP library: libstrophe
Desktop notification support: Enabled
OTR support: Enabled (libotr 4.1.1)
PGP support: Enabled (libgpgme 1.13.1-unknown)
OMEMO support: Enabled
C plugins: Enabled
Python plugins: Enabled (2.7.16+)
GTK icons: Enabled
@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Sep 8, 2019

Trying to have a look soon. Thanks for the feedback guys!
My tests where with profanity and Gajim, and this worked.

@jubalh jubalh reopened this Sep 8, 2019
@janteau

This comment has been minimized.

Copy link
Author

@janteau janteau commented Sep 15, 2019

@jubalh I took a look at my log files over the past couple of days, and it appears messages from my Gajim client were also not logged.

@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Sep 18, 2019

Gosh. There is a lot of stuff wrong here I think.
Findings so far:

In src/xmpp/message.c we have message handlers, like _handle_carbons and _private_chat_handler etc.
We make sure that message->plain is set there. If it's not we set it from message->body.

Later in src/event/server_events.c we have _sv_ev_incoming_plain which takes care of incoming messages on a higher level. Printing them to the window and logging it to a file.

There we have if (message->body) { ... message->plain = strdup(message->body)...} so it could be that we assigned the correct value to plain and now overwrite it.

Also we use chatwin_incoming_msg() in _sv_ev_incoming_plain for logging the message. But it could be that it needs to be logged as an outgoing message in case of a carbon.

And in sv_ev_incoming_carbon() I added chat_log_msg_out(message->jid->barejid, message->plain); so it should actually log twice.
This time using out.
And there is another mistake here message->jid is the from the inner forwarded stanza of the carbon AFAIK. So this will be the partner who we actually send the original message to, and not us who we sent the carbon to. We should probably use connection_get_fulljid(); to get our jid here. Or need to think whether message->jid needs to be our jid in the carbon case...

@jubalh jubalh closed this in 523681a Oct 4, 2019
@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Oct 4, 2019

@janteau please test with latest master and give feedback here.

@janteau

This comment has been minimized.

Copy link
Author

@janteau janteau commented Oct 7, 2019

@jubalh I started testing this tonight. It seems to be working so far. Messages sent from my other client (Conversations) seem to be appearing in my logs now. I will comment again if I find something not working with this. Thanks!

Unrelated I think, I did find a message that appeared in Profanity from a contact that did not get logged into the log file (while I was testing). I can't reproduce it (the following messages were logged). If I can find how to reproduce it, I'll open a separate issue report.

@janteau

This comment has been minimized.

Copy link
Author

@janteau janteau commented Oct 8, 2019

@jubalh Maybe this is expected, but my carbons are still not logged in MUCs. Seems to be working for 1-1 chats.

@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Oct 9, 2019

@weiss is it possible that messages to a MUC from another client are not also sent via Carbons?
So the only way to get them is MUC reflection?

@janteau this is a new bug then, introduced by 147be3a.
Where I stop logging incoming MUC messages when they come from our own nick.

I do this because we log the message already as an outgoing MUC message here: https://github.com/profanity-im/profanity/blob/master/src/event/client_events.c#L346

We log the outgoing one so we have an easier time with OMEMO.

@jubalh jubalh reopened this Oct 9, 2019
@weiss

This comment has been minimized.

Copy link
Contributor

@weiss weiss commented Oct 9, 2019

is it possible that messages to a MUC from another client are not also sent via Carbons?

Yes, type='groupchat' messages aren't carbon-copied.

So the only way to get them is MUC reflection?

Yes (and from the room's MAM archive/history).

@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Oct 9, 2019

So when receiving incoming MUC messages I have no way to know if it was sent from the current client or another one..

@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Oct 9, 2019

@paulfariello is it OMEMO wise possible to stop logging outgoing MUC messages and only log incoming ones?
Users have to trust their own keys for that to work, I guess.
We still know whether its OMEMO encrypted or not, so we can move the 'redact' logic to the incoming part only.

In this case we could remove outgoing logging and just log incoming stuff.

@weiss

This comment has been minimized.

Copy link
Contributor

@weiss weiss commented Oct 9, 2019

So when receiving incoming MUC messages I have no way to know if it was sent from the current client or another one..

You could add an origin ID to track your own messages. (Or just the normal message ID, but old servers might mess with that: XEP-0045 didn't require MUC services to maintain the original message ID until revision 1.31 from 2018.)

@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Oct 15, 2019

I'll close this issue and reopen #1201 instead.
Since this issue is actually closed but the fix in 1201 is not correct in case one uses multiple clients in MUC.

@jubalh jubalh closed this Oct 15, 2019
@jubalh

This comment has been minimized.

Copy link
Member

@jubalh jubalh commented Oct 21, 2019

@janteau #1201 is closed now. So the issue should be fixed! :)

@janteau

This comment has been minimized.

Copy link
Author

@janteau janteau commented Oct 27, 2019

@jubalh This seems to be working! Thanks! My 1:1 received messages do not seem to get logged now, not sure if it's related. I therefore opened a new issue #1214.

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.