Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add macaroon support to login, upload and download. #532
Conversation
added some commits
May 29, 2016
|
Requires #531. |
elopio
reviewed
May 30, 2016
| + if response.ok: | ||
| + return response.json()['discharge_macaroon'], None | ||
| + else: | ||
| + return None, response.text |
elopio
May 30, 2016
•
Edited 1 time
-
elopio
May 30, 2016
Member
-
May 30, 2016
Instead of returning a tuple, this should raise an exception. For now I'm just copying vila's work, we can fix this in a follow-up branch.
elopio
Jun 2, 2016
Member
@facundobatista what about returning the errors in the response? Should that be an exception too?
Like:
if error:
result['errors'] = error
Some of these statements come from pindonga's first implementation.
facundobatista
Jun 2, 2016
Contributor
It's ok to return the error in the HTTP response.
The point is how you signal errors through the function layers for finally to store it in the response.
The sensible way to do this is using exceptions, not flagging every function call. I'm happy to provide more details about this if you want, just ping me on IRC.
elopio
Jun 2, 2016
Member
@facundobatista I want to make better use of all our exceptions, and just have a couple of clues. When we start working on that I will ask for your input and review. Thanks!
elopio
reviewed
May 30, 2016
| +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| +# THE SOFTWARE. | ||
| +"""Code copied from pymacaroons until the package is available for xenial. |
elopio
May 30, 2016
•
Edited 1 time
-
elopio
May 30, 2016
Member
-
May 30, 2016
should we be working on backporting this?
cjwatson
Jun 1, 2016
Contributor
I've already sorted out a backport; it's awaiting SRU team approval in xenial-proposed/NEW.
elopio
Jun 2, 2016
•
Edited 1 time
-
elopio
Jun 2, 2016
Member
-
Jun 2, 2016
awesome, thanks Colin. I will come back and delete this next week.
elopio
added some commits
May 31, 2016
|
@elopio can you rebase please? |
elopio
added some commits
May 31, 2016
sergiusens
reviewed
May 31, 2016
| - oauth_path = self._API_PATH + 'tokens/oauth' | ||
| - if parsed_path.path.startswith(oauth_path): | ||
| - self._handle_oauth() | ||
| + tokens_discharge_path = self._API_PATH + 'tokens/discharge' |
elopio
Jun 1, 2016
Member
sure, why not. I just hate urljoin a little because when I miss the trailing / it does things I don't expect. But if the tests pass, all's good.
sergiusens
reviewed
May 31, 2016
| - upload_path = self._DEV_API_PATH + 'click-package-upload/' | ||
| - if parsed_path.path.startswith(upload_path): | ||
| + acl_path = self._DEV_API_PATH + 'acl/' | ||
| + upload_path = self._DEV_API_PATH + 'snap-upload/' |
|
So now I am not so eager to add a message on config load to tell the use they need to relogin, this could be treated as a credential invalidation problem just fine. |
elopio
added some commits
Jun 1, 2016
|
Updated. |
cjwatson
reviewed
Jun 1, 2016
| + unbound = macaroons.Macaroon.deserialize(unbound_raw) | ||
| + bound = root_macaroon.prepare_for_request(unbound) | ||
| + auth = 'Macaroon root={}, discharge={}'.format(root_macaroon_raw, | ||
| + bound.serialize()) |
cjwatson
Jun 1, 2016
Contributor
This should check that it doesn't break framing. See the Launchpad version of this.
elopio
Jun 2, 2016
Member
I like a lot better the way that you inject the auth in the request. I hope this code gets out of snapcraft and goes into a shared tool, but if it remains here we can try to copy more things from you.
However there are a couple of details that I don't like about your implementation.
https://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/view/18086/lib/lp/snappy/model/snapstoreclient.py#L69
Here I think you shouldn't use assert. It can be disabled and you should use it only for internal consistency checks. The value seems to come from somewhere out of your control, so it should be guarded with if -> raise.
The previous check seems to be safe with the assert.
https://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/view/18086/lib/lp/snappy/tests/test_snapstoreclient.py#L98
Here you are testing only the root macaroon. The second argument, the discharge_macaroon will fail on deserialize, before it gets to the assert.
It made sense to me that any problem reading the macaroon is an InvalidCredentialsError, so I am throwing an InvalidCredentialsError if deserialize fails with that binascii padding error.
Please let me know what do you think.
cjwatson
Jun 2, 2016
Contributor
I don't believe we ever disable asserts in Launchpad production, but I take your point. I'll fix the issues you've raised; thanks!
A deserialize check is probably good enough in practice, although personally I think it's still worth a low-level paranoia check on specific allowed characters when embedding it in an Authorization header, but perhaps that's just me.
elopio
Jun 2, 2016
Member
no, I think you are right. I was just trying to do the smallest implementation to get this in landed on time. Trying to implement your suggestion made it obvious that we are not doing a good job handling exceptions. I'm reporting a bug for the exceptions thing, and for the allowed characters check, to be fixed in one or two weeks. Thanks again.
cjwatson
reviewed
Jun 1, 2016
| - return session.get(download_url) | ||
| + auth = _macaroon_auth(self.conf) | ||
| + response = self.post( | ||
| + 'snap-upload/', data=json.dumps(data), |
cjwatson
Jun 1, 2016
Contributor
The endpoint is now called snap-push (bug report); there seems little point in adding new code that refers to the old endpoint.
facundobatista
reviewed
Jun 1, 2016
| + response = self.post( | ||
| + 'acl/', | ||
| + data=json.dumps({'permissions': acls}), | ||
| + headers={'Content-Type': 'application/json'}) |
facundobatista
Jun 1, 2016
Contributor
Better if you also send Accept: application/json as it may be used in the future (same in other requests)
elopio
added some commits
Jun 2, 2016
|
Thanks for your reviews. The branch is updated. |
|
retest this please |
sergiusens
merged commit f1a678a
into
snapcore:master
Jun 2, 2016
elopio
deleted the
elopio:macaroons5
branch
Jun 2, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
elopio commentedMay 30, 2016
LP: #1586910