v4 API: OAuth #74

n7studios · 2024-03-18T17:22:36Z

Summary

Implements the v4 API's OAuth implementation, removing API Key and Secret authentication.

Tidies up the make_request method, as 4xx and 5xx errors would always throw a ClientException or ServerException.

Tidies up the logging by removing repetitive log entries for helper methods get, post, put, delete.

Further PR's to follow to add, update and remove methods to provide full compatibility with the v4 API.

Testing

Tests outside of the scope of this PR are marked as incomplete using $this->markTestIncomplete(). As further PR's add compatibility to each section of the v4 API, these tests will be updated and the flag removed.

Added:

testGetOAuthURL: Test that the new get_oauth_url method returns the correct URl to begin the OAuth process.
testGetAccessToken: Test that the new get_access_token method returns the expected data.
testGetAccessTokenWithInvalidAuthCode: Test that the new get_access_token returns a ClientException when an invalid authorization code is specified.
testRefreshToken: Test that the new refresh_token method returns the expected data.
testRefreshTokenWithInvalidToken: Test that the new refresh_token returns a ServerException when an invalid refresh token is specified.

Updated:

testInvalidAPICredentials: Test that a ClientException is thrown when an invalid Client ID, Secret and Access Token is specified.
testGetAccount: Updated assertions to match the new shape of the account endpoint data

Removed:

testDebugAPIKeyAndSecretAreMasked: Credentials (such as Client ID, Secret, Access Token) are not logged. The Access Token is included in the header of any authenticated requests, and therefore again is not logged. Masking of the redundant API Key and Secret are no longer required.

Checklist

I have written a test and included it in this PR
I have run all tests and they pass
The code passes when running the PHP CodeSniffer
The code passes when running PHPStan
I have assigned a reviewer or two to review this PR (if you're not sure who to assign, we can do this step for you)

noelherrick

I think this all makes sense, but I am adding the primary developer on the API project to see if I'm missing anything.

noelherrick · 2024-03-20T15:11:35Z

src/ConvertKit_API.php

@@ -1316,9 +1315,6 @@ public function delete_custom_field(int $id)
     */
    public function list_purchases(array $options)


I don't think we have any parameters on this API call, so I don't know if we still want to keep this options parameter.

This is presently unchanged, as I didn't want to submit a large, unreadable PR with all changes for v4 compatibility. I'll make sure this is addressed when submitting the v4 PR for purchases.

README.md

src/ConvertKit_API.php

noelherrick · 2024-03-20T15:30:24Z

@mercedesb I would love to get your eyes on this implementation of the V4 API

mercedesb

I'm not sure I caught everything but there are some breaking changes from V3 to V4 that need to be addressed. We have breaking changes documentation. And I'll get the couple items I noticed missing added to it too.

src/ConvertKit_API.php

mercedesb · 2024-03-20T21:26:30Z

src/ConvertKit_API.php

@@ -424,8 +539,7 @@ public function create_tag(string $tag)
        return $this->post(
            'tags',
            [
-                'api_key' => $this->api_key,
-                'tag'     => ['name' => $tag],
+                'tag' => ['name' => $tag],


The request shape has changed in v4. The root tag attribute is no longer supported.

mercedesb · 2024-03-20T21:27:35Z

src/ConvertKit_API.php

-                'api_key' => $this->api_key,
-                'tag'     => $apiTags,
-            ]
+            ['tag' => $apiTags]


Creating multiple tags is now under the bulk namespace.

mercedesb · 2024-03-20T21:27:57Z

src/ConvertKit_API.php

-            'api_secret' => $this->api_secret,
-            'email'      => $email,
-        ];
+        $options = ['email' => $email];


The param has changed in v4 from email to email_address.

mercedesb · 2024-03-20T21:30:17Z

src/ConvertKit_API.php

-                'api_secret' => $this->api_secret,
-            ]
-        );
+        return $this->delete(sprintf('subscribers/%s/tags/%s', $subscriber_id, $tag_id));


I believe this path should actually be sprintf('tags/%s/subscribers/%s', $tag_id, $subscriber_id). Ref

mercedesb · 2024-03-20T21:31:01Z

src/ConvertKit_API.php

-                'api_secret' => $this->api_secret,
-                'email'      => $email,
-            ]
+            ['email' => $email]


V4 doesn't currently support unsubscribing by email address.

mercedesb · 2024-03-20T21:32:59Z

src/ConvertKit_API.php

-                'api_secret' => $this->api_secret,
-            ]
-        );
+        return $this->delete(sprintf('automations/hooks/%s', $rule_id));


Webhooks routes have changed from automations/hooks to webhooks in v4

n7studios · 2024-03-21T09:22:47Z

@mercedesb I would love to get your eyes on this implementation of the V4 API

Thanks for the review. Scope of this PR is to support v4 OAuth:

Further PR's to follow to add, update and remove methods to provide full compatibility with the v4 API.

Comments noted for said future PR's.

n7studios · 2024-03-21T09:29:02Z

I'm not sure I caught everything but there are some breaking changes from V3 to V4 that need to be addressed. We have breaking changes documentation. And I'll get the couple items I noticed missing added to it too.

Thanks for the review @mercedesb. The scope of this PR is to support v4 OAuth, for ease of review:

Further PR's to follow to add, update and remove methods to provide full compatibility with the v4 API.

If you and @noelherrick prefer, happy to combine all work into a single PR, though. Let me know!

noelherrick · 2024-03-21T10:58:40Z

@n7studios this PR is pretty thick as is, so I understand your hesitancy, but Mercedes suggested about 5 one-liners, so I don't think it would bloat it too much. I would hate for those suggestions to get lost, but that being said, I'm fine with not addressing them here as long as we account for all of those.

What is your plan for merging? It looks like this is the main PR that all the other PRs will merge into.

noelherrick · 2024-03-21T11:01:17Z

@mercedesb What did you think of the OAuth part specifically? Any comments around that since that's the heart of this PR? Also, this will supplant any usage of the V3 API - will we freeze the V3 API upon the release of V4? Or should there be a plan around releasing updates for consumers still on V3?

mercedesb · 2024-03-21T13:22:39Z

The scope of this PR is to support v4 OAuth, for ease of review

Sorry @n7studios, I missed that bit and jumped the gun. I have no preferences on how you manage the work needed to support V4, feel free to break apart PRs however makes sense to you.

What did you think of the OAuth part specifically?

The OAuth bits LGTM

Also, this will supplant any usage of the V3 API - will we freeze the V3 API upon the release of V4? Or should there be a plan around releasing updates for consumers still on V3?

We are not planning to make any more updates to V3, unless we find major security issues. So there should probably be a plan for releasing updates to V3 as a "just in case" though the likelihood is slim.

We plan to eventually sunset V3. But I expect that to take months since we have so many people using it.

V4 will eventually support API Keys to make that process easier for creators who are not planning on creating apps and just want to use the API for their own personal uses.

n7studios · 2024-03-21T13:29:16Z

@n7studios this PR is pretty thick as is, so I understand your hesitancy, but Mercedes suggested about 5 one-liners, so I don't think it would bloat it too much. I would hate for those suggestions to get lost, but that being said, I'm fine with not addressing them here as long as we account for all of those.

Let's address those in separate PR's (as some are likely to be more than one liners e.g. needing to support cursor based pagination),. I'll add a comment to each linking to the separate PR, so they're tracked.

What is your plan for merging? It looks like this is the main PR that all the other PRs will merge into.

Correct - other v4 PR's will merge in to this one, which will then form our final PHP SDK implementation into the main branch and subsequently published as version 2.0 of our SDK.

n7studios · 2024-03-21T13:37:35Z

Sorry @n7studios, I missed that bit and jumped the gun. I have no preferences on how you manage the work needed to support V4, feel free to break apart PRs however makes sense to you.

Thanks for reviewing - happy to assign you to other PRs if you'd like to review, as appreciate your input here.

V4 will eventually support API Keys to make that process easier for creators who are not planning on creating apps and just want to use the API for their own personal uses.

Will API Key + Secret authentication work in the same way as it does in the v3 API? Appreciate this isn't supported right now, but want to make sure this SDK is designed to accommodate both methods (API, OAuth) in the future.

mercedesb · 2024-03-21T14:07:20Z

Will API Key + Secret authentication work in the same way as it does in the v3 API?

Not the exact same way, but close. We want to adhere to RFC6750. So we'll be using the Authorization header. And there won't be a secret.

I'm hoping to add support for creating multiple API keys (rather than just one) so that when we get to the point of supporting scopes (for OAuth too), then we can limit specific OAuth apps and API Keys on read/write permissions for specific resources so creators can have more fine-grained control.

We won't be migrating V3 keys b/c we don't want to break existing landing page embeds and some other functionality so all V4 keys will be new.

…thods

# Conflicts: # tests/ConvertKitAPITest.php

v4 API: Webhooks

…e-subscriber-tests

…pagination

v4 API: Improve Subscriber Tests

…pagination

…ilter

This allows specific headers to be defined depending on the request i.e. an API request or fetching HTML.

v4 API: Forms and Landing Pages: Pagination and Total Count

…ml-resource

v4 API: Get HTML Resources

v4 API: Docs

v4 API: Use Trait for Methods

n7studios added the enhancement label Mar 18, 2024

n7studios added this to the 2.0 milestone Mar 18, 2024

n7studios self-assigned this Mar 18, 2024

n7studios requested review from a team, noelherrick and corydhmiller and removed request for a team March 20, 2024 12:43

n7studios marked this pull request as ready for review March 20, 2024 12:43

noelherrick reviewed Mar 20, 2024

View reviewed changes

noelherrick requested a review from mercedesb March 20, 2024 15:28

mercedesb reviewed Mar 20, 2024

View reviewed changes

Updated methods

9bd41a8

Started tests

a47d0e9

corydhmiller removed their request for review March 21, 2024 14:19

n7studios added 7 commits March 21, 2024 14:36

Started work on subscriber methods

bf6c5d4

Completed first pass of subscriber methods

344b717

Added callback_url for bulk add subscribers

2d9db50

Started stubbing tests

24faea8

Add get_response_interface and get_response_status_code helper me…

b0588ce

…thods

Remove status_code, as response object includes it

ba62560

Moved test

f116beb

n7studios added 28 commits April 2, 2024 17:18

Merge remote-tracking branch 'origin/v4-api-oauth' into v4-api-webhooks

8d57692

# Conflicts: # tests/ConvertKitAPITest.php

Merge pull request #92 from ConvertKit/v4-api-webhooks

dba401b

v4 API: Webhooks

Merge remote-tracking branch 'origin/v4-api-oauth' into v4-api-improv…

0abfdc3

…e-subscriber-tests

Completed tests for type filtering

87e871a

Merge remote-tracking branch 'origin/v4-api-oauth' into v4-api-forms-…

02de911

…pagination

Merge pull request #93 from ConvertKit/v4-api-improve-subscriber-tests

59c50ab

v4 API: Improve Subscriber Tests

Merge remote-tracking branch 'origin/v4-api-oauth' into v4-api-forms-…

67e1346

…pagination

Updated get_forms and get_landing_pages to support new status f…

57bbb00

…ilter

Completed tests

66cb9ae

Set headers on Request object instead of Client

68d149a

This allows specific headers to be defined depending on the request i.e. an API request or fetching HTML.

Run testGetResource tests

8fcdcc8

Coding standards

688f14e

Merge pull request #91 from ConvertKit/v4-api-forms-pagination

10f8442

v4 API: Forms and Landing Pages: Pagination and Total Count

Merge remote-tracking branch 'origin/v4-api-oauth' into v4-api-get-ht…

06c94be

…ml-resource

Use helper methods for user agent and headers

2bb2b37

Add auth parameter to request_headers method with tests

fcb034f

Merge pull request #94 from ConvertKit/v4-api-get-html-resource

a596a33

v4 API: Get HTML Resources

First draft of migration guide

f6563c9

Update wording on how to register an OAuth application

f327145

Added links between readme and migration guide

fb47594

Fixed indenting of sub bullet points

c5ac174

Implement methods as traits for portability across multiple API clients

3d22954

Coding Standards

d93c396

Move VERSION const

e73f492

Fix tests to call correct get_request_headers method

6277e11

Fix get_subscriber_id definition when no subscribers found by email

3276476

Merge pull request #95 from ConvertKit/v4-api-docs

4ade456

v4 API: Docs

Merge pull request #96 from ConvertKit/v4-api-methods-traits

d079068

v4 API: Use Trait for Methods

n7studios merged commit 503cfea into master Apr 17, 2024
4 checks passed

n7studios deleted the v4-api-oauth branch June 26, 2024 03:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v4 API: OAuth #74

v4 API: OAuth #74

n7studios commented Mar 18, 2024 •

edited

Loading

noelherrick left a comment

noelherrick Mar 20, 2024

n7studios Mar 20, 2024

noelherrick commented Mar 20, 2024

mercedesb left a comment

mercedesb Mar 20, 2024

mercedesb Mar 20, 2024

mercedesb Mar 20, 2024

mercedesb Mar 20, 2024

mercedesb Mar 20, 2024

mercedesb Mar 20, 2024

n7studios commented Mar 21, 2024

n7studios commented Mar 21, 2024

noelherrick commented Mar 21, 2024

noelherrick commented Mar 21, 2024

mercedesb commented Mar 21, 2024

n7studios commented Mar 21, 2024

n7studios commented Mar 21, 2024

mercedesb commented Mar 21, 2024

		@@ -1316,9 +1315,6 @@ public function delete_custom_field(int $id)
		*/
		public function list_purchases(array $options)

v4 API: OAuth #74

v4 API: OAuth #74

Conversation

n7studios commented Mar 18, 2024 • edited Loading

Summary

Testing

Checklist

noelherrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelherrick commented Mar 20, 2024

mercedesb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n7studios commented Mar 21, 2024

n7studios commented Mar 21, 2024

noelherrick commented Mar 21, 2024

noelherrick commented Mar 21, 2024

mercedesb commented Mar 21, 2024

n7studios commented Mar 21, 2024

n7studios commented Mar 21, 2024

mercedesb commented Mar 21, 2024

n7studios commented Mar 18, 2024 •

edited

Loading