store, daemon, client, cmd/snap: handle PASSWORD_POLICY_ERROR #3278

Merged
merged 3 commits into from May 8, 2017

Conversation

Projects
None yet
3 participants
Member

chipaca commented May 5, 2017

We had the wrong type for SSO errors. This was usually fine, because the most common errors either have no extra field, or the extra field fits in a map[string][]string. But sometimes, and in particular for PASSWORD_POLICY_ERROR, it's a map[string]string. This made the user-visible error you'd get as a result of PASSWORD_POLICY_ERROR to be unintelligible and unhelpful (a json decode error).

Additionally, PASSWORD_POLICY_ERROR is rather different from other errors that have an extra field; in these, there is a long, detailed explanation of the cause of the error together with a URL the user needs to go to to address the issue. With this branch this is surfaced appropriately; for example (and the case that triggered this branch),

$ sudo snap login jlenton+test@gmail.com
Password of "jlenton+test@gmail.com": 
error: Recently, there was a security breach on another website unrelated to
       Ubuntu One. Ubuntu One was not affected and is not connected in any way
       with this incident, but the password you're trying to use would put your
       Ubuntu One account at risk because you're using the same email and
       password that was breached elsewhere.

       To make sure your account is kept safe, you will need to use a different
       password for this account.

       To address this, go to: https://login.ubuntu.com/
store, daemon, client, cmd/snap: handle PASSWORD_POLICY_ERROR code er…
…rors from sso, which are slightly different
Contributor

zyga commented May 5, 2017

There seems to be a folding error in the tests:

+ MATCH 'repeat the command including --devmode'
+ snap install --channel beta test-snapd-devmode
+ true
error: pattern not found, got:
error: The publisher of snap "test-snapd-devmode" has indicated that they do
       not consider this revision to be of production quality and that it is
       only meant for development or testing at this point. As a consequence
       this snap will not refresh automatically and may perform arbitrary
       system changes outside of the security sandbox snaps are generally
       confined to, which may put your system at risk.
       If you understand and want to proceed repeat the command including
       --devmode; if instead you want to install the snap forcing it into
       strict confinement repeat the command including --jailmode.
-----

Maybe travis sets some default terminal width and your local testing didn't capture this?

Member

chipaca commented May 5, 2017

sigh... there was a bug in the folding code where I'd misunderstood the width parameter: it doesn't take into account the indent. Fixed that, and of course it broke some of those tests...

I'd run them locally but it probably picked up my term width :-)
I'll fix the tests (expect can match regexps, yay).

zyga approved these changes May 8, 2017

+1

A few comments, and LGTM once they're tuned to your satisfaction.

cmd/snap/error.go
@@ -122,6 +130,9 @@ If you understand and want to proceed repeat the command including --classic.
// TRANSLATORS: %s is an error message (e.g. “cannot yadda yadda: permission denied”)
msg = fmt.Sprintf(i18n.G(`%s (try with sudo)`), err.Message)
}
+ case client.ErrorKindPasswordPolicy:
+ usesSnapName = false
+ msg = err.Message
@niemeyer

niemeyer May 8, 2017

Contributor

That looks the same as the default already.

@chipaca

chipaca May 8, 2017

Member

explicit is better than ... no? oh alright then :-)

store/auth.go
@@ -51,10 +51,32 @@ var (
UbuntuoneRefreshDischargeAPI = ubuntuoneAPIBase + "/tokens/refresh"
)
+// a stringish is something that can be a []string or a string
+// like the values of the "extra" documents in error responses
+type stringish []string
@niemeyer

niemeyer May 8, 2017

Contributor

Can we name this as stringList instead? stringish forces one to read the documentation at all times to have any idea of what it means (most things are "stringish" :).

store/errors.go
@@ -70,8 +70,27 @@ func (e *ErrDownload) Error() string {
return fmt.Sprintf("received an unexpected http response code (%v) when trying to download %s", e.Code, e.URL)
}
+type ErrPasswordPolicy map[string]stringish
@niemeyer

niemeyer May 8, 2017

Contributor

These types are not following the standard pattern correctly. They should all be FooError instead of ErrFoo. The latter is applied for values, while the former is applied to types. Okay to not fix here since this was wrong before already, but let's try to fix it at some point in the future.

@chipaca

chipaca May 8, 2017

Member

Yeah, all errors in there are ErrFoo. For a different branch.

cmd/snap/error.go: drop case that is the same as default
 store: s/stringish/stringList/g
Contributor

zyga commented May 8, 2017

All review feedback is applied, there are two reviews, the tests are green. Merging.

@zyga zyga merged commit e96cffd into snapcore:master May 8, 2017

7 checks passed

artful-amd64 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@chipaca chipaca deleted the chipaca:sso-errors-extra-values-not-always-lists branch May 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment