Skip to content

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

Merged
tstirrat15 merged 4 commits intoauthzed:mainfrom
holgerstolzenberg:feature/add-security-section-to-openapi-specs
Nov 12, 2024
Merged

Add missing global 'security' declaration to openapi.proto#120
tstirrat15 merged 4 commits intoauthzed:mainfrom
holgerstolzenberg:feature/add-security-section-to-openapi-specs

Conversation

@holgerstolzenberg
Copy link
Copy Markdown
Contributor

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

Original idea: authzed/authzed-go#255

@tstirrat15
Copy link
Copy Markdown
Contributor

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

@tstirrat15
Copy link
Copy Markdown
Contributor

@holgerstolzenberg
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@holgerstolzenberg
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Yep, this makes sense now. Thank you!

@tstirrat15
Copy link
Copy Markdown
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
Copy Markdown
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
@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