New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow authenticating as a GitHub App #62

Merged
merged 8 commits into from Jul 13, 2018

Conversation

2 participants
@Mariatta
Copy link
Collaborator

Mariatta commented Jun 19, 2018

  • Add jwt and oauth_token parameters to:
    • abc.GitHubAPI._make_request
    • abc.GitHubAPI.get_item
    • abc.GitHubAPI.get_iter
    • abc.GitHubAPI.post
    • abc.GitHubAPI.patch
    • abc.GitHubAPI.put
    • abc.GitHubAPI.delete
  • Add jwt argument to sansio.create_headers
  • Raise ValueError if both jwt and oauth_token are passed in above methods
  • Update the documentation
  • Update the changelog

Closes #23

Allow authenticating as a GitHub App
- Add jwt parameter to abc.GitHubAPI
- Add auth_type and token parameters to:
  * abc.GitHubAPI._make_request
  * abc.GitHubAPI.get_item
  * abc.GitHubAPI.get_iter
  * abc.GitHubAPI.post
  * abc.GitHubAPI.patch
  * abc.GitHubAPI.put
  * abc.GitHubAPI.delete
- Add jwt argument to sansio.create_headers
- Raise ValueError if both jwt and oauth_token are passed in create_headers
- Update the documentation
- Update the changelog

Closes #23
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jun 19, 2018

Codecov Report

Merging #62 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #62    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          14     14            
  Lines        1046   1255   +209     
  Branches       58     65     +7     
======================================
+ Hits         1046   1255   +209
Impacted Files Coverage Δ
gidgethub/test/test_abc.py 100% <100%> (ø) ⬆️
gidgethub/sansio.py 100% <100%> (ø) ⬆️
gidgethub/test/test_sansio.py 100% <100%> (ø) ⬆️
gidgethub/abc.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5c3c2f...64babea. Read the comment docs.

@Mariatta Mariatta changed the title Allow authenticating as a GitHub App [wip] Allow authenticating as a GitHub App Jun 20, 2018

@Mariatta
Copy link
Collaborator

Mariatta left a comment

Not ready for merging yet :)
I've left my own comments where I'm unsure.

@@ -16,9 +16,11 @@ class GitHubAPI(abc.ABC):
"""Provide an idiomatic API for making calls to GitHub's API."""

def __init__(self, requester: str, *, oauth_token: Opt[str] = None,
jwt: Opt[str] = None,

This comment has been minimized.

@Mariatta

Mariatta Jun 20, 2018

Collaborator

Rethinking this (out loud). The json web token expires after max 10 minutes, so maybe it doesn't make sense to be initialized here.
I'm kinda wanting to store the private key instead. 🤔

This comment has been minimized.

@Mariatta

Mariatta Jun 20, 2018

Collaborator

I decided to not add jwt argument here because I just don't think it makes sense.

accept: str = sansio.accept_format()) -> Any:
data, _ = await self._make_request("POST", url, url_vars, data, accept)
accept: str = sansio.accept_format(),
auth_type: Opt[str] = None,

This comment has been minimized.

@Mariatta

Mariatta Jun 20, 2018

Collaborator

As far as I know, only one API accepts the bearer jwt authorization type, and it is a POST method.
I've added the same auth_type and token arguments to the other methods for symmetry, but now I'm not sure about it. Maybe only post should have these arguments.

This comment has been minimized.

@Mariatta

Mariatta Jun 20, 2018

Collaborator

I had some more time to experiment with GitHub APIs. Basically everything in listed in this documentation can be accessed using json web token. It means getitem, getiter and post will benefit from the auth_type params.

@Mariatta Mariatta changed the title [wip] Allow authenticating as a GitHub App Allow authenticating as a GitHub App Jun 20, 2018

Mariatta added some commits Jun 20, 2018

@Mariatta Mariatta requested a review from brettcannon Jun 20, 2018

@Mariatta

This comment has been minimized.

Copy link
Collaborator

Mariatta commented Jun 20, 2018

This is ready for review now. Thanks!

@brettcannon

This comment has been minimized.

Copy link
Owner

brettcannon commented Jun 22, 2018

Seeing it written out I think I prefer the separate auth_token/jwt arguments like you have with create_headers. It eliminates the need for all the if/elif/else lines because you can just blindly pass auth_token and jwt blindly down the call stack and the name still makes it obvious what kind of authorization you're doing.

Or am I over looking something? I'm not worried about future flexibility if that's why you went with this initial design as I'm sure GitHub is going to push GraphQL more anyway so I think this might be the end of the line for major changes to their v3 API. 😄

@Mariatta

This comment has been minimized.

Copy link
Collaborator

Mariatta commented Jun 23, 2018

Alright no prob :) I've changed the signature, and made oauth_token and jwt as keyword arguments to the other methods. ValueError is raised when both are passed.

I'll look forward to gidgethub v4: Add GraphQL support 😉

Mariatta added some commits Jun 25, 2018

@brettcannon

This comment has been minimized.

Copy link
Owner

brettcannon commented Jul 9, 2018

Just got back from vacation; I will try to review this week.

As for GraphQL support, I have some ideas on a generalized GraphQL client library for Python if you're at all curious.

@brettcannon

This comment has been minimized.

Copy link
Owner

brettcannon commented Jul 13, 2018

I just added periods to the end of some docstrings. Otherwise LGTM! So feel free to merge when you're ready.

@Mariatta

This comment has been minimized.

Copy link
Collaborator

Mariatta commented Jul 13, 2018

😄 Thanks!!

@Mariatta Mariatta merged commit ad93cf8 into brettcannon:master Jul 13, 2018

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to b5c3c2f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Mariatta Mariatta deleted the Mariatta:issue-23 branch Jul 13, 2018

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