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

Partner credentials #41

Closed

Conversation

gavinhodge
Copy link
Contributor

There's a fair bit in this PR... Summary of changes:

  • Rework Public Credentials
    • Allow RSA signing of requests and client side certificates, but this can't be turned on via the PublicCredentials constructor.
    • Base url is now an instance variable (needed later for partner credentials)
    • Store the token expiry date and provide expired() to check if token is still valid
    • There are no breaking changes - PublicCredentials work as normal
  • Create Partner Credentials
    • Inherits PublicCredentials, constructor allows RSA cert and client side cert
    • Changes the base url to the partner one
    • Adds refresh() function to gain a new token
  • Manager
    • Construct with credentials object instead of oauth.
    • Endpoint URLs are built using base url from credentials
    • Pass client side certificate to requests

This is a cleaned up version of a fork that's been used in production for a few months.

Gavin Hodge and others added 4 commits August 11, 2014 23:04
Client Certificate was not included in initial oauth request
Url property had wrong base url for Partner Credentials
Manager objects wouldn't notice new oauth object after refresh() is
called
@aidanlister
Copy link
Collaborator

Wow, great patch, everything looks pretty good, and thanks for the test coverage. @freakboy3742 I'll defer to you :)

@aidanlister
Copy link
Collaborator

I made this a bit tougher for myself than it needed to be by applying a bunch of other commits first and then spending several hours resolving conflicts, but I have tested the functionality and it works as expected, the test suite passes, and my own integration still functions so I'm going to merge this in now.

aidanlister added a commit that referenced this pull request Aug 13, 2014
@gavinhodge
Copy link
Contributor Author

Ouch, sorry this stole hours of your time! Thanks for looking it over, much appreciated.

@aidanlister
Copy link
Collaborator

No worries, thank you muchly for contributing your well written and tested code back!

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.

None yet

2 participants