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

Fix linting errors in OAS files. #334

Merged
merged 4 commits into from
Mar 21, 2023
Merged

Fix linting errors in OAS files. #334

merged 4 commits into from
Mar 21, 2023

Conversation

msporny
Copy link
Contributor

@msporny msporny commented Jan 23, 2023

This PR fixes linting errors in the OAS files. OAS linting requires that every endpoint define security properties and at least one server endpoint. Reviewers should pay specific attention to this PR because it does add authorization mechanisms available on each endpoint, including noAuth, didAuth, oAuth2, and zCap (since the current set of implementers support at least one of those mechanisms, if not multiple). Setting this PR to draft so we can discuss.

The intent of this PR is NOT to establish the correct authz mechanisms for each endpoint, that will be done in a future PR. The goal here is to get "close enough" and then fine tune the authz mechanisms in a future PR. Do not take the authz mechanisms provided as anything more than a work in progress at this point in time.

Comment on lines 34 to 38
authorizationUrl: /oauth2/authorize
tokenUrl: /oauth2/token
scopes:
read: Grants read access
write: Grants write access
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
authorizationUrl: /oauth2/authorize
tokenUrl: /oauth2/token
scopes:
read: Grants read access
write: Grants write access
authorizationUrl: /TBD
tokenUrl: /TBD
scopes:
TBD: There are currently no scopes defined.

Copy link
Contributor Author

@msporny msporny Mar 21, 2023

Choose a reason for hiding this comment

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

This has been removed in commit f2c17c0, and is ultimately up to the instance to define.

oAuth2:
type: oauth2
flows:
authorizationCode:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't have to use authorizationCode -- need to figure out how to say "this is up to implementers".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been removed in commit f2c17c0 and is up to the instance to define.

paths:
components:
securitySchemes:
noAuth:
Copy link
Contributor Author

@msporny msporny Jan 24, 2023

Choose a reason for hiding this comment

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

Suggested change
noAuth:
networkAuth:

Choose a reason for hiding this comment

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

As noted on call, we understand the scope of this PR is to fix lint errors (we never want to compromise about dev tools not being broken), but we will update in future PRs to correctly make sure we don't unintentionally rule out use cases for "no authorization (header) required", "I enforce access to this endpoint with a firewall" or "some other secret auth method".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been fixed in f2c17c0.

@msporny msporny marked this pull request as ready for review February 21, 2023 20:29
Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

We should rename oauth2 to oauth2-vcapi so that we can only define the parts we know we can define at this time. There were some requirements when using oauth2 that weren't really ready to set, so if we invent a name for now as a placeholder we can just do the parts we have consensus on / know what we're doing.

@msporny
Copy link
Contributor Author

msporny commented Mar 21, 2023

We should rename oauth2 to oauth2-vcapi so that we can only define the parts we know we can define at this time.

I was able to update it to just oAuth2 and make the scheme bearer without defining all the oauth-y details in commit f2c17c0.

@msporny msporny requested a review from dlongley March 21, 2023 18:41
@msporny
Copy link
Contributor Author

msporny commented Mar 21, 2023

Discussed on the 2023-03-21 call.

Normative, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 6f8b6ea into main Mar 21, 2023
@msporny msporny deleted the msporny-fix-linting-errors branch March 21, 2023 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants