Skip to content

Fix OMEMO devicelist access model by reconfiguring it#1591

Merged
jubalh merged 7 commits intoprofanity-im:masterfrom
paulfariello:fix/omemo-devicelist-access-model
Aug 20, 2021
Merged

Fix OMEMO devicelist access model by reconfiguring it#1591
jubalh merged 7 commits intoprofanity-im:masterfrom
paulfariello:fix/omemo-devicelist-access-model

Conversation

@paulfariello
Copy link
Contributor

Fix #1538

Handle precondition-not-met returned when inserting our device in the device list. Try to reconfigure it properly and give up if it fails again.

Also add support for missing device list node. It rely on auto-create. We should ensure this feature is enabled and fallback to a proper create with config otherwise.

@paulfariello paulfariello force-pushed the fix/omemo-devicelist-access-model branch from 986c921 to 4d93df4 Compare August 19, 2021 08:02
@jubalh
Copy link
Member

jubalh commented Aug 19, 2021

Awesome! Thanks for this!

@jubalh jubalh added this to the 0.11.1 milestone Aug 19, 2021
@jubalh jubalh added the OMEMO label Aug 19, 2021
}

if (g_strcmp0(xmpp_stanza_get_name(pubsub_error), "precondition-not-met") == 0) {
static gboolean reconfigured = false;
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure whether this works as intended.

Copy link
Member

Choose a reason for hiding this comment

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

we will after the first run always be in true but it could be that we switch accounts without restarting profanity. or it could be that we need to republish because while profanity was running another client did something to the devicelist and we need to republish.

@jubalh
Copy link
Member

jubalh commented Aug 19, 2021

It rely on auto-create. We should ensure this feature is enabled and fallback to a proper create with config otherwise.

I asked around and heard that the auto-create is pretty common. So we probably don't have to bother with adding the fallback.

@jubalh jubalh self-requested a review August 20, 2021 13:53
@jubalh jubalh merged commit 1d08d8c into profanity-im:master Aug 20, 2021
@jubalh
Copy link
Member

jubalh commented Aug 20, 2021

There is the corner case with the static variable mentioned in the review above.

Instead of using a static variable we maybe could also pass it along the userdata parameter. That's a long line of calls. According to paul:

  • _omemo_devicelist_publish_result
  • omemo_devicelist_configure
  • _omemo_devicelist_configure_submit
  • _omemo_devicelist_configure_result
  • omemo_devicelist_request
  • _omemo_receive_devicelist
  • omemo_set_device_list
  • _handle_own_device_list
  • omemo_devicelist_publish
  • _omemo_devicelist_publish_result

omemo_devicelist_request and _handle_own_device_list seem to be not so easy.

The corner cases probably will be quite rare anyways.

@paulfariello
Copy link
Contributor Author

The issue with the static boolean could arise in two cases:

  • deconnection from a client that already reconfigured it's own device list. Followed by a connection to another account that also have a misconfigured device list.
  • running client that has already reconfigured it's own device list. With another client misconfiguring the existing devicelist and removing the running client from the list.
    Each time the effect would be that profanity won't be able to reconfigure correctly the devicelist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[OMEMO] profanity should fix the PEP access model if wrongly configured by another client

2 participants