search

ConsumerDataStandardsAustralia / standards-maintenance

This repository houses the interactions, consultations and work management to support the maintenance of baselined components of the Consumer Data Right API Standards and Information Security profile.
41 stars 9 forks source link

Update Register and DCR Swagger specs to use Common Field Types #546

Closed CDR-API-Stream closed 1 year ago

CDR-API-Stream commented 2 years ago

Description

Some parameters in the following sections specify field types that are not aligned to the Common Field Types definitions -

The details have been acknowledged in the Known Issues list since v1.13.0 (Register APIs use different field type definitions).

Area Affected

Register Swagger DCR Swagger

Change Proposed

Change all field types to use Common Field Types.

DSB Proposed Solution

The current DSB proposal for this issue is in this comment.

CDR-API-Stream commented 1 year ago

The changes have been staged for review: https://github.com/ConsumerDataStandardsAustralia/standards-staging/compare/release/1.20.0...maintenance/546

anzbankau commented 1 year ago

A small issue we have noticed in the staged changes - There appears to be an issue with the definition of redirect_uris. It is defined as an array but is also defined as x-cds-type": "URIString", which would make it a single value.

CDR-API-Stream commented 1 year ago

A small issue we have noticed in the staged changes - There appears to be an issue with the definition of redirect_uris. It is defined as an array but is also defined as x-cds-type": "URIString", which would make it a single value.

Thanks @anzbankau, this issue has been corrected.

perlboy commented 1 year ago

A few notes:

  1. Can we validate that the Register is definitely compliant with the proposed types already?
  2. pattern is used at the same time as ExternalRef which is "ok" but seems to result in double defining a few fields (seems like the Bearer token field in particular)
  3. client_id and the Client Id parameters specify it as String but I believe oauth2 client identifiers are limited to UTF-8 ASCII? I admit my W3C keymap knowledge isn't great though so maybe I'm wrong.
  4. I believe String type in CDS are case sensitive but that type is now being proposed for use *id fields. Are we to assume identifiers can now collide in case sensitivity? That seems to be what was permitted before but now it's being explicitly stated? This is particularly relevant because at some point in history the Register "magically" converted all identifiers to upper case causing a number of issues for existing installations while the Regulator assumed that since they were a guid they were interchangeable.
  5. iat and other epoch times are listed as PositiveInteger, it's pedantic but this excludes 0 which isn't technically accurate but maybe it makes sense (ie. 0 would be a schematic error condition)
  6. Page number refers to int32 but PositiveInteger has no upper bound (I admit it'd be pretty amazing to have 2^32 holders though!)
  7. The type of x-v/x-min-v has changed from string to integer in a number of areas, this is an explicit spec change 🛑 . I note that headers are, by definition, strings and these fields are specified as string throughout other Standards endpoints. This matters quite a bit since codegen often does casting poorly in these cases. I notice some of the occurrences have a default specified, as a string, presumably because that's the only way to make it compliant (headers are strings and strings only).
  8. Indenting. A lot of the changes aren't appropriately indented.
CDR-API-Stream commented 1 year ago

To be explicit, the proposal to remediate this issue is to modify the OAS for the Register APIs to use the common field types only if this change will not make the standards misaligned with the current ACCC implementation.

We will check to ensure that this is the case. The comments from @perlboy are much appreciated and will be a starting point for this check.

markverstege commented 1 year ago

Thanks @perlboy, 

1. Can we validate that the Register is _definitely_ compliant with the proposed types already?

ACCC is reviewing the Register APIs swagger file.

2. `pattern` is used at the same time as `ExternalRef` which is "ok" but seems to result in double defining a few fields (seems like the Bearer token field in particular)

Thanks for picking this up. As discussed in the iteration call last week, the pattern needs to be removed. This is now updated in the latest staged changes. 

3. `client_id` and the `Client Id` parameters specify it as `String` but I believe oauth2 client identifiers [are limited to UTF-8 ASCII](https://www.rfc-editor.org/rfc/rfc6749#page-71)? I admit my W3C keymap knowledge isn't great though so maybe I'm wrong.

I have set this to be ExternalRef so that the standards don't seek to change the meaning.

4. I believe String type in CDS are case sensitive but that type is now being proposed for use `*id` fields. Are we to assume identifiers can now collide in case sensitivity? That seems to be what was permitted before but now it's being explicitly stated? This is particularly relevant because at some point in history the Register "magically" converted all identifiers to upper case causing a number of issues for existing installations while the Regulator assumed that since they were a guid they were interchangeable.

I have changed this to be ExternalRef.

5. `iat` and other epoch times are listed as `PositiveInteger`, it's pedantic but this excludes `0` which isn't technically accurate _but_ maybe it makes sense (ie. `0` would be a schematic error condition)

I have changed this to be ExternalRef. You are right - given we can't have an iat set at 0 I had originally set it to be PositiveInteger.

6. Page number refers to `int32` but `PositiveInteger` has no upper bound (I admit it'd be pretty amazing to have 2^32 holders though!)

Do you have a suggestion on how to treat this? Noting that the pagination for the Energy and Banking APIs already uses PositiveInteger without defining an upper bound. I had aligned to these specs for consistency.

7. The type of `x-v`/`x-min-v` has changed [from string to integer](https://github.com/ConsumerDataStandardsAustralia/standards-staging/compare/release/1.20.0...maintenance/546#diff-b18f601f8cf79623521c918506f4cd50b9c4cdb4bb14a9a3f777166adf9bfa87R150) in a number of areas, this is an explicit spec change 🛑 . I note that headers are, by definition, strings _and_ these fields are specified as `string` throughout other Standards endpoints. This matters quite a bit since codegen often does casting poorly in these cases. I notice some of the occurrences [have a default specified](https://github.com/ConsumerDataStandardsAustralia/standards-staging/compare/release/1.20.0...maintenance/546#diff-b18f601f8cf79623521c918506f4cd50b9c4cdb4bb14a9a3f777166adf9bfa87R358), as a string, presumably because that's the only way to make it compliant (headers are strings and strings only).

The standards clearly state in the description for the x-v and x-min-v fields that they:

Must be set to a positive integer.

I don't see this as redefining the header or making a spec change. It is removing spec ambiguity by better representing the existing requirement.

8. Indenting. A lot of the changes aren't appropriately indented.

Thanks. I have reviewed and re-formatted the swagger files so the indentation is correct.

ACCC-CDR commented 1 year ago

The ACCC's view is as follows:

  1. While the changes seem minor from the Register's point of view, they nonetheless change the behaviour of existing endpoints (generally by constraining values to a greater extent than at present) and/or conflict with the Standards.
  2. The salient issue is whether they will impact existing implementations' interactions with the Register API. This is a question for the participants, not the ACCC.
  3. This change involves a non-zero, but hard to quantify, risk of adverse impacts on participants. This risk should be acknowledged and mitigated.
  4. Our established practice is to not change the behaviour of existing endpoint versions without a reported incident or a clear opportunity to avoid one arising; neither is apparent here.
  5. These changes should therefore be made with an accompanying increase in API versions and appropriate scheduling of obligation dates.
  6. We have highlighted issues with the proposed swagger changes below, noting that this is not intended to be an exhaustive list.
  7. Due to the risks involved in this change we feel it should be treated with the same care as a breaking change.

Swagger Change Comments:

CDR-API-Stream commented 1 year ago

In response to @ACCC-CDR:

  1. While the changes seem minor from the Register's point of view, they nonetheless change the behaviour of existing endpoints (generally by constraining values to a greater extent than at present) and/or conflict with the Standards.
  2. The salient issue is whether they will impact existing implementations' interactions with the Register API. This is a question for the participants, not the ACCC.
  3. This change involves a non-zero, but hard to quantify, risk of adverse impacts on participants. This risk should be acknowledged and mitigated.

The DSB don't believe this is the case. Certainly it isn't the intent. As stated earlier ... the proposal to remediate this issue is to modify the OAS for the Register APIs to use the common field types only if this change will not make the standards misaligned with the current ACCC implementation. If there are changes that do not align to this intent we will simply not make them in this round of changes.

In this context, specific examples of misalignment would be helpful and we will defer those changes.

  1. Our established practice is to not change the behaviour of existing endpoint versions without a reported incident or a clear opportunity to avoid one arising; neither is apparent here.

That is also the intent for this CR.

  1. These changes should therefore be made with an accompanying increase in API versions and appropriate scheduling of obligation dates.

As the specific intent is to avoid a breaking change there is no need for this.

  1. We have highlighted issues with the proposed swagger changes below, noting that this is not intended to be an exhaustive list.

Thank you. This is very helpful and we will amend the staged changes accordingly.

  1. Due to the risks involved in this change we feel it should be treated with the same care as a breaking change.

As stated, this is explicitly not the intent.

We can incorporate this one

  • x-v and x-min-v headers are changed from string to integer (PositiveInteger). This is in conflict with other OpenAPI specification files (cds_banking.json, cds_common.json, cds_energy.json)

Mark responded to this one above as it was raised by perlboy. Consistency is important so we can look at this again.

  • If-None-Match and Etag are set to "x-cds-type": "String" but should be "x-cds-type": "ASCIIString"
  • Should "issuer" be "x-cds-type": "URIString" and not "x-cds-type": "String"?

We can make those changes

  • Register OIDD should not have the following fields - id_token_signing_alg_values_supported, code_challenge_methods_supported

This change is out of scope of this CR

  • totalPages andtotalRecords - should be a NaturalNumber not a PositiveInteger (like the other openapi files)

Ok

  • Should page parameter default to 1 like the other openapi files?
  • Should page-size parameter default to 25 like the other openapi files?

Does the CDR Register currently honour these defaults. If it doesn't then we shouldn't make this change. Also, it isn't strictly in the scope of this CR

  • The values for various Enums are in conflict with the rules for the Enum type, particularly around capitalisation and spacing.

Again, we were just addressing Common Types in this CR

ACCC-CDR commented 1 year ago

The ACCC notes the DSB's intention to produce a non-breaking change. The ACCC suggests that the appropriate approach in this case is for the presumption to be that each specific alteration is, in fact, a breaking one and for the body proposing the alteration, ie the DSB, to rebut this presumption before proceeding.

In terms of specific feedback, the ACCC does not support the alterations to enums that are currently staged. We express no view on whether they are breaking or not. Per our earlier remarks, this question should be addressed by other parties. We point out that the values for the enums often do not conform to the rules in the Standards. Given that the object of the exercise is stated to be consistency, these alterations appear to be, at best, counterproductive.

We also note the non-trivial amount of time and effort that has been expended, and continues to be expended, on a change that is explicitly intended to have no material impact. On the most optimistic view of things, there will still be significant inconsistencies after this change is implemented. We suggest that the cost-benefit analysis of this change is not favourable.

nils-work commented 1 year ago

This change request was incorporated through https://github.com/ConsumerDataStandardsAustralia/standards/issues/272#issuecomment-1362187373