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

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

Conversation

holgerstolzenberg
Copy link
Contributor

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

Original idea: authzed/authzed-go#255

@tstirrat15
Copy link
Contributor

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

@tstirrat15
Copy link
Contributor

@holgerstolzenberg
Copy link
Contributor Author

holgerstolzenberg commented Nov 8, 2024

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
Copy link
Contributor

tstirrat15 commented Nov 8, 2024

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
Copy link
Contributor Author

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

Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, I have two comments, otherwise LGTM

.gitignore Outdated Show resolved Hide resolved
authzed/api/v1/openapi.proto Show resolved Hide resolved
@holgerstolzenberg
Copy link
Contributor Author

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

@tstirrat15
Copy link
Contributor

Yep, this makes sense now. Thank you!

@tstirrat15
Copy link
Contributor

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
Copy link
Contributor

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.

@tstirrat15 tstirrat15 merged commit 9ce037a into authzed:main Nov 12, 2024
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants