Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/modelcmd: support JUJU_USER_DOMAIN #6862
Conversation
rogpeppe
changed the base branch from
develop
to
user-domain
Jan 24, 2017
| +} | ||
| + | ||
| +// Cookies implements http.CookieJar.Cookies by | ||
| +// adding the |
| + Jar CookieJar | ||
| + // jar holds the internal version of the cookie jar - it has | ||
| + // methods that clients should not use, such as Save. | ||
| + jar *domainCookieJar |
jameinel
Jan 24, 2017
Owner
having a member "Jar" and another member "jar" sounds like a huge chance to make mistakes in using the right one.
Can we clarify these more with something like 'domainJar' at least?
I'm not sure why one is exported and one isn't, and comments like
"we send it to everyone because we don't know who needs it", feels very much like a security hole.
rogpeppe
Jan 24, 2017
Owner
They're actually the same object. The exported one is an interface type so that we can hide methods that we don't want external packages to invoke. Because they're the same object, it doesn't really matter which one is used, so ISTM that using a similar name is OK.
I could rename jar to internalJar if you'd prefer.
"we send it to everyone because we don't know who needs it", feels very much like a security hole.
Can you think of any actual security hole that might result from sending our preferred authentication domain to the parties that we make HTTP requests to?
I guess it might be possible to add a hook to the bakery package so that we could add the cookie only when discharging, but at the moment that seems like more complexity than is warranted.
rogpeppe
Jan 24, 2017
Owner
OK, I've removed the Jar field and replaced it with a CookieJar method that returns the internal jar implementation, so there's no possibility of them getting out of step.
| return &APIContext{ | ||
| Jar: jar, | ||
| - WebPageVisitor: webPageVisitor, | ||
| + jar: jar, |
jameinel
Jan 24, 2017
Owner
things like this where they are subtly the same jar, except when they won't be
jameinel
Jan 24, 2017
Owner
this is the sort of "they're never different until someone doesn't understand the code and sets one without setting the other one".
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
rogpeppe commentedJan 24, 2017
This can be used to select an alternative authentication
domain when authenticating using an external identity
manager.
We also clean up the tests a little by using IsolationSuite
instead of the deprecated FakeJujuXDGDataHomeSuite,
and unexport some fields from APIContext that aren't
used.