Skip to content

Support for tcp transport#57

Merged
acrisci merged 9 commits into
altdesktop:masterfrom
M-Bab:master
Sep 2, 2020
Merged

Support for tcp transport#57
acrisci merged 9 commits into
altdesktop:masterfrom
M-Bab:master

Conversation

@M-Bab
Copy link
Copy Markdown
Contributor

@M-Bab M-Bab commented Aug 23, 2020

Implementation for D-Bus communication via TCP/IP. Previously there was also AUTH ANONYMOUS added to the authentifications but this is now already implemented - so no need to change anything there.

@acrisci
Copy link
Copy Markdown
Member

acrisci commented Aug 23, 2020

Is it possible to write tests for this?

@M-Bab
Copy link
Copy Markdown
Contributor Author

M-Bab commented Aug 25, 2020 via email

@M-Bab
Copy link
Copy Markdown
Contributor Author

M-Bab commented Aug 25, 2020

Okay I managed to get it fully working for a local test:

  1. Open a local socket:
    socat TCP-LISTEN:55556,reuseaddr,fork,range=127.0.0.1/32 UNIX-CONNECT:$(echo $DBUS_SESSION_BUS_ADDRESS | sed 's/unix:path=//g')

  2. This piece of code creates a notification via tcp:

import asyncio
import sys
from dbus_next.aio import MessageBus
#from dbus_next.message import Message, MessageType

async def main(args):
  bus = await MessageBus(bus_address="tcp:host=127.0.0.1,port=55556").connect()
  #reply = await bus.call(Message(destination='org.freedesktop.DBus',path='/org/freedesktop/DBus',interface='org.freedesktop.DBus',member='ListNames'))
  introspection = await bus.introspect('org.freedesktop.Notifications', '/org/freedesktop/Notifications')
  obj = bus.get_proxy_object('org.freedesktop.Notifications', '/org/freedesktop/Notifications', introspection)
  notification = obj.get_interface('org.freedesktop.Notifications')
  await notification.call_notify("test.py", 0, "", "DBus Test", "Test notification", [""], dict(), 3000)

if __name__ == "__main__":
  loop = asyncio.get_event_loop()
  basefilename = loop.run_until_complete(main(sys.argv))

It is somehow amazing that it works because neither d-feet nor notify-send were able to connect to the socat in my case.

Or what kind of tests specifically shall I add?

@acrisci
Copy link
Copy Markdown
Member

acrisci commented Aug 29, 2020

The only way I can ensure this will be stable with future changes if you add tests to the test suite for your use cases.

M-Bab added 2 commits August 30, 2020 19:13
- Add a check to the test_valid_addresses using a tcp address
- Added test_connection_via_tcp to the aio low level tests.
  A local network socket is opened with asyncio and the MessageBus
  is initialized which calls setup_socket. Afterwards its asserted
  that the network connection was successful and properly configured

Added a simple example to try out the DBus via TCP functionality.
@M-Bab
Copy link
Copy Markdown
Contributor Author

M-Bab commented Aug 30, 2020

Here you go 👍. Added 2 tests which are simple but should be very effective to protect the functionality. Also added an example for anyone willing to use it.

@pytest.mark.asyncio
async def test_connection_via_tcp():
server = await asyncio.start_server(None, '127.0.0.1', 55556)
bus = MessageBus(bus_address="tcp:host=127.0.0.1,port=55556")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you call connect() to send a message across?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Basically yes. You can specify the callback function and receive the "AUTH EXTERNAL ..." which can be replied with "OK" but then the system got stock (probably due to a lack of the Hello message).

But thats not ultimately necessary to unit test this function because the proof of a network connection seams good enough for me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd really like to see a message sent across the connection and successful auth annonymous. #54 for example will add assumptions that the socket is unix that would break this feature and have these tests still pass.

For an example of how to add dbus configuration to the test suites to listen on a tcp address, see the playerctl dockerfile and test suite which is based on this project.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see the point and I tried to get it working locally - but it did not work before the weekend ended. Somehow the session.conf line order fragility, systemd and even apparmor joined forces to prevent any feeling of success 😞

Maybe I am also not clever enough or at least not experienced enough in DBus/Docker for this. If you don't want to merge it, it's not a problem either because my company can live with my fork as well. The possibility of a TCP connection is all they needed - I just thought it would be nice to upstream it for other users and to ensure a future for this feature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok then this might be a bit more complicated. I can merge it with an issue to create tests if it's a big task. How exactly are you using it? The usage example you gave did some kind of socket forwarding.

@acrisci
Copy link
Copy Markdown
Member

acrisci commented Sep 2, 2020

This is fine for now 👍

@acrisci acrisci closed this Sep 2, 2020
@acrisci acrisci reopened this Sep 2, 2020
@acrisci acrisci merged commit de1b07c into altdesktop:master Sep 2, 2020
@M-Bab
Copy link
Copy Markdown
Contributor Author

M-Bab commented Sep 2, 2020

Thanks a lot. I am sorry I could not get a good working test for the combination network+anonymous auth via dbus config. Some colleagues of mine figured it out on an embedded device but it didn't work for me on any x86 machine.

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.

2 participants