Skip to content
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

Add and expose auth types: MultiAuth ApiToken* #137

Merged
merged 1 commit into from
Feb 17, 2019

Conversation

cognifloyd
Copy link
Contributor

@cognifloyd cognifloyd commented Jan 16, 2019

This adds more auth classes designed to be used when initializing a
Consumer subclass. There are two generic AuthToken types that may
be used directly or may be subclassed by API library authors to define
the relevant query parameter or header.
There is also a MultiAuth type which allows for passing in a list of auth
methods, so that they all take effect.

The purpose of the generic AuthToken types is to make it simple for
uplink-based libraries to provide a standardized way to authenticate
with their api.

The purpose of MultiAuth is to allow for multiple auth methods. This may
be required when ProxyAuth needs to be combined with another auth
method, or when an API requires multiple auth tokens.

@prkumar
Copy link
Owner

prkumar commented Jan 16, 2019

@cognifloyd - Looks like one unit test is failing the CI build:

=================================== FAILURES ===================================
______________________________ test_bearer_token _______________________________
request_builder = <MagicMock spec='RequestBuilder' id='140036067042416'>
    def test_bearer_token(request_builder):
        # Setup
        bearer_token = auth.BearerToken("bearer-token")
    
        # Verify
        bearer_token(request_builder)
>       auth_str = bearer_token._auth_str
E       AttributeError: 'BearerToken' object has no attribute '_auth_str'
tests/unit/test_auth.py:51: AttributeError

Thanks for contributing, by the way!

@cognifloyd
Copy link
Contributor Author

cognifloyd commented Jan 16, 2019

@prkumar Yeah. I haven't looked at the tests yet. I will do that hopefully this week.

Conceptually, are you okay with the direction of this PR? eg I'm exporting the auth classes in __all__ now.

Copy link
Owner

@prkumar prkumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks good. I think ApiTokenParam and ApiTokenHeader are great additions to the library, and we exposing the auth classes is a good move too. I just have a few suggestions, which I've added below.

uplink/auth.py Outdated Show resolved Hide resolved
uplink/auth.py Show resolved Hide resolved
uplink/auth.py Outdated Show resolved Hide resolved
@prkumar
Copy link
Owner

prkumar commented Jan 20, 2019

I haven't had experience with APIs that use a multi-auth strategy, so I'm interested to see an example: do you know of any public APIs that support this sort of authentication?

@cognifloyd
Copy link
Contributor Author

I don't think very many (if any) APIs require multiple forms of auth on the server side.

I'm designing an API that might require multiple tokens on the server side, not just for intermediary auth, but it's a very specific use-case peculiar to our internal infrastructure. I don't know of any generally available APIs that require multiple forms of auth on the server side.

However, many clients require multiple forms of auth to, for example, authenticate to an HTTP proxy and to the API server.

Copy link
Contributor Author

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of subclassing, we could create some kind of auth meta class, or add a decorator that turns a class into an auth class?
Or should API libraries have some kind of factory function that their consumers would use to pass in the token?

uplink/auth.py Outdated Show resolved Hide resolved
uplink/auth.py Show resolved Hide resolved
uplink/auth.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 11, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #137   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          41     41           
  Lines        2052   2083   +31     
  Branches      163    167    +4     
=====================================
+ Hits         2052   2083   +31
Impacted Files Coverage Δ
uplink/auth.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 0179c2a...27803a2. Read the comment docs.

@cognifloyd cognifloyd force-pushed the multi_auth branch 9 times, most recently from 80894f7 to b43fdcb Compare February 12, 2019 06:18
@cognifloyd
Copy link
Contributor Author

cognifloyd commented Feb 12, 2019

@prkumar done including tests (codecov @ 100%).

  • expanded the MultiAuth to make it act more like a list, allowing for building it in whatever way makes sense to the API library consumer.
  • expanded the generic ApiTokenParam and ApiTokenHeader classes to allow using them directly ApiTokenParam(param="api_token", token="****") as well as by subclassing with class attributes. (See my comments and commit messages above).
  • refactored BasicAuth and ProxyAuth to reuse the ApiTokenHeader.
  • added/adjusted tests for all of the above.

What do you think? I can revert the changes to BasicAuth and ProxyAuth if you'd like (changing that made it more consistent in internal implementation details, but is otherwise unnecessary).

I can also squash my commits if you'd like.

@prkumar
Copy link
Owner

prkumar commented Feb 14, 2019

@cognifloyd - This looks great! Thanks so much for adding coverage and for extending the ApiTokenParam and ApiTokenHeader classes to be used directly, too. I'll go ahead approve the changes and merge this in ASAP.

After merging, I'll also make a separate commit to add you to the AUTHORS.rst file. Please let me know what name and/or public handle (e.g., @cognifloyd) that you'd like me to append to the file .Typically, I'd use the name and handle of your GitHub account, but I can use a different account/name if you'd prefer. For example, using your GitHub, here's what I'd add to AUTHORS.rst:

Jacob Floyd (@cognifloyd)

Alternatively, you could make a separate commit to this PR with that change. Whatever works best for you! Thanks for contributing!

@cognifloyd
Copy link
Contributor Author

I added myself to AUTHORS.rst

This exposes all auth methods and adds these additional auth classes:
 - MultiAuth
 - ApiTokenParam
 - ApiTokenHeader

The purpose of MultiAuth is to allow for multiple auth methods. This may
be required when an API user needs to supply intermediate credentials--
as when using ProxyAuth to authenticate with an intermediate proxy--
or when an API requires multiple auth tokens. MultiAuth acts like a list
so that users can append, extend or iterate over the contained auth
methods.

The purpose of the the ApiToken* auth methods is to allow both direct
API users and API library authors to easily provide auth tokens in a
standardized manner.

When directly supplying the auth token, some users may prefer to supply
both the parameter/header name and the token at the same time. Thus, we
allow such users to directly use the generic ApiToken* auth methods.

For API library authors that want to supply the parameter/header name in
a reusable way (so users only have to supply the token), they can
use properties on an ApiToken* subclass to define the relvant query
parameter or header.

This also refactors BasicAuth and ProxyAuth as subclasses of
ApiTokenHeader to harmonize the implementation details of all the auth
methods. Any uplink users that directly accessed the private _auth_str
attribute will need to replace that with _header_value.
@cognifloyd
Copy link
Contributor Author

Squashed and rebased on master.

@prkumar prkumar merged commit d84cf4a into prkumar:master Feb 17, 2019
cognifloyd added a commit to cognifloyd/uplink that referenced this pull request Feb 18, 2019
cognifloyd added a commit to cognifloyd/uplink that referenced this pull request Feb 18, 2019
prkumar added a commit that referenced this pull request Feb 19, 2019
@prkumar prkumar added this to the v0.9.0 milestone Feb 21, 2019
@prkumar prkumar mentioned this pull request Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants