interfaces/builtin: add evolution interfaces #2226

renatofilho · Oct 27, 2016

Implement EDS [calendar, contact] interface.

niemeyer · Oct 28, 2016

niemeyer requested changes Oct 28, 2016

View changes

Haven't gone in detail through the interface, but a few early comments:

Rather than an attribute saying which services one is trying to access, these should most likely be two independent interfaces
We need a more descriptive name for this interface. On a quick read, "evolution-calendar" and "evolution-address-book" might be candidates

renatofilho · Oct 28, 2016

@niemeyer

I have been discussing about this interface with @jdstrand, our current idea is to have an interface named: unity8-pim, which contains all services related with contact and calendar, including "sync" services.

We need to provide interfaces to access:

EDS - [contact, calendar]
SyncEvolution DBUS API - [calendar sync]
Buteo DBUS API - [contacts sync]

What do you think about that?

jdstrand · Nov 9, 2016

@niemeyer - as @renatofilho said, we've discussed this a bit. As proposed, this interface should be renamed 'unity8-pim' because all the policy and accesses for it are for using Unity8 application APIs that simply happen to use eds under the hood. Furthermore, the various sync services are specific to Unity8 and only cover the parts of eds that Unity8 cares about. In other words, this is not a general-purpose 'evolution-contacts' or 'evolution-calendar' interface. In fact, I prefer this under 'unity8-*'[1] to leave the door open for proper 'evolution-contacts' or 'evolution-calendar' interfaces down the line (again, instead of this interface which is very limited).

An argument can be made to split out the various interfaces that @renatofilho mentioned (I'll provide possible names to demonstrate what this might look like):

unity8-contacts, unity8-calendar (EDS - [contact, calendar])
unity8-calendar-sync (SyncEvolution DBUS API - [calendar sync])
unity8-contacts-sync (Buteo DBUS API - [contacts sync])

That said, unity8-pim with attributes like this PR is trying to do seems more aligned with the Ubuntu Personal Unity8 implementation in that the slot implementation shares the same backend currently (eds) and breaking out the sync functionality seems somewhat strained and the Unity8 implementation views all of these as a unit (which suggests a single interface).

An alternative middle ground might be to do:

unity8-contacts with optional 'sync' attribute
unity8-calendar with optional 'sync' attribute

I'm going to defer looking at this PR further until we decide on the name of the interface(s).

[1] note that I am advocating for 'unity8-' for naming unity8/ubuntu personal interfaces unless the interface is general and not unity8-specific.

niemeyer · Nov 22, 2016

Looking at the interface, I still don't quite understand why this isn't a straightforward break out of interface based on the functionality offered, just like every other case we have.

For example, evolution-calendar gives access to the Evolution calendar. evolution-contacts gives access to the evolution contacts service.

This is not even about unity8, apparently.

renatofilho · Nov 22, 2016

I do not mind splitting it in several interfaces.

My proposal following this idea is:

[Calendar]
evolution-calendar - EDS calendar

[Contacts]
evolution-contacts - EDS contacts
unity8-contacts - Addressbook Service (keep it as unity8 in case we decide to use a different server)

[Sync]
buteo - For use buteo [contact sync]
syncevolution - For syncevolution service [calendar sync at the moment, we have plans to move it to buteo]

@jdstrand what do you think about that?

niemeyer · Nov 22, 2016

(keep it as unity8 in case we decide to use a different server)

If you change the service, you use another interface. It doesn't make much sense to allow people to talk to the evolution service under a unity8 name, and then break every old software that depended on that interface to talk to evolution because unity8 picked something else.

niemeyer · Nov 22, 2016

And by "change the interface" I really mean "use another interface" (fixed comment above).

jdstrand · Nov 22, 2016

@niemeyer fyi, in the case of unity8, the applications themselves aren't programming to evolution-- they are programming to qtubuntu APIs that happen to use evolution under the hood to call out to evolution over dbus. AIUI, the qtubuntu folks want the flexibility to change out the backend if they need to.

jdstrand · Nov 22, 2016

@renatofilho - honestly if we were to break these out into evolution-* then I think I would prefer unity8-evolution-* instead since the evolution that you are providing via qtubuntu may not be compatible with an evolution that gnome would provide. That gets somewhat complicated though since the dbus apis are likely very similar, yet different and I doubt that a slot implementation of unity8-evolution-* and a slot implementation of evolution-* would be coinstallable. Conversely, I doubt that a unity8 plugging app would work with another slot implementation of evolution-*.

niemeyer · Nov 22, 2016

That sounds problematic on itself. Having dbus evolution interfaces that disagree with what evolution itself does would make me pretty upset if I was an evolution developer. Is that really what's going on?

renatofilho · Nov 22, 2016

@jdstrand yes if we are using evolution-* we need somehow specify the version, since this DBUS API is private and they change it a lot.

renatofilho · Nov 22, 2016

@niemeyer, to give you a bit more of context.

Yes the EDS DBUS API change a lot (this is a private API), the docs says to never uses that, instead use the glib client library and that one we are using on QtContacts/QtOrganizer implementation.

About ubuntu applications: All apps uses QContacts/QtOrganizer API.

The code looks like that:

QContactManager mrg;
mgr->setManager("[eds | folks | ubuntu]"); // you can specify which backend do you want to use.
// Default one for ubuntu is "ubuntu" which make uses of
// "com.canonical.pim.AddressBook".
// For calendar we make access directly to eds and the default
// one is "eds"
var contacts = mrg->contacts();

renatofilho · Nov 24, 2016

@jdstrand @niemeyer We all agree with the last suggestion from @jdstrand ? Can I go ahead and implement this way?

niemeyer · Nov 24, 2016

Yes, let's please go with unity8-contacts and unity8-calendar then, as suggested by @jdstrand.

I still don't understand the "sync" distinction, though. Why would that be a separate setting or interface? Isn't that giving access to the exact same data for read/write than the interfaces above would already give alone? The fact the API is different doesn't matter much.. the question is what is being written to or read from, isn't it?

renatofilho · Nov 25, 2016

@niemeyer about the sync.

The idea is to keep it apart because the apps should not be allowed to sync. The sync itself will be done by the system core (push notifications, interval based, etc..). But since our push notification does not support any of our services (google, owncloud, etc..), as workaround we are allowing the apps to request syncs, this way the user can request a sync from the server.

Today local changes are intercepted by the system and trigger a new sync, but remote changes are not notified to our local system and the user needs to manually ask for a sync.

Bug related with that: https://bugs.launchpad.net/ubuntu/+source/address-book-app/+bug/1319546

zyga · Dec 1, 2016

zyga requested changes Dec 1, 2016

View changes

There are a few fixes you should make. I've suggested them inline. When you are done please also merge master into your branch to make sure we carry any other fixes that might prevent this from working.

Missing NewUnity8ContactsInterface()

done

Please go fmt in this directory to correct this.

Feels like this needs the reservedForOS: true flag as this is just about the client side.

In fact this is the declaration of the slot and the interface. If you take a close look you will see that I am returning a "unity8PimInterface" instance and not the common interface.

Feels like this needs the reservedForOS: true flag as this is just about the client side.

same as before

Can you please remove this?

done

renatofilho · Dec 2, 2016

changes are ready for review again.

jdstrand · Dec 5, 2016

jdstrand requested changes Dec 5, 2016

View changes

Can you explain how unity8-pim relates to unity8-calendar and unity8-contacts? I was expecting only these two and not unity8-pim....

What is the '.*' referring to here? As written, it doesn't seem specific to calendar.

Evolution export several services with name like "org.gnome.evolution.dataserver.Subprocess.Backend.Calendarx6605x2"

Can you then use this instead: name=org.gnome.evolution.dataserver.Subprocess.Backend.Calendar*?

done

I think nearly all of these should be moved to ConnectedSlotAppArmor. In snappy, we mediate both what slot implementations the client can connect to (ConnectedPlug) and what clients the slot implementation can connect to (ConnectedSlot). PermanentSlot policy should only contain enough policy so that the service may start listening for clients. As such, please rework these rules into ConnectedSlotAppArmor, using ###PLUG_SECURITY_TAGS###. There are plenty of examples in interfaces/builtin/* to get you started.

These will only work for classic systems where eds is shipped as a deb (ie, peer=(label=unconfined) says that the connected client can connected to any unconfined process that uses these paths/interfaces/methods/etc).

Is this interface expected to work on classic systems? If so, then you'll want to move these unconfined rules to unity8CalendarConnectedPlugAppArmor. See the ofono interface for an example.

Please adjust unity8CalendarConnectedPlugAppArmor to use ###SLOT_SECURITY_TAGS### for the peer label (again, the ofono interface can by a guide).

This is problematic-- the dbus interface is meant for system bus policy, but eds is a session service. I'm not sure how to advise here since snappy session services have not been designed yet....

The dbus backend is currently for the system bus, but you have only 'bus=session' rules. As such, please remove use of this backend for now. This can be updated when tvoss/the Personal team update snapd for session services.

done

Everything I said about the calendar interface applies to the contacts interface (backend is not contacts-specific, use ###PLUG_SECURITY_TAGS### in ConnectedSlot, update PermanentSlot, update ConnectedPlug for classic and ###SLOT_SECURITY_TAGS###, and dbus is problematic.

fixed

renatofilho · Dec 5, 2016

unity8-pim is the base class for both calendar and contacts. It contains all common rules, since both interfaces share alot of apparmor policies. Basically it contains a base policy that is appended to individual policies (calendar and contacts)

And it contains the Source manager policy that is common for both interfaces.

jdstrand · Dec 5, 2016

@renatofilho - re unity8-pim> ah, I didn't look closely at it. I thought it was a different interface altogether. Organizationally, this makes me think that perhaps these should be organized differently. Maybe unity8_pim_internal.go? Perhaps @zyga would like to comment on that.

renatofilho · Dec 6, 2016

@jdstrand renamed unity8-qtpim to unity8-qtpim-common to match with the concept already used by "commonInterface".

chipaca · Dec 14, 2016

What's the status of this PR?

renatofilho · Dec 14, 2016

@chipaca this is ready for a second round of review. It is on @jdstrand list, he should have some time soon to look at it.

jdstrand · Dec 14, 2016

jdstrand requested changes Dec 14, 2016

View changes

Please apply my comments in calendar to contacts and common (eg, dbus, Usage, 'reserved', etc)

You can remove the 'Usage' metatag here and everywhere. It is no longer needed now that we have the base declaration.

done

This is already in unity8-common

done

I think all of these are likely 'receive' only, no? Perhaps not. Note that the 'send's here mean that the slot implementation can send to any unconfined process with the specified paths....

I think that is ok. If the app is unconfined I do not see problem in sending the information.

This was based on ofono interface and it has a similar rule with (send/receive).

Since slot implementations require a snap declaration to be in the store, I'm 'ok' with this access to unconfined. Note that my comment was not about unconfined talking to the slot (which should always be fine), it is about the slot perhaps being able to abuse its privilege and send weird stuff to unconfined. Can you try out these rules with just 'receive' and report back? They might be required, but it is worth check if they're not.

Re ofono> ofono was different; the rule is there so an ofono snap can talk to a network-manager deb.

We discussed this on IRC. There isn't a clear requirement for 'send' to unconfined, so Renato will remove it.

done

Same with this one.

done

Can you update this description. The language is very 'click' oriented. It should simply be:

# Allow connected clients to communicate with calendar service via DBus

done

This is problematic-- the dbus interface is meant for system bus policy, but eds is a session service. I'm not sure how to advise here since snappy session services have not been designed yet....

The dbus backend is currently for the system bus, but you have only 'bus=session' rules. As such, please remove use of this backend for now. This can be updated when tvoss/the Personal team update snapd for session services.

done

Please remove (see above)

done

I would say to add something for the DBus backend here, but won't since I have asked you to remove it :)

Copy/paste issue: ofono

done

Copy/paste

done

Please remove the blank line.

done

This isn't needed since it is already in unity8PimCommonPermanentSlotAppArmor

done

Please remove the extra blank line.

done

This gives complete access to all DBus services provided by the slot and renders all other rules for communicating with the slot meaningless. What are you trying to achieve with this rule?

Please remove Usage and note about 'Reserved because...'

done

connect, getpeername, getsockopt, setsockopt, socketpair and socket are all in the default template.

done

Please remove Usage and note about 'Reserved because...'

done

connect, getsockopt and socket are all in the default template.

done

jdstrand · Dec 15, 2016

jdstrand requested changes Dec 15, 2016

View changes

Thanks for all the changes! This is getting close.

Nipick. Can you remove the space before 'gives'.

done

I think all of these are likely 'receive' only, no? Perhaps not. Note that the 'send's here mean that the slot implementation can send to any unconfined process with the specified paths....

I think that is ok. If the app is unconfined I do not see problem in sending the information.

This was based on ofono interface and it has a similar rule with (send/receive).

Since slot implementations require a snap declaration to be in the store, I'm 'ok' with this access to unconfined. Note that my comment was not about unconfined talking to the slot (which should always be fine), it is about the slot perhaps being able to abuse its privilege and send weird stuff to unconfined. Can you try out these rules with just 'receive' and report back? They might be required, but it is worth check if they're not.

Re ofono> ofono was different; the rule is there so an ofono snap can talk to a network-manager deb.

We discussed this on IRC. There isn't a clear requirement for 'send' to unconfined, so Renato will remove it.

done

I just noticed this. Can you change this to const like you have it in unity8_calendar.go? While older interfaces aren't always consistent with this, it is current standard.

done.

Can you remove the space before ' gives'?

Please remove the 'send's to unconfined from the contacts' rules too.

done

Please use const.

done

Please remove this blank line.

done

Please use const.

done

Please remove the space before ' gives' here too.

done

Please use const.

done

Please use const.

done

I didn't notice this one before. Why do connecting client need to RequestName and ReleaseName? I expected this in CommonPermanentSlotAppArmor, not for plugging clients.

done

Please use const

done

getsockname is in the default template. Please remove.

done

Please use const.

done

Same here.

done

jdstrand · Dec 19, 2016

jdstrand reviewed Dec 19, 2016 • edited

View changes

Thanks for making the additional changes! There are a couple of important open questions and tweaking when plugging on classic.

The base declaration discussions have concluded and so now I can advise that since these are both slot implementations, you also need:

deny-connection: true

for both of these.

done

Since other changes are being made, can you put this comment on one line?

done

I asked about this one before. This is too broad. Why is this needed?

Sorry I removed from calendar but I did not see that on contacts.
Done

This is too broad, especially on OnClassic when you set ##SLOT_SECURITY_TAGS### to unconfined. Why is this needed?

Tested and this is not necessary. (removed)

This rule combined with this from unity8PimCommonConnectedPlugAppArmor:

dbus (send)
	bus=session
	path=/org/freedesktop/*
	interface=org.freedesktop.DBus.Properties
	peer=(label=###SLOT_SECURITY_TAGS###),

means that a connected snap may Get and Set any property of any unconfined service on the session bus, which is far too lenient. Also note the above comments on how this affects other rules.

done

s/uses session be/session services are/

done

LegacyAutoConnect can be dropped.

done

jdstrand · Dec 19, 2016

jdstrand approved these changes Dec 19, 2016

View changes

jdstrand · Dec 19, 2016

Thank you for making these changes. +1 on the security policy and base declaration.

niemeyer · Jan 16, 2017

niemeyer approved these changes Jan 16, 2017

View changes

zyga · Jan 17, 2017

zyga approved these changes Jan 17, 2017

View changes

Looks good now, all of my comments have been addressed or explained. Thank you.

jdstrand · Jan 17, 2017

FYI, the travis failures are unrelated:

2017/01/16 11:04:58 Failed tasks: 2
- linode:ubuntu-16.04-32:tests/main/snapd-reexec
- linode:ubuntu-16.04-64:tests/main/snapd-reexec

zyga · Jan 17, 2017

Master is somewhat broken currently. The issue should be resolved soon though. Let's try to merge master tomorrow and see how this feels.

niemeyer changed the title from Eds to interfaces/builtin: add evolution interfaces Oct 28, 2016

niemeyer added the Blocked label Oct 28, 2016

jdstrand removed the Blocked label Jan 9, 2017

@@ -82,5 +82,6 @@ func (s *AllSuite) TestInterfaces(c *C) {
          	c.Check(all, DeepContains, builtin.NewTpmInterface())
          	c.Check(all, DeepContains, builtin.NewUPowerObserveInterface())
          	c.Check(all, DeepContains, builtin.NewUnity7Interface())
+        +	c.Check(all, DeepContains, builtin.NewUnity8CalendarInterface())

@@ -473,7 +475,7 @@ func (s *baseDeclSuite) TestConnection(c *C) {
          		"location-observe": true,
          		"lxd":              true,
          		"mir":              true,
-        -		"udisks2":          true,
+        +		"udisks2":          true,

+        +		name: "unity8-calendar",
+        +		permanentSlotAppArmor: unity8CalendarPermanentSlotAppArmor,
+        +		connectedPlugAppArmor: unity8CalendarConnectedPlugAppArmor,
+        +		permanentSlotDBus:     unity8CalendarPermanentSlotDBus,

+        +		name: "unity8-contacts",
+        +		permanentSlotAppArmor: unity8ContactsPermanentSlotAppArmor,
+        +		connectedPlugAppArmor: unity8ContactsConnectedPlugAppArmor,
+        +		permanentSlotDBus:     unity8ContactsPermanentSlotDBus,

@@ -63,6 +63,10 @@
          			"revisionTime": "2015-07-22T06:53:40Z"
          		},
          		{
+        +			"path": "github.com/path/of/dependency",

snapcore/snapd

Join GitHub today

interfaces/builtin: add evolution interfaces #2226

Conversation

renatofilho commented Oct 27, 2016

niemeyer requested changes Oct 28, 2016 View changes

niemeyer changed the title from Eds to interfaces/builtin: add evolution interfaces Oct 28, 2016

niemeyer added the Blocked label Oct 28, 2016

renatofilho commented Oct 28, 2016

jdstrand commented Nov 9, 2016

niemeyer commented Nov 22, 2016

renatofilho commented Nov 22, 2016 • edited Edited 1 time renatofilho Nov 22, 2016

niemeyer commented Nov 22, 2016 • edited Edited 1 time niemeyer Nov 22, 2016

niemeyer commented Nov 22, 2016

jdstrand commented Nov 22, 2016

jdstrand commented Nov 22, 2016

niemeyer commented Nov 22, 2016

renatofilho commented Nov 22, 2016

renatofilho commented Nov 22, 2016

renatofilho commented Nov 24, 2016

niemeyer commented Nov 24, 2016

renatofilho commented Nov 25, 2016 • edited Edited 1 time renatofilho Nov 25, 2016

zyga requested changes Dec 1, 2016 View changes

renatofilho commented Dec 2, 2016

jdstrand requested changes Dec 5, 2016 View changes

renatofilho commented Dec 5, 2016 • edited Edited 1 time renatofilho Dec 5, 2016

jdstrand commented Dec 5, 2016

renatofilho commented Dec 6, 2016

chipaca commented Dec 14, 2016

renatofilho commented Dec 14, 2016 • edited Edited 1 time renatofilho Dec 14, 2016

jdstrand requested changes Dec 14, 2016 View changes

jdstrand requested changes Dec 15, 2016 View changes

Renato Araujo Oliveira Filho added some commits Jul 21, 2016

All checks have passed

jdstrand reviewed Dec 19, 2016 • edited View changes

Renato Araujo Oliveira Filho added some commits Dec 19, 2016

Some checks were not successful

All checks have passed

jdstrand approved these changes Dec 19, 2016 View changes

jdstrand commented Dec 19, 2016

jdstrand removed the Blocked label Jan 9, 2017

Some checks were not successful

niemeyer approved these changes Jan 16, 2017 View changes

niemeyer requested changes Oct 28, 2016

View changes

renatofilho commented Nov 22, 2016 •

Edited 1 time

renatofilho
Nov 22, 2016

niemeyer commented Nov 22, 2016 •

Edited 1 time

niemeyer
Nov 22, 2016

renatofilho commented Nov 25, 2016 •

Edited 1 time

renatofilho
Nov 25, 2016

zyga requested changes Dec 1, 2016

View changes

jdstrand requested changes Dec 5, 2016

View changes

renatofilho commented Dec 5, 2016 •

Edited 1 time

renatofilho
Dec 5, 2016

renatofilho commented Dec 14, 2016 •

Edited 1 time

renatofilho
Dec 14, 2016

jdstrand requested changes Dec 14, 2016

View changes

jdstrand requested changes Dec 15, 2016

View changes

jdstrand reviewed Dec 19, 2016 • edited

View changes

jdstrand approved these changes Dec 19, 2016

View changes

niemeyer approved these changes Jan 16, 2017

View changes