eu-digital-green-certificates / dgc-testdata

Repository for storing generated QR code data for testing.
Apache License 2.0
157 stars 218 forks source link

BG: EXPECTEDSCHEMAVALIDATION #298

Open bhavin-qryptal opened 3 years ago

bhavin-qryptal commented 3 years ago

Affected Country: BG

@stamo , @SchulzeStTSI and @daniel-eder - FYI.

Issue Description

Payload does not comply with Schema.

Ref: BG/2DCode/raw/1.json BG/2DCode/raw/2.json

>           raise error
E           jsonschema.exceptions.ValidationError: None is not of type 'array'
E           
E           Failed validating 'type' in schema['properties']['v']:
E               {'description': 'Vaccination Group',
E                'items': {'description': 'Vaccination Entry',
E                          'properties': {'ci': {'description': 'Certificate '
E                                                               'Identifier, format as '

image

Proposed Solution

Removing these NULL values should resolve the issue.

    "r": null,
    "t": null,
stamo commented 3 years ago

Thanks a lot we are aware of that, that's why we issued 3 and 4 which are correct. The problem is that we have issued approximately 1000000 certificates in production with these null objects and we are asking everyone to make schema validation a little bit softer, to accept these QR codes

SchulzeStTSI commented 3 years ago

@bhavin-qryptal Sorry for the confusion. There is an exception currently for BG which should ignore nulls for arrays and a different format for DOB. Can you relax the schema f.e. like if(countryCode=="BG") then the type of r is not array, but ["null","array"]. DOB should be as well allowed for T00:00:00. We wait for the final decision tomorrow.

bhavin-qryptal commented 3 years ago

@SchulzeStTSI and @stamo , thank you for your inputs.

I believe, in order to relax this, schema change would be required. Validation script has NO specific logic to deal with individual attributes of the payload.

SchulzeStTSI commented 3 years ago

@bhavin-qryptal Let's see how we handle this. I would like to copy the schema into the script(folder) instead of loading it live. Then we can relax it easily. The official schema should not be touched.

bhavin-qryptal commented 3 years ago

@SchulzeStTSI , Are you suggesting to modify the schema before it is used for validation by test script? In that case, wouldn't such code fail validation when real life apps and services try to validate such codes?

SchulzeStTSI commented 3 years ago

@bhavin-qryptal Currently is the discussion about, if the verifier should do a mandatory validation of the schema. The most of the people cite Postal's Law, which means that the issuer should be very strict, but the verifier must be so flexible as possible. Under this assumptions the verifier schema can be relaxed, but the issuer must still following the strict one. Currently we have some apps outside which do a very strict validation, and some which are validate nothing. I think the golden way is somewhere in the middle. Let's see what the todays discussion outcome is.

bhavin-qryptal commented 3 years ago

@SchulzeStTSI , Thank you for explanation, it is useful. For now, validation script is marking such tests as XFAIL. Test script could be updated after today's discussion. It MIGHT be a good idea to have two sets of schema, One for issuer and other for validator.

SchulzeStTSI commented 3 years ago

@bhavin-qryptal Let's relax the schema here for the codes. What do you think where should place in the best case the schema copy?

bhavin-qryptal commented 3 years ago

@SchulzeStTSI , Please help me with few basic queries.

  1. Who would author the schema to use for validation?
  2. How would this new set of validation only schema be controlled and how would it remain in sync with schema use for generation?
  3. Likewise https://id.uvci.eu/DGC.schema.json, could we have new set of schema available at some public URL (e.g. https://id.uvci.eu/DGC.validation.schema.json)? Also this new validation schema set should be maintained under GitHub repository.

Looking forward to have your thoughts and comments.

vanlooverenkoen commented 3 years ago

In the future, this should be discussed first. Before we merge such a pull request. Right now all pull requests are merged even if the checks are failing. If the schema should be relaxed, the github checks should be relaxed as well so we don't have problems when other countries create a new pull request.

SchulzeStTSI commented 3 years ago

@vanlooverenkoen Yes you are absolutley right. This was an quick merge, just to highlight the current situation with the productive codes.

@bhavin-qryptal Regarding your questions:

  1. The schema is created within this repo: https://github.com/ehn-dcc-development/ehn-dcc-schema in multiple versions. We should create under "tests" a folder with the different version 1.0,1.1,1.2 etc and copy them into it. This schemas must be relaxed regarding the array handling for r,t,v (["null","array"]) and the dob(https://github.com/eu-digital-green-certificates/dgca-app-core-android/blob/main/decoder/src/main/java/dgca/verifier/app/decoder/JsonSchema.kt#L54 )
  2. The sync should be manually. All issuers and verifiers use the schema which are currently offically released. But this is for issuing. Verifiers can have a different behaviour (or no validation). This is still under discussion.
  3. We are working on the public URL. Still under discussion. Currently all verifier apps which are doing a validation integrate it hardcoded.
vaubaehn commented 3 years ago

3. We are working on the public URL. Still under discussion. Currently all verifier apps which are doing a validation integrate it hardcoded.

Verifier apps and wallet apps . Adjustments to the schemas made in the past two weeks and problems with implementations on issuer's side are already causing problems with wallet apps and verifier apps, where (previous) schemas and validation rules are hardcoded, see issue mentioned above @SchulzeStTSI 's reply.

Are developers of wallet and verifier apps are informed pro-actively from your side to take account of changes? Or are they forced to regularly track changes here by themselves?

In this view, does it even make sense that wallet apps and verifier apps already have gone live, when they can't handle all adjustments from the past weeks?

Thank you in advance for your reply.

bhavin-qryptal commented 3 years ago
  • The sync should be manually. All issuers and verifiers use the schema which are currently offically released. But this is for issuing. Verifiers can have a different behaviour (or no validation). This is still under discussion.

Purpose if the validation script is to ensure that all the codes issues / submitted by member state complies with the standards. Actual validator app could have liberal implementation but the validation script should have stricter checks to ensure good compliance (e.g. schema compliance). Otherwise, we would miss capturing important compatibility issues upfront.

SchulzeStTSI commented 3 years ago

@vanlooverenkoen We announce all changes in our regular meetings. This kind of changes were announced last week, after the report from bulgaria at monday.

@bhavin-qryptal OK agreed. We should keep this repo as it is to ensure more compliance. I will contact stamo and then we move the codes to another repo for "in field" compatibility/versioning checks. Thank you. I let you know when we have setup it.