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

Next Version #111

Merged
merged 39 commits into from
Sep 4, 2020
Merged

Next Version #111

merged 39 commits into from
Sep 4, 2020

Conversation

bencorlett
Copy link
Member

@bencorlett bencorlett commented Aug 31, 2020

This PR serves as the basis for an entire rewrite of this package. The previous version has been around for a very long time and is quite long in the tooth.

Key improvements over the older version are:

  1. The overall architecture in the older version leads to a lot of duplication in tests and our tests are in fact often not really testing anything meaningful.
  2. After re-reading the spec, there are a couple of areas where we currently don't correctly address creating an OAuth signature. Obviously this works for most folks, however there's some edge cases which we don't account for. Upon searching the web for other OAuth 1 packages, it appears nobody (that I've seen) accounts for these. An example is where the same parameter is found in both the request body and the query string. Both instances should be used for generating the OAuth signature. The new version accounts for this.
  3. The creation of additional providers which customise functionality was painful in the previous iteration. By utilising PSR-7, PSR-17 and PSR-18, providers have a lot more flexibility to setup unique requests if they need to instead of overriding a lot of functionality.
  4. Again, with the addition of PSR, we have decoupled from requiring specific Guzzle versions. We suggest Guzzle as a perfect partner for this package, however our only dependency is an implementation of the relevant PSRs as well as a Guzzle support package for handling RFC 3986 encoding/decoding. The final iteration of this package may remove that dependency.
  5. The rewrite express a Generic Provider, which any OAuth1-compliant server should work with. We will likely remove all additional providers by extracting them into external packages (up for debate) and supply only the Generic Provider out of the box.
  6. The rewrite has extensive unit test coverage with many scenarios in key areas, such as signature generation and OAuth flow. We have taken examples right out the OAuth spec and plugged them into our tests to ensure that we catch the edge cases exposed by the spec. We also have 100% code coverage, obviously 😉.

To do:

  • Decide if we extract providers out and ship with only a Generic Provider, or do we ship with the same providers as the old version? Thoughts: maybe we ship with the most common ones. Some less common ones like Magento 1 introduce the requirement of XML-based PHP extensions being dependencies…
  • Write up detailed documentation with usage examples.
  • Add a handy Guzzle middleware to sign authenticated OAuth 1 requests, or recommend the usage of the official OAuth subscriber.
  • Decide what properties to officially support on the User object. At the time of writing, we support id, email, username and all other data is provider-specific and is accessible via the user's metadata array.
  • Explore adding integrations/bridges (likely in separate repositories) for common frameworks such as Laravel/Symfony.

@bencorlett bencorlett changed the base branch from master to develop August 31, 2020 11:25
@mfn
Copy link

mfn commented Aug 31, 2020

image
💥

😄

@bencorlett bencorlett force-pushed the feature/new-version-2020 branch 2 times, most recently from c08a56e to 581c5fb Compare September 1, 2020 04:52
@bencorlett bencorlett changed the title Version 2 Next Version Sep 1, 2020
@bencorlett bencorlett force-pushed the feature/new-version-2020 branch 3 times, most recently from f1f7a96 to 7ca7918 Compare September 2, 2020 09:04
@bencorlett
Copy link
Member Author

image
💥

😄

@mfn not so much anymore!

…ter implementation for development purposes.

The Twitter provider will be likely separated into a standalone package and we’ll only ship with a spec compliant, OAuth 1 provider.
…concerns and starts improving test coverage of implementation thus far.
…1 compliant servers and for the purposes of testing the functionality exposed by the base provider.
… the normalized parameters which meant it wasn’t providing the correct base string for signature generation.
@mfn
Copy link

mfn commented Sep 3, 2020

  • Add a handy Guzzle middleware to sign authenticated OAuth 1 requests, or recommend the usage of the official OAuth subscriber.

I'm not familiar with this implementation (so many changes 🙀) and I'm not sure if this it the proper forum to bring this up.

guzzle/oauth-subscriber works by storing the token credentials on the class in the constructor. This means once instantiated, this "state" persists to all requests performed with it.

I've a use-case where I want to pass these credentials on a per request basis (by allowing credentials being passed via Guzzles $options). This frees the Guzzle client from being tied to a specific set of credentials and allows to be more conservative with resources and logic:

  • a single Guzzle client instance can be used for all instances, as it's not tied to a set of credentials
  • every call explicitly can pass a different set of credentials

This is useful when interacting with multiple identities but not requiring to have multiple HTTP clients for this.


I searched for an opportunity to mention this somewhere in the hopes there are peers recognizing this as a valid use-case. Right now, to solve this, I've had to copy OAuth1 into a private source tree and adapt it accordingly.

The discussion in #88 triggered me to write this here.

@bencorlett
Copy link
Member Author

bencorlett commented Sep 3, 2020 via email

@mfn
Copy link

mfn commented Sep 3, 2020

Why don’t you want to create a separate client instance for each account you’re acting on behalf of?

Persistent background worker job connecting to the same remote OAuth1 system on behalf of identities likely different for each job.

To conserve memory and reduce GC, I try to make objects "stay" if possible, make them state-less so they can be shared like this.

Thanks!

…in preparation to introduce the RSA-SHA1 signer.
…ndicate the correct return types due to a shared trait.

This issue upsets both PHPStorm and PHPStan and the workaround solves this.
…7.4 when pulling in the lowest deps.

Shoutout to @arcanedev-maroc for this!
Because we integrate with PSRs, we only had to decouple our test suite from Guzzle because only Guzzle 7 provides PSR-18 support and Guzzle 7 requires PHP 7.2+. We switched this implementation out for a popular Guzzle 6 adaptor, which we don’t even use in our tests as we mock PSR-18.
@bencorlett
Copy link
Member Author

bencorlett commented Sep 4, 2020

Why don’t you want to create a separate client instance for each account you’re acting on behalf of?

Persistent background worker job connecting to the same remote OAuth1 system on behalf of identities likely different for each job.

To conserve memory and reduce GC, I try to make objects "stay" if possible, make them state-less so they can be shared like this.

Thanks!

Hi @mfn, here's a snippet of how I would solve your problem. I've included all of the boilerplate as well as I'd like to know your thoughts on it overall:

<?php

require_once __DIR__.'/vendor/autoload.php';

// Boilerplate to create a Twitter provider
$twitter = new League\OAuth1\Client\Provider\Twitter(
    new League\OAuth1\Client\Credentials\ClientCredentials(
        'app-id',
        'app-secret',
        'https://api.client.com/callback',
    )
);

// Create an OAuth client, giving a provider, PSR-17 HTTP request factory and PSR-18 client.
//
// You can use:
// 1.  `http-interop/http-factory-guzzle` for the PSR-17 HTTP request factory
// 2a. `guzzlehttp/guzzle` (v7 for PHP 7.2+) or
// 2b. `php-http/guzzle6-adapter` (for PHP 7.1)
//
// I'm currently testing the package in PHP 7.1, so I'm using 1 and 2b from above
$oauthClient = new League\OAuth1\Client\Client(
    $twitter,
    new Http\Factory\Guzzle\RequestFactory(),
    new Http\Adapter\Guzzle6\Client()
);

// Now you need a PSR-7 request, I use `guzzlehttp\psr7` for this
$request = new GuzzleHttp\Psr7\Request('GET', 'https://api.example.com/me');

// Here's your token credentials / access tokens
$tokenCredentials = new League\OAuth1\Client\Credentials\Credentials('token-id', 'token-secret');

// Now, you could either set this with the client for continual use:
//
// $oauthClient->setTokenCredentials($tokenCredentials);
// $response = $oauthClient->executeAuthenticatedRequest($request);
//
// However you won't want to do this as this sets the token credentials for the client overall.
// You might want to send unique token credentials every time:

try {
    $response = $oauthClient->executeAuthenticatedRequest($request, $tokenCredentials);
} catch (Psr\Http\Client\ClientExceptionInterface $e) {
    // 
} catch (Throwable $e) {
    //
}

@bencorlett
Copy link
Member Author

@mfn out of curiosity - are you integrating with a known OAuth 1 provider? I'm looking through what still uses OAuth 1 and there isn't a lot!

@mfn
Copy link

mfn commented Sep 4, 2020

@bencorlett The code in general looks fine to me and decouples the token credentials, seems exactly what I was talking about.

are you integrating with a known OAuth 1 provider?

Yes, Twitter ;)

Currently using guzzle/oauth-subscriber absorbed in the code base and adapted to serve the use-case.

Once a (dev) release is available, I can try integrating it and see how it works out. At first the code looks boilerplate-y though, but I might judge this wrong. I'm just used to directly use Guzzle 6 and not having to deal with the abstractions too much.

Thanks!

@bencorlett
Copy link
Member Author

bencorlett commented Sep 4, 2020 via email

@bencorlett
Copy link
Member Author

Will open additional PRs for further work. Develop will point to the next version from now on.

@bencorlett bencorlett merged commit 9c74a05 into develop Sep 4, 2020
@bencorlett bencorlett deleted the feature/new-version-2020 branch September 4, 2020 10:55
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