Merge lp://staging/~mardy/ubuntuone-credentials/signon-plugin-part1 into lp://staging/ubuntuone-credentials

FAILED: Continuous integration, rev:237
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/144/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/21/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/21/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/21/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/144/rebuild

Replied to inline comments.

On Wed, 2016-04-20 at 08:14 +0000, Alberto Mardegan wrote:
> Because Token::isValid() is currently broken; either we add a check
> there to see that every variable is non empty, or we make sure we
> don't add empty variables into the map when we construct the token.

Please file a bug about this issue, and revert this change from this
branch. You don't fix all cases here, and I think this is probably the
wrong fix anyway. It's an issue separate from the signon-plugin, so
let's treat it as such and deal with it more appropriately.

> > +            if (pair.count() < 2) continue;
> Yes, it helps the migration from using the current password plugin to
> the new one: the new one won't store the encoded token in the
> password field, which will be used for the password instead.
> However, to ease the migration, the new plugin first checks is the
> password fields contains an encoded token: to do that, it tries to
> load the password into a Token with Token::fromQuery, and then checks
> whether the token is valid. If the password field does contain a
> password and not a token, this would break. (see lines 974-1000 of
> this diff)

So the plug-in can read the data from a different plug-in type to
convert it? If the line is required, please make it a 3 line addition
with open/close curly braces.

> Answering in random order:
> No, this won't affect the current account plugin: with the code I'm
> going to propose, the account plugin disables all signon UI
> interactions, which means that this code path will not be taken and
> that the account plugin will receive error codes instead, so that it
> will be able to adapt its UI and query the user itself (exactly like
> it's currently doing). So, no additional dialogs there.
>
> This code is mainly for the case of the Click scope, as well as PayUI
> (unless you can handle UI requests there, in which case we could do a
> similar thing as we are doing for the account plugin, of disabling
> signon UI in there too) and any third party app using the UbuntuOne
> account (a launchpad client, for example).

Can you clarify how this would work exactly for the click scope? A flow
chart of screen grabs or such perhaps? I think design wants us to
always show branded UI for cases where we require the user to log in
again.

> What happens in that case is that the client will receive the U1
> token data, if available, and no UI interactions will be required.
> If, however, the token data is not available (because the existing
> token has been invalidated via the website, or because the client is
> using a different TokenName and therefore a new token needs to be
> created instead), there might be the need to show the login dialog to
> the user. In that case, this authentication plugin asks signond to
> popup a UI to request the needed info.

How does online-accounts know the token has been invalidated via the
web site or such? We would need to change the clients to call some
other methods than they are currently calling, for these cases?

> The parameters you see being passed here into the "data" dictionary
> describe what are the needed fields, and it's possible to override
> the dialog title and each field's descr...

On 20/04/2016 23:15, Rodney Dawes wrote:
> Please file a bug about this issue, and revert this change from this
> branch. You don't fix all cases here, and I think this is probably the
> wrong fix anyway. It's an issue separate from the signon-plugin, so
> let's treat it as such and deal with it more appropriately.

https://bugs.launchpad.net/bugs/1572943

>>> + if (pair.count() < 2) continue;
[...]
> So the plug-in can read the data from a different plug-in type to
> convert it? If the line is required, please make it a 3 line addition
> with open/close curly braces.

OK.

[...]
> Can you clarify how this would work exactly for the click scope? A flow
> chart of screen grabs or such perhaps? I think design wants us to
> always show branded UI for cases where we require the user to log in
> again.

I can do that of course, but IMHO the best thing is whether you would
test the silo and see for yourself.
There is not much branded in the current authentication UI (apart from
the page title), but in principle this is something which can be
improved easily.

> How does online-accounts know the token has been invalidated via the
> web site or such? We would need to change the clients to call some
> other methods than they are currently calling, for these cases?

The authentication plugins checks the validity of a token before
returning it to the client: see the
SignOnPlugin::respondWithStoredData() method: near the end it calls
SignOnPlugin::checkTokenValidity() and if the token is invalid it won't
return it; instead, it will try to get a new one.

> Does this UI request open the Main.qml plug-in we are providing? Or it
> only opens generic UI provided by online-accounts itself?

The latter. It's a generic UI, configurable with a few parameters.

FAILED: Continuous integration, rev:238
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/146/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/23/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/23/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/23/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/146/rebuild

To clarify: I'm talking about silo 79 (https://requests.ci-train.ubuntu.com/#/ticket/1049). No matter what the train says, the packages in the silo PPA appear to be working as expected, and testing is quite easy: create an U1 plugin, then go to the website to revoke the token and finally try to install an app from the click scope.
You will get a dialog like this beauty: http://www.mardy.it/archivos/ubuntuone.png
If you enter the correct credentials, the package installation will start; otherwise, the dialog will reappear. And indeed, if you type the right credentials but you have 2fa enabled, the dialog will reappear and will have an additional field for the 2fa.

Before you say it: I'm aware of the uglyness of the dialog, and also that it would be a much better experience if the dialog didn't close at every attempt, but instead remain on screen with a spinner until the authentication has completed.
Both these things are solvable; the first with some UI designers input, the latter with some non trivial changes to ussoa, which I didn't start doing given that U1 was the only account requiring these changes, and I was not sure whether the U1 authentication plugin was ever going to land.
However, now that it appears that we are going to have an owncloud plugin, I might get started on them anyway.

FAILED: Continuous integration, rev:239
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/147/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/24/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/24/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/24/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/147/rebuild

OK, I don't know what you did, but trying to merge your library-symbols branch on top of this one, or vice versa, results in a bunch of conflicts.

Also, it looks like you forgot to bzr mv tst_plugin.cpp to test_plugin.cpp.

FAILED: Continuous integration, rev:240
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/148/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/25/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/25/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/25/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/148/rebuild

I've now manually rebased the branch on top of the library-symbols one.

I also couldn't do it manually, and I blame bazaar for that :-)

Basically, both the signon-plugin-part1 and the library-symbols branch were built on top of the huge original signon-plugin branch; I simply added a different partial revert commit to each of these branches, in order to keep only a subset of the changes. But now, when I tried to merge one on top of the other, bazaar decided to replay the whole history of the commits (at least, that's my explanation for what happaned) and of course it failed. If the merge were just content based (like I believe git does), this would have merged just fine.

FAILED: Continuous integration, rev:239
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/149/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/26/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/26/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/26/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/149/rebuild

Several comments in-line, and the tests are crashing for me, so I've only done a fairly basic review of those as there is a lot of code in test_plugin.cpp added here, and crashing tests are not useful anyway. Thanks.

FAILED: Continuous integration, rev:240
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/150/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/27/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/27/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/27/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/150/rebuild

I updated the branch addressing your comments. Notable exceptions/remarks:

1) I removed the i18n classes, but I don't see a reason to prefer including the glib i18n functions over libintl.h; also, I added a utility _() method at the top of the file, in order to keep the i18n functionality confined.

2) I reintroduced the checks on the Token() constructor; while indeed they could be done on the client side, I think the Token class is the best place to do so. In order to remove code duplication I also changed the second constructor to chain up on the first one.

On Tue, 2016-04-26 at 08:51 +0000, Alberto Mardegan wrote:
> I updated the branch addressing your comments. Notable
> exceptions/remarks:
>
> 1) I removed the i18n classes, but I don't see a reason to prefer
> including the glib i18n functions over libintl.h; also, I added a
> utility _() method at the top of the file, in order to keep the i18n
> functionality confined.

Because gi18n-lib.h defines macros for _ and similar, which pass the
correct GETTEXT_PACKAGE value in for the domain, as should be done for
libraries and plug-ins. Instead, your code is passing domain = 0
always, and adds more code that we need to personally maintain in this
tree, for no particularly good reason. Instead, we can depend on the
header from glib for this, in case anything changes in the future that
would result in this function being broken, and only glib will need to
be fixed. I don't understand the reluctance to depend on something
which is only a header file, and which is part of a low level library
which a very significant portion of the system already depends on, and
thus duplicating code and increasing our own maintenance burden.

> 2) I reintroduced the checks on the Token() constructor; while indeed
> they could be done on the client side, I think the Token class is the
> best place to do so. In order to remove code duplication I also
> changed the second constructor to chain up on the first one.

Please remove these changes. I'm not sure what you mean about being
done on the client side. What I asked for previously was for a bug to
be filed about this specific issue you mentioned, and for a separate
branch to fix only that issue, as it is unrelated to building the plug-
in itself, which is what this branch should be doing.

Couple of comments inline, and my reply to your previous comment.

Also, the tests are failing. Storing the dates as ISO strings as provided by the server should resolve the issue. Please build and run 'make check' and ensure the tests pass.

FAILED: Continuous integration, rev:243
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/152/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/29/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/29/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/29/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/152/rebuild