Add authentication validation to the JSON schema #2646
Conversation
…ot sure if this is intended but it makes sense to me.
…sed. The error message is a bit janky, though
sander1095
requested review from
Aniruddh25,
aaronburtle,
RubenCerna2079,
ravishetye,
rohkhann,
neeraj-sharma2592,
sourabh1007 and
sezal98
as code owners
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
@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! |
…serialization issues
|
Hi @Aniruddh25 and @RubenCerna2079 ! I've found some to look into this PR. I've updated it. The failed unit tests now run correctly on my machine! There's 1 impactful change you should be aware of: I decided to move away from I think it's worth a try to run the pipelines again and see the results! If the integration fail again, please post the stacktraces. I can't access them as I get a 403 error when trying to see more details in Azure Pipelines. |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
@sander1095 I left a few comments, besides that it LGTM, I will approve once the comments are resolved.
|
@sander1095 I just saw that some of the tests related to the config validation are failing, if you need any help with that, please let me know. |
|
@RubenCerna2079 That's annoying. Last time I checked they passed on my machine, I'll have another look. My sincere apologies for this! Also: I see integration tests fail. I can not inspect the failures as I dont have permission for Azure DevOps. Can you share the test failure reasons/stracktraces? |
|
@sander1095 I just sent the details through teams |
JsonShema.NET supports the `oneof + const` validation which NJsonSchema didn't, which is great. Sadly JsonSchema.NET returns tons of errors about things that aren't really errors. An example is the if statement for cosmosdb that requires the database/container/schema properties. If the user has selected mssql, they haven't selected cosmos, and so all the IF statements for cosmosdb are FALSE. This makes error handling quite difficult. My workaround is to switch to another popular library: Newtonsoft.Json.Schema. We're already using Newtonsoft.Json in some parts, so this seems to be a better fit. This also returns the LineNumber validation, which is great as this requires less changes in the PR overall!
|
Hey @RubenCerna2079 ! Good news! I just fixed the unit tests by switching from JsonSchema.Net to Newtonsoft.Json.Schema. This package fixes the issues I had with NJsonSchema AND it actually has functioning error reporting. This package seems to be a much better fit as the project already has a dependency on Newtonsoft.Json. |
|
/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. |
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
jwtproperty. 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?
jwtproperty.StackOverflow helped me with the finer details of the validation.NJsonSchematoJsonSchemaNetbecauseNJsonSchemadoesn't validateanyof + constcorrectly.JsonSchemaNetdoes support thishttpstohttpin the schema file to be spec compliant. Draft-07's original URL is http-based.httpsurl, it will fail because of the unknown schema URL.How was this tested?
Sample Request(s)