search

authzed / api

Protocol Buffers & gRPC Services used by SpiceDB
https://authzed.com/docs
Apache License 2.0
16 stars 18 forks source link

Add missing global 'security' declaration to openapi.proto #120

Closed holgerstolzenberg closed 3 weeks ago

holgerstolzenberg commented 3 weeks ago

Fixes ApiKeyAuth method being used by clients generated via OpenAPI generator.

Original idea: https://github.com/authzed/authzed-go/pull/255

tstirrat15 commented 3 weeks ago

Gonna have a look at this today. Thanks for putting it up!

tstirrat15 commented 3 weeks ago

How does it differ from what's right above it? https://github.com/authzed/api/pull/120/files#diff-2c89108fef442d02f29499535203f20595b6fdd3cff830a5b3fd40e979eece0bR34-R41

holgerstolzenberg commented 3 weeks ago

Okay maybe I have messed up the syntax a bit 🙈. Let's take it different, as I am not familiar with buf.

This is what the target swagger.json should look like:

Screenshot 2024-11-08 at 17 33 53

I have updated the PR, hope that makes more sense now.

tstirrat15 commented 3 weeks ago

Hmm... the thing I'm getting hung up on is that there's two security blocks. Is that correct/expected? Should we change the existing security block rather than adding a new one? I'm also fairly unfamiliar with how this part of the OpenAPI spec works.

holgerstolzenberg commented 3 weeks ago

No I think this is totally fine - even if not being really straight forward at fist. The official docs unfortunately use yaml for examples already.

So you have essentially two elements:

securityDefinitions defines what is actually available from your API, but it does not apply that auth method to anywhere. Here it just says: The API supports an Authentication named ApiKeyAuth that is of type apiKey and will be applied by using a header named Authorization.

In the security section you just name where your defined security methods are going to be applied by just cross referencing them. Line 3035 means, take the auth method named ApiKey and then apply it to all endpoints ([]).

Even if that looks unfamiliar, it is confirmed working. So you might want to take a look here: https://github.com/ewerk/authzed-http-client/blob/fc74dc40f8d81720f976b1d866106d4e8907bfed/.github/workflows/main-build.yml#L34

holgerstolzenberg commented 3 weeks ago

@tstirrat15 Could you also please review - I am not able to request that and it seems like a second review is required.

tstirrat15 commented 3 weeks ago

Yep, this makes sense now. Thank you!

tstirrat15 commented 3 weeks ago

Before I merge this I'm going to try pulling it into authzed-go as a SHA and see if the openapi definition makes sense.

tstirrat15 commented 3 weeks ago

I checked this out and it looks good. I'm going to go ahead and merge this, and then I'll do the work to pull it into authzed-go.