Merge into trunk : pasteboard : Code : content-hub

Reviewer	Review Type	Date Requested	Status
Tyler Hicks			Approve on 2016-08-25
system-apps-ci-bot	continuous-integration		Needs Fixing on 2016-08-25
Daniel d'Andrada (community)		2016-06-02	Approve on 2016-08-03
PS Jenkins bot	continuous-integration		Pending
Review via email: mp+296352@code.staging.launchpad.net

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-06-14:

#

FAILED: Continuous integration, rev:286
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/2/
Executed test runs:
FAILURE: https://jenkins.canonical.com/system-apps/job/build/563/console
FAILURE: https://jenkins.canonical.com/system-apps/job/build-0-fetch/563/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/2/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-08:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/20/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-08:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/22/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-18:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/24/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-18:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/25/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-07-18:

#

In include/com/ubuntu/content/hub.h:

"""
Q_INVOKABLE virtual Paste* create_paste(const QMimeData& data);
"""

Who owns the created Paste object? Is the caller free to delete it? Will that Paste object live forever, as long as Hub does? Please document it (I know other methods are undocumented and that sucks).

--------------------------------------------------

In include/com/ubuntu/content/paste.h:

Since Paste is public API, I would like to see it documented. Again, I know existing the API isn't, but we can do better than that.

"""
#include <com/ubuntu/content/item.h>
"""

What do you use from this header?

"""
namespace com
{
namespace ubuntu
{
namespace content
{
namespace detail
{
}
}
}
}
"""

You can remove this as you don't forward-declare anything in there.

"""
Q_PROPERTY(int id READ id)
[...]
Q_PROPERTY(QString source READ source)
"""

If those properties never change you should declare them as CONSTANT, like this:
Q_PROPERTY(QString source READ source CONSTANT)

"""
    enum State
    {
        created,
        charged,
        saved,
        collected
    };
"""

What do those states mean and why should user the care about them all?

"""
    Q_INVOKABLE virtual int id() const;
    Q_INVOKABLE virtual State state() const;
    Q_INVOKABLE virtual bool charge(const QMimeData& mimeData);
    Q_INVOKABLE virtual QMimeData* mimeData();
    Q_INVOKABLE virtual QString source() const;
"""

They don't have to (nor should) be Q_INVOKABLE if they're already getters or setters of Q_PROPERTIES. You added the Q_INVOKABLE so they could be called from QML, right?

"""
//Q_PROPERTY(QMimeData mimeData READ mimeData WRITE charge)
"""

Why commented out?

"""
Q_INVOKABLE virtual bool charge(const QMimeData& mimeData);
"""

Is the user supposed to call this? What's the use case? He can call Hub::create_paste(mimeData) and later change the paste contents passing a different QMimeData?

What's the use of Paste::source property?

review: Needs Information

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-07-19:

#

"""
Q_INVOKABLE virtual const QMimeData* latest_paste_buf();
"""

That usually involves a round-trip through the D-Bus session + deserialization of the QMimeData, right?

So if I (the client) wanna get it asynchronously to avoid blocking my current thread (essentially because of the IPC involved), should I call this method from a separate worker thread? Or would it be simple for you to expose an async version of this on the client API?

review: Needs Information

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-07-19:

#

Hub is missing a signal to inform clients that the clibboard content has changed.

review: Needs Fixing

Revision history for this message

Ken VanDine (ken-vandine) wrote on 2016-07-20:

#

Download full text (3.2 KiB)

> In include/com/ubuntu/content/hub.h:
>
> """
> Q_INVOKABLE virtual Paste* create_paste(const QMimeData& data);
> """
>
> Who owns the created Paste object? Is the caller free to delete it? Will that
> Paste object live forever, as long as Hub does? Please document it (I know
> other methods are undocumented and that sucks).

Caller is free to delete it. The object will be available as long as the hub is running. Eventually we'll cache them to disk as well, so the stack will be restored.

>
> --------------------------------------------------
>
> In include/com/ubuntu/content/paste.h:
>
> Since Paste is public API, I would like to see it documented. Again, I know
> existing the API isn't, but we can do better than that.

Paste is technically a public API, but applications won't have direct access to the Paste object under confinement. Which reminds me why create_paste shouldn't return the Paste object. That should really just return a boolean. The Paste object will be used for the clipboard app.

>
> """
> #include <com/ubuntu/content/item.h>
> """
>
> What do you use from this header?

That can be removed

>
> """
> namespace com
> {
> namespace ubuntu
> {
> namespace content
> {
> namespace detail
> {
> }
> }
> }
> }
> """
>
> You can remove this as you don't forward-declare anything in there.
>
> """
> Q_PROPERTY(int id READ id)
> [...]
> Q_PROPERTY(QString source READ source)
> """
>
> If those properties never change you should declare them as CONSTANT, like
> this:
> Q_PROPERTY(QString source READ source CONSTANT)

Agreed, that would be better

>
> """
> enum State
> {
> created,
> charged,
> saved,
> collected
> };
> """
>
> What do those states mean and why should user the care about them all?

The user probably doesn't care, and we might just blow away the state property.

>
>
> """
> Q_INVOKABLE virtual int id() const;
> Q_INVOKABLE virtual State state() const;
> Q_INVOKABLE virtual bool charge(const QMimeData& mimeData);
> Q_INVOKABLE virtual QMimeData* mimeData();
> Q_INVOKABLE virtual QString source() const;
> """
>
> They don't have to (nor should) be Q_INVOKABLE if they're already getters or
> setters of Q_PROPERTIES. You added the Q_INVOKABLE so they could be called
> from QML, right?

Yes, although we probably only need the charge method to be callable from QML.

>
>
> """
> //Q_PROPERTY(QMimeData mimeData READ mimeData WRITE charge)
> """
>
> Why commented out?

I don't see this commented out, maybe you reviewed this before my last push.

>
> """
> Q_INVOKABLE virtual bool charge(const QMimeData& mimeData);
> """
>
> Is the user supposed to call this? What's the use case? He can call
> Hub::create_paste(mimeData) and later change the paste contents passing a
> different QMimeData?

Not directly, this is for use by the hub. Once the Paste object is charged, it can no longer be charged again. So the data won't change. For qtubuntu, you should only use the API exposed by hub.h.

>
> What's the use of Paste::source property?

This is the appId of the application that created the paste. We'll use this in the...

> In include/com/ubuntu/content/hub.h:
> 
> """
>     Q_INVOKABLE virtual Paste* create_paste(const QMimeData& data);
> """
> 
> Who owns the created Paste object? Is the caller free to delete it? Will that
> Paste object live forever, as long as Hub does? Please document it (I know
> other methods are undocumented and that sucks).

Caller is free to delete it.  The object will be available as long as the hub is running.  Eventually we'll cache them to disk as well, so the stack will be restored.

> 
> --------------------------------------------------
> 
> In include/com/ubuntu/content/paste.h:
> 
> Since Paste is public API, I would like to see it documented. Again, I know
> existing the API isn't, but we can do better than that.

Paste is technically a public API, but applications won't have direct access to the Paste object under confinement.  Which reminds me why create_paste shouldn't return the Paste object.  That should really just return a boolean.  The Paste object will be used for the clipboard app.

> 
> """
> #include <com/ubuntu/content/item.h>
> """
> 
> What do you use from this header?

That can be removed

> 
> """
> namespace com
> {
> namespace ubuntu
> {
> namespace content
> {
> namespace detail
> {
> }
> }
> }
> }
> """
> 
> You can remove this as you don't forward-declare anything in there.
> 
> """
>     Q_PROPERTY(int id READ id)
> [...]
>     Q_PROPERTY(QString source READ source)
> """
> 
> If those properties never change you should declare them as CONSTANT, like
> this:
> Q_PROPERTY(QString source READ source CONSTANT)

Agreed, that would be better

> 
> """
>     enum State
>     {
>         created,
>         charged,
>         saved,
>         collected
>     };
> """
> 
> What do those states mean and why should user the care about them all?

The user probably doesn't care, and we might just blow away the state property.

> 
> 
> """
>     Q_INVOKABLE virtual int id() const;
>     Q_INVOKABLE virtual State state() const;
>     Q_INVOKABLE virtual bool charge(const QMimeData&  mimeData);
>     Q_INVOKABLE virtual QMimeData* mimeData();
>     Q_INVOKABLE virtual QString source() const;
> """
> 
> They don't have to (nor should) be Q_INVOKABLE if they're already getters or
> setters of Q_PROPERTIES. You added the Q_INVOKABLE so they could be called
> from QML, right?

Yes, although we probably only need the charge method to be callable from QML.

> 
> 
> """
>     //Q_PROPERTY(QMimeData mimeData READ mimeData WRITE charge)
> """
> 
> Why commented out?

I don't see this commented out, maybe you reviewed this before my last push.

> 
> """
>     Q_INVOKABLE virtual bool charge(const QMimeData&  mimeData);
> """
> 
> Is the user supposed to call this? What's the use case? He can call
> Hub::create_paste(mimeData) and later change the paste contents passing a
> different QMimeData?

Not directly, this is for use by the hub.  Once the Paste object is charged, it can no longer be charged again.  So the data won't change.  For qtubuntu, you should only use the API exposed by hub.h.

> 
> What's the use of Paste::source property?

This is the appId of the application that created the paste.  We'll use this in the clipboard app to show the application icon next to the item in the stack.

Revision history for this message

Ken VanDine (ken-vandine) wrote on 2016-07-20:

#

> Hub is missing a signal to inform clients that the clibboard content has
> changed.

I'm adding property that returns a list of mimetypes on the stack which includes a changed signal.

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-21:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/26/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-21:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/27/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-21:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/28/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-21:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/29/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-21:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/30/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-25:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/31/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-07-26:

#

Made some modifications here:
http://bazaar.launchpad.net/~dandrader/content-hub/pasteboard/revision/285

- Fix unnecessary round trip and memory leak when fetching a Paste from the service
- Simplify D-Bus interface and traffic using ay instead of av
- Create separate sync and async versions of createPaste

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-07-26:

#

Code making use of it is here:
lp:~dandrader/qtubuntu/content-hub-clipboard

In src/ubuntumirclient/clipboard.cpp

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-07-26:

#

> Made some modifications here:
> http://bazaar.launchpad.net/~dandrader/content-hub/pasteboard/revision/285
>
> - Fix unnecessary round trip and memory leak when fetching a Paste from the
> service
> - Simplify D-Bus interface and traffic using ay instead of av
> - Create separate sync and async versions of createPaste

Oh, and added async versions for latestPaste() and pasteById() as well.

Revision history for this message

Ken VanDine (ken-vandine) wrote on 2016-07-26:

#

> > Made some modifications here:
> > http://bazaar.launchpad.net/~dandrader/content-hub/pasteboard/revision/285
> >
> > - Fix unnecessary round trip and memory leak when fetching a Paste from the
> > service
> > - Simplify D-Bus interface and traffic using ay instead of av
> > - Create separate sync and async versions of createPaste
>
> Oh, and added async versions for latestPaste() and pasteById() as well.

These changes are great! Only comment I have is I prefer making createPaste async and making the sync version something like createPasteSync. Makes it feel like the preferred function is the async one. But that's not a must for me, it's just more like I'm used to doing with gobject style code. What do you think is more common for Qt?

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-07-26:

#

On 26/07/2016 18:05, Ken VanDine wrote:
>>> Made some modifications here:
>>> http://bazaar.launchpad.net/~dandrader/content-hub/pasteboard/revision/285
>>>
>>> - Fix unnecessary round trip and memory leak when fetching a Paste from the
>>> service
>>> - Simplify D-Bus interface and traffic using ay instead of av
>>> - Create separate sync and async versions of createPaste
>> Oh, and added async versions for latestPaste() and pasteById() as well.
> These changes are great! Only comment I have is I prefer making createPaste async and making the sync version something like createPasteSync. Makes it feel like the preferred function is the async one. But that's not a must for me, it's just more like I'm used to doing with gobject style code. What do you think is more common for Qt?

I followed the convention used here:
http://doc.qt.io/qt-5/qdbusabstractinterface.html#public-functions

But I also prefer prefer the way you suggest. Made the change (same
revision number).

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-27:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/32/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-27:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/33/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-29:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/36/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-29:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/37/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-29:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/38/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-07-29:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/39/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-08-01:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/40/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-08-01:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/41/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-08-01:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/42/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-08-01:

#

Looks good to me.

review: Approve

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-08-01:

#

CMakeLists.txt:79:set(CONTENT_HUB_VERSION_MAJOR 0)
CMakeLists.txt:80:set(CONTENT_HUB_VERSION_MINOR 0)
CMakeLists.txt:81:set(CONTENT_HUB_VERSION_PATCH 1)

Please update those to match the version in debian/changelog.

Otherwise things like that will fail:
pkg_check_modules(CONTENT_HUB libcontent-hub>=0.2 REQUIRED)

review: Needs Fixing

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-08-01:

#

CMakeLists.txt:79:set(CONTENT_HUB_VERSION_MAJOR 0)
CMakeLists.txt:80:set(CONTENT_HUB_VERSION_MINOR 0)
CMakeLists.txt:81:set(CONTENT_HUB_VERSION_PATCH 1)

Please update those to match the version in debian/changelog.

Otherwise things like that will fail:
pkg_check_modules(CONTENT_HUB libcontent-hub>=0.2 REQUIRED)

review: Needs Fixing

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-08-01:

#

Would be good also to move over the test for the serialization and deserialization code from lp:qtmir in tests/mirserver/Clipboard/clipboard_test.cpp.

Namely TEST(ClipboardTest, MimeDataSerialization)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-08-01:

#

Made some changes in lp:~dandrader/content-hub/pasteboard revision 285. Let me know what you think.

It was really annoying me while testing on a laptop that content-hub was refusing to accept a copy from an app that unity8 was displaying (because ubuntu-app-launch wasn't tracking the running app for some reason). That cannot happen and would be a regression since it works with the current clipboard implementation.

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-08-03:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/43/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Ken VanDine (ken-vandine) wrote on 2016-08-03:

#

> CMakeLists.txt:79:set(CONTENT_HUB_VERSION_MAJOR 0)
> CMakeLists.txt:80:set(CONTENT_HUB_VERSION_MINOR 0)
> CMakeLists.txt:81:set(CONTENT_HUB_VERSION_PATCH 1)
>
> Please update those to match the version in debian/changelog.
>
> Otherwise things like that will fail:
> pkg_check_modules(CONTENT_HUB libcontent-hub>=0.2 REQUIRED)

Done

Revision history for this message

Ken VanDine (ken-vandine) wrote on 2016-08-03:

#

> Would be good also to move over the test for the serialization and
> deserialization code from lp:qtmir in
> tests/mirserver/Clipboard/clipboard_test.cpp.
>
> Namely TEST(ClipboardTest, MimeDataSerialization)

Done

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-08-03:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/44/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Ken VanDine (ken-vandine) wrote on 2016-08-03:

#

> Made some changes in lp:~dandrader/content-hub/pasteboard revision 285. Let me
> know what you think.
>
> It was really annoying me while testing on a laptop that content-hub was
> refusing to accept a copy from an app that unity8 was displaying (because
> ubuntu-app-launch wasn't tracking the running app for some reason). That
> cannot happen and would be a regression since it works with the current
> clipboard implementation.

This is fine, ultimately I hope it'll be a real corner case. But it's really not that big of a deal if it fails.

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-08-03:

#

Thanks!

review: Approve

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-08-03:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/45/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2016-08-03:

#

Ubuntu Security would like to review this change. We were involved in the early design stages and were awaiting an update to the design doc for this feature. The design doc that we have access to is still based on an old design that doesn't match this implementation. We're blocked until the doc can be updated.

review: Needs Information

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-08-23:

#

Wrote focused surface verification based on persistent surface ids here:
http://bazaar.launchpad.net/~dandrader/content-hub/pasteboard/revision/315

lp://staging/~ken-vandine/content-hub/pasteboard updated on 2016-08-24

315. By Ken VanDine on 2016-08-24: merged trunk
316. By Ken VanDine on 2016-08-24: Daniel d'Andrada 2016-08-23 Authenticate using surface ids instead of process ids

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-08-24:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/46/rebuild

FAILED: Continuous integration, rev:316
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/46/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/system-apps/job/build/1286/console
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-0-fetch/1286
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=vivid+overlay/1152
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=xenial+overlay/1152
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=yakkety/1152
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1135
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1135/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1135
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1135/artifact/output/*zip*/output.zip
    FAILURE: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=yakkety/1135/console
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1135
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1135/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1135
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1135/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=yakkety/1135
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=yakkety/1135/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/1135
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/1135/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial+overlay/1135
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial+overlay/1135/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=yakkety/1135
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=yakkety/1135/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/46/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2016-08-24:

#

The client is blocking on startup waiting for PasteFormats value.

Wrote a fix for it here:
http://bazaar.launchpad.net/~dandrader/content-hub/pasteboard/revision/317

There I've also reduced D-Bus round trips by already sending the paste formats on its changed signal.

lp://staging/~ken-vandine/content-hub/pasteboard updated on 2016-08-25

317. By Ken VanDine on 2016-08-25: Daniel d'Andrada 2016-08-24 Improve PasteFormatsChanged

Revision history for this message

system-apps-ci-bot (system-apps-ci-bot) wrote on 2016-08-25:

#

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/48/rebuild

FAILED: Continuous integration, rev:317
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/48/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/system-apps/job/build/1314/console
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-0-fetch/1314
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=vivid+overlay/1178
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=xenial+overlay/1178
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-1-sourcepkg/release=yakkety/1178
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1158
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=vivid+overlay/1158/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1158
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1158/artifact/output/*zip*/output.zip
    FAILURE: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=amd64,release=yakkety/1158/console
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1158
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=vivid+overlay/1158/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1158
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1158/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=yakkety/1158
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=armhf,release=yakkety/1158/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/1158
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=vivid+overlay/1158/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial+overlay/1158
        deb: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=xenial+overlay/1158/artifact/output/*zip*/output.zip
    FAILURE: https://jenkins.canonical.com/system-apps/job/build-2-binpkg/arch=i386,release=yakkety/1158/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-apps/job/lp-content-hub-ci/48/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2016-08-25:

#

I gave this a quick review. It was more of a design review than a detailed code review.

The security of this design is rooted in the random surface ID created by Mir. That surface ID must be randomly generated and of significant size so that bruteforces of the ID is not feasible. I'm told that the ID is randomly generated and evenly distributed throughout a full 128 bits. That should be sufficient.

A future improvement that I'd like to see is a penalty after a number of incorrect surface ID guesses so that a malicious app cannot simply brute force the 128 bit space without any negative side effects. The penalty could be time based, where the pasteboard doesn't return back for several seconds. There are other options, as well.

I had some trouble understanding the intent behind the usage of the APP_ID in the merge proposal. Ken tells me that it is for a future design when there is a pasteboard UI so that the user will be prompted with the APP_ID of the application requesting access to the paste buffer. Since the APP_ID isn't used right now, I'm not concerned, but I'd like to understand more about how the APP_ID is being retrieved before we start prompting users. I'd prefer that we use the APP_ID directly from the D-Bus daemon via the GetConnectionCredentials bus method:

https://dbus.freedesktop.org/doc/dbus-specification.html#bus-messages-get-connection-credentials

Thanks for working towards a secure copy and paste solution. I look forward to this design being improved on in the future with the introduction of a pasteboard UI.

review: Approve

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2016-08-25:

#

Upon thinking about my suggestion, for a future enhancement, of a penalty to be handed out upon a number of wrong surface ID guesses, I realized that content-hub is not the correct place to implement the penalty. Mir clients will be able to call isSurfaceFocused() themselves to brute force the surface ID. Once they find a valid surface ID, then they can simply pass that to the content-hub. Therefore, the penalty mechanism would need to be implemented in Mir itself.

content-hub

Merge lp://staging/~ken-vandine/content-hub/pasteboard into lp://staging/content-hub

Commit message

Description of the change

Preview Diff

Subscribers