Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces: mpris updates (fix unconfined introspection, add name attribute) #1460
Conversation
jdstrand
added some commits
Jun 30, 2016
zyga
reviewed
Jun 30, 2016
| @@ -189,18 +187,23 @@ func (iface *MprisInterface) ConnectedPlugSnippet(plug *interfaces.Plug, slot *i | ||
| func (iface *MprisInterface) PermanentSlotSnippet(slot *interfaces.Slot, securitySystem interfaces.SecuritySystem) ([]byte, error) { | ||
| switch securitySystem { | ||
| case interfaces.SecurityAppArmor: | ||
| - snippet := mprisPermanentSlotAppArmor | ||
| + name, err := iface.GetName(slot.Attrs) |
zyga
Jun 30, 2016
Contributor
This should be a private method since nothing apart from this module needs to call it.
jdstrand
Jul 8, 2016
Contributor
I started with private by I wanted to test GetName() specifically in mpris_test.go and wasn't sure how to do that without making it public. Is there another way? If not and you feel strongly about this, I suppose I could instead of testing GetName() specifically I could instead test getName() via PermanentSlotSnippet tests.
zyga
Jul 11, 2016
Contributor
Yes, you can use export_test.go to create public variants of private functions. Look for this file in various directories for ideas on how to use it.
zyga
reviewed
Jun 30, 2016
| +func (iface *MprisInterface) GetName(attribs map[string]interface{}) (string, error) { | ||
| + // default to snap name if 'name' attribute not set | ||
| + mprisName := "@{SNAP_NAME}" | ||
| + for attr := range attribs { |
zyga
Jun 30, 2016
Contributor
I think we should ignore unknown attributes, just access name directly and handle its absence. Let other attributes be as-is.
jdstrand
Jul 8, 2016
Contributor
I was on the fence with this and definitely see your POV. As a counter POV, since we are talking about security policy generation, I like to err on the side of caution and error out if we get something we don't expect. I think this also goes along with the idea that interfaces form contracts between snaps and silently ignoring part of the contract felt weird.
Thoughts?
zyga
Jul 11, 2016
Contributor
I had this discussion early on when interfaces were still called capabilities. We decided to ignore unknown attributes and allow any attributes to exist.
zyga
reviewed
Jun 30, 2016
| + if attr != "name" { | ||
| + return "", fmt.Errorf("unknown attribute '%s'", attr) | ||
| + } | ||
| + name, ok := attribs[attr].(string) |
zyga
Jun 30, 2016
Contributor
I'm not 100% sure about this but I'd split this into two-lookup:
name, ok := attribs["name"]
if !ok {
// name not provided, use defaults or something
}
nameStr, ok := name.(string)
if !ok {
// name is there but isn't a string
}
jdstrand
Jul 8, 2016
Contributor
Well, I set the default up above in mprisName and only modify it if 'name' is an attribute, so it seems equivalent? Maybe I'm missing the context for your comment. Can you explain your reservation to the current implementation?
zyga
Jul 11, 2016
Contributor
Hmm, I don't see the default being set above.
All I meant to say is that in name, ok := attribs[attr].(string) the ok will refer to the type assertion, not to the presence of the attribute. In the snippet I gave above you can differentiate between the missing attribute and the attribute being of the incorrect type.
|
@jdstrand can I help you landing this? e.g. help with exprt_test.go or something? |
|
@mvo5 - nah, I was on vacation when the latest feedback came in and will circle back once I get through the various other snapd reviews, etc. If I need something, I'll holler; thanks for the offer. :) |
mvo5
and others
added some commits
Aug 12, 2016
|
Both the integration tests and the CI tests have unrelated testsuite failures. |
|
Looks good now, thank you! |
jdstrand commentedJun 30, 2016
•
Edited 1 time
-
jdstrand
Jun 30, 2016
The mpris interface is functional for vlc but needs a bit of polishing. This PR does that.
The name attribute affects the DBus well-known name that the snap is allowed to bind to and the name attribute is optional. When not specified,
org.mpris.MediaPlayer2.@{SNAP_NAME}is used like before (ie, this PR does not require a change to existing snaps like vlc). When specified,org.mpris.MediaPlayer2.$name_attributeis used instead. This should help with older apps whose snap name isfoo(ie, that must have a lower-case snap name) but which bind onorg.mpris.MediaPlayer2.Foo.Among other things, existing vlc snap (with mpris modifications since that isn't in the store yet) is verified to continue to work. Controlling via indicator-sound also confirmed to continue to work (once you rename vlc_vlc.desktop as vlc.desktop in /var/lib/snapd/desktop/applications). In addition, unconfined processes (eg d-feet) verified as able to exercise vlc's mpris DBus API.