B-Interactive / cloudflare-stream-wordpress

A fork of the official Cloudflare Stream plugin for WordPress.
GNU General Public License v2.0
16 stars 3 forks source link

Switch from zones to accounts API #11

Closed B-Interactive closed 2 years ago

B-Interactive commented 2 years ago

I'm opening this for contemplation. I'm currently not fully convinced a change is needed, but I realise I may also be looking at it wrong.

I've used the /zones API for API calls. This is as opposed to using the standard /accounts API.

Feedback in the Cloudflare forum prompted me to consider this again. At the very least, it looks like I need to update README.md, because referencing the /zones API as "more secure" doesn't appear to apply to Stream, based on that feedback.

The reason I originally used the /zones API, is because it, in conjunction with an API token with only Account.Stream:Edit permission, could access some general account data, that is otherwise unavailable using the /accounts API.

For example (using /zones):

curl -X GET "https://api.cloudflare.com/client/v4/zones/{ZONE ID}" -H "Authorization: Bearer {TOKEN}" -H "Content-Type:application/json"

Returns data that includes the account ID and account email (among other things).

Using the /accounts equivalent on the other hand, produces a permission error, unless additional permissions beyond Account.Stream:Edit are granted (I'm not sure which specific one is required):

curl -X GET "https://api.cloudflare.com/client/v4/accounts/{ACCOUNT ID}" -H "Authorization: Bearer {TOKEN}" -H "Content-Type:application/json"
{"success":false,"errors":[{"code":9109,"message":"Unauthorized to access requested resource"}],"messages":[],"result":null}

Currently, the /zones API is not being used to pull these additional details in this plugin. In an earlier (non-public) version of my own plugin, I had pulled the account email and ID and simply presented it in the plugin's settings.

However, potentially the same /zones API request could be made to grab the account specific subdomain. This is something I might raise with Cloudflare.

davidmpurdy commented 2 years ago

I don't think I fully understand how zones are supposed to relate to Stream structurally (if at all). Using zones feels a little strange to me since my understanding is that they're a level down, structurally (Stream is at the account level, zones are at the domain level, right?).

It is easier to get working though (I checked and it seems to work with any of the zone IDs in the account with Stream). I think you're right that's a Cloudflare question - I'd be interested to hear if that's an intentional architecture decision or maybe an accident of implementation.

B-Interactive commented 2 years ago

I think you're right @davidmpurdy, it would seem zones aren't supposed to really come into play with Stream.

My implementation using zones was originally at least, taking advantage of what now appears to be an anomaly with that end-point.

Given the official recommendation seems to be to use /accounts (for Stream), and currently this plugin does not try to take advantage of the data available with /zones, I will commence switching it over to /accounts.

Cloudflare have said they will look into including an API call that can retrieve the account specific subdomain, so that's something to keep an eye out for.

B-Interactive commented 2 years ago

I've pushed a branch with this update, but I want to run it by you @davidmpurdy first, to make sure this isn't going to impact your current efforts with the blocks. Please let me know!

If the plugin is already set up with Zone ID and API token, this update will automatically pull and save the Account ID, so no intervention should be needed by the user. This action is prompted by either of these two things:

  1. An admin accesses the plugin's settings page (regardless of whether Save Settings is hit).
  2. A Cloudflare Stream API call is made.

In both scenarios, the Account ID is only fetched, if it isn't already in the local database. It is using add_option(), so it will not update an existing Account ID, only an absent one.

davidmpurdy commented 2 years ago

That doesn't cause any issues for the block that I can see (this past week got busy and I haven't done much to get an actual PR ready anyway).

My only question is whether it's worth creating the base URL in its own function so it wouldn't have to be changed in PHP and JS in the future if it changed again? Maybe even including the account ID so the two places we interface with the API just need to be aware of endpoint/authentication/payload? Or maybe that's over-thinking it since it's not likely to change often if at all.

Edit: Also, I like the approach to automatically getting the account ID if it's needed. Realizing there's a slight edge case where I don't believe uploading a new video via the block would trigger the update, and since we're generating the iframe locally, is it conceivable that if someone was just uploading (not browsing) they might not trigger the update for a while? I don't know if it matters since the zone endpoint still works, just thinking if we did get the base URL from a function, that could be used to trigger the update.

Apologies if any of this is half-baked - I'm still in the midst of some big work deadlines so I haven't thought through this as carefully as I normally would.

B-Interactive commented 2 years ago

Thanks @davidmpurdy , that's a good point regarding the base URL, and it would actually benefit solutions for the variable sub-domains, or even Cloudflare URL options (videodelivery.net, cloudflarestream.com) which may be a part of that solution too.

I'll take another look at it and see about eliminating redundancy on the current doubling up of those request functions too.

B-Interactive commented 2 years ago

Completed with 8aec255043ced8b65abf17c7a2889eaef37ca1a2.

davidmpurdy commented 2 years ago

Minor bug with trying to upgrade to the account ID when it doesn't exist.

Fixed with #13

B-Interactive commented 2 years ago

Thanks for picking that up, and providing the patch!