-
Notifications
You must be signed in to change notification settings - Fork 248
Add authentication validation to the JSON schema #2646
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
Conversation
…ot sure if this is intended but it makes sense to me.
…sed. The error message is a bit janky, though
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for adding this validation and improving developer experience.
@Aniruddh25 I've processed your comments. I took a look at the failures of the CI pipeline of the last commits. They seem to be unrelated to my changes: Let's hope that a re-run will make them go green! |
@RubenCerna2079 Could you perform a re-run on this PR :) |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
I see I broke some config tests. I'll hopefully have time to look into this next week! |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
This version also can't be made available in their private feeds. Based on Ruben Cerna's advice, I added 13.0.3-beta1 instead. This then causes NU1605 issues, which I was also adviced to ignore.
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
I can now see the integration test failure reasons! Some of them fail because the package migration changed the JSON Schema result messages. I can update this soon. |
…ed by me and the team
Other tests failing in the pipeline do not fail locally, so I assume these are flaky.
…-auth-to-jsonschema
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for pushing through this.
/azp run |
Azure Pipelines successfully started running 6 pipeline(s). |
Hey @sander1095, I saw that the tests that were failing are not due to your PR, so I decided to ignore them in order to get your PR in. I will solve this issue in a separate PR. Thanks for your work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for these changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for these contributions!
Why make this change?
Closes #2643
The JSON schema does not contain a list of auth providers, nor does it validate the use of the
jwt
property. This results in a suboptimal developer experience. By adding this list and validation, developers have an easier time learning about all the options and can be more productive.What is this change?
jwt
property. StackOverflow helped me with the finer details of the validation.NJsonSchema
toNewtonsoft.Json.Schema
becauseNJsonSchema
doesn't validateanyof + const
correctly.Newtonsoft.Json.Schema
does work in this case.https
tohttp
in the schema file to be spec compliant. Draft-07's original URL is http-based.https
url, it will fail because of the unknown schema URL.How was this tested?
Sample Request(s)