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

[OMEMO]: Fix bundle publishing #1496

Merged
merged 7 commits into from Mar 10, 2021
Merged

[OMEMO]: Fix bundle publishing #1496

merged 7 commits into from Mar 10, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 8, 2021

This PR fixes #1492 and subsumes #1486.

@jubalh
Copy link
Member

jubalh commented Mar 8, 2021

NOTE: Although the option "pubsub#max_items=max" is REQUIRED, this
option is not supported by some servers.

Do you know which ones?

@ghost
Copy link
Author

ghost commented Mar 8, 2021 via email

src/xmpp/omemo.c Outdated Show resolved Hide resolved
src/xmpp/omemo.c Outdated Show resolved Hide resolved
src/xmpp/omemo.c Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Mar 8, 2021 via email

@ghost ghost requested a review from jubalh March 9, 2021 09:48
@ghost
Copy link
Author

ghost commented Mar 9, 2021

Hi, I just checked the diff again. I don't see any accidental comments any more. I've also tested it once more.

One notable change (introduced by @StefanKropp) that I noticed is that when OMEMO is activated by default but no crypto material has been generated yet, the following will be written to the console:

OMEMO: cannot publish crypto materials before they are generated

This should remind the user to generate crypto material first.
Should I remove some of the debug/warning logs?

@jubalh
Copy link
Member

jubalh commented Mar 9, 2021

Thanks for the updates.

I think the PR looks good now. I'm uncertain whether some of the log_info() should actually be log_debug() though. But I see that this is debatable and there is no common rules for it yet in Profanity.

@StefanKropp could you also give it a quick review?

StefanKropp and others added 7 commits March 9, 2021 17:47
Reduce the request during startup of profanity
Removed omemo_start_sessions from sv_ev_roster_received
Also "handle" some errors in `_omemo_bundle_publish_configure` if the
stanzas can't be found
Use the following options in `omemo_bundle_publish()`:

- "pubsub#persist_items" = "true"
- "pubsub#access_model" = "open"

The same options are also used in Gajim.

I've tested this on two different servers. The bundle was successfully
added as a new PEP node. Test cases:

1. Normal use on my main account
2. Log in into a fresh tesst account on a different server
3. `/omemo clear_device_list`. In this case, the client(s) may have to
   be restarted.

Note: In `_omemo_bundle_publish_result`, there's a route that is taken
when the bundle publish stanza failed. In this case, the node is
configured manually, i.e. the access_model is set to 'open'. I have
manually tested this case, but this case didn't naturally occur for me.

Note: The option "pubsub#max_items=max" is REQUIRED for the bundle
publication, as per XEP-0384. However, this is not done in other clients
(I've checked the source code of Gajim and Conversations), and it is
also not supported by Prosody. Cf. <xsf/xeps#988>.
@ghost
Copy link
Author

ghost commented Mar 9, 2021

I just force-pushed to fix the mail address in my commits. I didn't change anything else.

@jubalh
Copy link
Member

jubalh commented Mar 10, 2021

Got an ACK from @StefanKropp via PM.

@jubalh jubalh merged commit 96580f9 into profanity-im:master Mar 10, 2021
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.

OMEMO: Recipent lists profanity's key as inactive
2 participants