Skip to content

[ZEN-4778] Validation should allow empty scopes array in Security Requirement Object #490

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

Merged
merged 3 commits into from
Apr 10, 2019

Conversation

nbhusare
Copy link
Contributor

@nbhusare nbhusare commented Apr 5, 2019

The PR fixes the Validator to allow "empty scope arrays" in the Security Requirement Object for Security Schemes of type - oauth2 or openIdConnect.

In addition, for scheme of type oauth2, the validation checks if each scope in the array corresponds to a scope declared in one or more OAuth flows. Please note that this is NOT applicable to the scheme of type "openIdConnect".

048

… contains values if the secutiry-scheme-type is of type oauth2 or openIdConnect
@nbhusare nbhusare added the Bug label Apr 5, 2019
@nbhusare nbhusare self-assigned this Apr 5, 2019
@nbhusare nbhusare requested a review from tedepstein April 5, 2019 14:05
Copy link
Collaborator

@tedepstein tedepstein left a comment

Choose a reason for hiding this comment

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

Code changes look straightforward and minimal. Passed code review.

nbhusare added 2 commits April 8, 2019 19:23
…urity requirement object corresponds to a scope declared in one or more OAuth flows
}
}
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nbhusare , did something happen with formatting here? Indentation looks like it changed.

@tedepstein
Copy link
Collaborator

tedepstein commented Apr 10, 2019

@nbhusare , it looks like a whitespace formatting issue was introduced in commit d006279, addressing issue #485 / ZEN-4779 - Wrong error issued with OpenID Connect security scheme. Also, it's a good idea to mention the relevant github and Jira issue(s) in the commit message, for easier cross-referencing.

Aside from these minor issues, code changes look fine. I'll QA test it in the API Studio PR.

@tedepstein
Copy link
Collaborator

@neerajbhusare , this passed QA, so I will go ahead and merge. It would be good to add automated tests for fixes like these, but I don't want to hold up progress for it. I'll make a note on the Jira issue that we should add tests before closing the issue.

@tedepstein tedepstein merged commit 22bd3a0 into master Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants