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.

Sign up for GitHub

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

Jump to bottom

OMEMO-encrypted MUC logging functionality #94

Merged
merged 11 commits into from
Aug 13, 2021
Merged

OMEMO-encrypted MUC logging functionality #94

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
170b5ee
added basic OMEMO-encrypted MUCs logging functionality, still has bugs
kssytsrk Aug 9, 2021
641a446
attempt to fix conference logger plugin
kssytsrk Aug 10, 2021
c3a7991
clean up
kssytsrk Aug 10, 2021
09b4626
fix logger plugin
kssytsrk Aug 12, 2021
71a5e0a
made OMEMO plugin display right MUC nicks
kssytsrk Aug 12, 2021
55b6939
fixed bug where OMEMO logs outgoing message even if not supposed to b…
kssytsrk Aug 12, 2021
5d138f0
clean up
kssytsrk Aug 12, 2021
78801f0
removed unneeded qdebug call
kssytsrk Aug 12, 2021
05db402
added comments
kssytsrk Aug 12, 2021
ecdd025
fixed review suggestions
kssytsrk Aug 13, 2021
fbc9d89
update changelog.txt, bump versions
kssytsrk Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion generic/conferenceloggerplugin/conferenceloggerplugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ bool ConferenceLogger::incomingStanza(int account, const QDomElement &stanza)
{
if (enabled) {
if (stanza.tagName() == "message") {
if (stanza.attribute("type") == "groupchat") {
if (stanza.attribute("type") == "groupchat" && stanza.firstChildElement("encrypted").isNull()) {
QString from = stanza.attribute("from");
QStringList List = from.split("/");
QString room = List.takeFirst();
Expand Down
74 changes: 72 additions & 2 deletions generic/omemoplugin/src/omemoplugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,26 @@ bool OMEMOPlugin::decryptMessageElement(int account, QDomElement &message)
processEncryptedFile(account, message);
}

// logging functionality for OMEMO-encrypted groupchats
if (message.attribute("type") == "groupchat") {
QString from = message.attribute("from");
QStringList List = from.split("/");
QString room = List.takeFirst();
if (!List.isEmpty()) {
from = List.join("/");
}
if (from != m_mucNicks[room]) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
QStringList List = from.split("/");
QString room = List.takeFirst();
if (!List.isEmpty()) {
from = List.join("/");
}
if (from != m_mucNicks[room]) {
QString room = from.section('/', 0, 0);
QString nickname = from.section('/', 1);
if (nickname != m_mucNicks.value(room)) {

prevent empty insertions if for some reason room is not in the map.

QString Stamp = message.firstChildElement("x").attribute("stamp");
QDomElement body = message.firstChildElement("body");
if (!body.isNull()) {
QString Text = body.text();
QString MyJid = m_accountInfo->getJid(account);
MyJid = MyJid.replace("@", "_at_");
logMuc(room, from, MyJid, Text, Stamp);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
logMuc(room, from, MyJid, Text, Stamp);
logMuc(room, nickname, MyJid, Text, Stamp);

taking into account my previous suggestion

}
}
}

return true;
}

Expand Down Expand Up @@ -313,8 +333,16 @@ bool OMEMOPlugin::outgoingStanza(int account, QDomElement &xml)
return false;
}

if (xml.nodeName() == "presence" && !xml.hasAttributes()) {
m_omemo->accountConnected(account, m_accountInfo->getJid(account));
if (xml.nodeName() == "presence") {
if (!xml.hasAttributes())
m_omemo->accountConnected(account, m_accountInfo->getJid(account));
// get all MUC nicks of the current account for groupchat logging
// functionality
else {
QStringList room_nick = xml.attribute("to").split("/");
Copy link
Member

Choose a reason for hiding this comment

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

I believe this thing may interfere with some non-muc presences. Like for example when we send a direct presence to a single contact.
Please at least add FIXME here.

m_mucNicks.insert(room_nick[0], room_nick[1]);
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we never clean it.
from the first glance it may look like a memory leak. But if it's rather a small cache for Psi life time, likely it's fine.
And I'd rather also use sections here to avoid side effects splitting by "/"

}

}

return false;
Expand All @@ -330,6 +358,22 @@ bool OMEMOPlugin::encryptMessageElement(int account, QDomElement &message)
return false;
}

// logging functionality for OMEMO-encrypted groupchats
if (message.attribute("type") == "groupchat") {
Copy link
Member

Choose a reason for hiding this comment

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

Please prefer wrapping string literals with QLatin1String. This way we improve performance by avoiding string conversion from "maybe unicode" to "unicode".

QLatin1String("groupchat")

QString room = message.attribute("to");
QString from = m_mucNicks[room];
Copy link
Member

Choose a reason for hiding this comment

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

m_mucNicks.value(room)

will avoid invalid empty nickname insertion if room is not there.
Also it may be necessary to do some initialization if it's not there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but wouldn't .value() also return an empty string if the room is not there (unless the second parameter of defaultValue is specified, but I'm not sure what kind of default value would be good for this)? Qt docs say so, although I might be misunderstanding it.

Copy link
Member

Choose a reason for hiding this comment

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

It will return empty string. that's correct.
Since nickname can't be empty, you can check for emptiness and maybe do some initialization if empty.

Imagine a case where you enable OMEMO plugin already after joining. So you missed the initial presence and have to live with this somehow. (drop everything encrypted / force rejoin / ask user what to do)

Copy link
Member

Choose a reason for hiding this comment

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

I'd go with the easiest way improving the stuff with consequent MRs :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so I changed the behavior to logging the full jid instead of the nick in case the nick is empty. Would that be fine for now? Can't think of any elegant solution to the problem yet

Copy link
Member

Choose a reason for hiding this comment

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

yep. should be good for now.
but technically all the MUC info has to be available somewhere in Psi including nicks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, there is a mucNicks() function in ContactInfoAccessingHost, but that just gives a list of nicks from a MUC (without the real jids), so I suppose the realJid() function could be ran on each of the nicks until the one associated with the needed account is found... But that would probably be slow, especially if the MUC is big (although I strongly doubt that there exist big OMEMO-encrypted MUCs, as that's very uncomfortable since everyone seems to have to have everyone else in their roster to encrypt all messages). Not sure what other options are there to fix this.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to modify plugin interfaces if anything is needed.
We didn't release any stable plugins sdk yet. So it's Ok to change it.
I think after the coming release we can think about some stabilization of the plugins API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, okay. I'll see what can be done, would be great to remove the presence tracking hack as a whole

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

if (m_omemo->isEnabledForUser(account, room)) { // only log if encryption is enabled
QString Stamp = message.firstChildElement("x").attribute("stamp");
QDomElement body = message.firstChildElement("body");
if (!body.isNull()) {
QString Text = body.text();
QString MyJid = m_accountInfo->getJid(account);
MyJid = MyJid.replace("@", "_at_");
logMuc(room, from, MyJid, Text, Stamp);
}
}
}

return m_omemo->encryptMessage(m_accountInfo->getJid(account), account, message);
}

Expand Down Expand Up @@ -544,4 +588,30 @@ bool OMEMOPlugin::execute(int account, const QHash<QString, QVariant> &args, QHa

return false;
}

// the code partly taken from the Conference Logger plugin
void OMEMOPlugin::logMuc(QString room, const QString &from, const QString &myJid,
const QString &text, QString stamp)
{
room = room.replace("@", "_at_");
room = "_in_" + room;
if (stamp.isEmpty()) {
stamp = QDateTime::currentDateTime().toString("yyyy-MM-dd hh:mm:ss");
} else {
stamp.insert(4, "-");
stamp.insert(7, "-");
stamp.replace("T", " ");
}
QFile file(m_applicationInfo->appHistoryDir() + QDir::separator() + myJid + room + ".conferencehistory");
if (file.open(QIODevice::WriteOnly | QIODevice::Append)) {
QTextStream out(&file);
out.setCodec("UTF-8");
out.setGenerateByteOrderMark(false);
#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 0)
out << stamp << " " << from << ": " << text << Qt::endl;
#else
out << stamp << " " << from << ": " << text << endl;
#endif
}
}
}
2 changes: 2 additions & 0 deletions generic/omemoplugin/src/omemoplugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ private slots:
void updateAction(int account, const QString &user);
void processEncryptedFile(int account, QDomElement &xml);
void showOwnFingerprint(int account, const QString &jid);
void logMuc(QString room, const QString &from, const QString &myJid, const QString &text, QString stamp);

private:
bool m_enabled = false;
Expand All @@ -147,6 +148,7 @@ private slots:
EventCreatingHost * m_eventCreator = nullptr;
PsiAccountControllingHost * m_accountController = nullptr;
OptionAccessingHost * m_optionHost = nullptr;
QMap<QString, QString> m_mucNicks;
};
}
#endif // PSIOMEMO_OMEMOPLUGIN_H