ConsumerDataStandardsAustralia / standards

Work space for data standards development in Australia under the Consumer Data Right regime
Other
321 stars 56 forks source link

Decision Proposal 73 - May Draft Feedback Cycle 2 #73

Closed CDR-API-Stream closed 5 years ago

CDR-API-Stream commented 5 years ago

This thread has been created to allow for continuing feedback across the API standards and Information Security profile. All feedback is welcome.

WestpacOpenBanking commented 5 years ago

Although the feedback period for the May is now closed, we have the comments and endorsements in relation to feedback given by other stakeholders:

Westpac shares ANZ’s expectation that FAPI headers would not be expected for product API calls. We endorse CBA’s remarks on Product Bundle Representation, Pagination and Bulk APIs. We also share concerns, noted in earlier rounds of feedback, in relation to PII & Third-Party Private Data.

Westpac agrees with NAB that the InfoSec profile should have an independent security review. We also note that the InfoSec profile has dependencies, and that there will be future need for alignment with the CX recommendations and the ACCC directory design. Westpac supports ANZ’s comments in relation to dynamic client registration. We support CBA’s comments regarding the design and functionality of the registry. We endorse CBA’s comments relating to a consent API and requesting sharing duration. As suggested by Ralph Bragg, we agree that Australia should align with FAPI standards where possible. We believe that the Australian consumer data right regime would benefit from Consumer Data Standards Australia engagement in international standards development processes including for FAPI.

WestpacOpenBanking commented 5 years ago

We suggest that availableBalance in BankingBalance is made optional rather than mandatory. This is because our term deposits do not have a concept of an available balance. Inputting ‘0’ in this case would misrepresent this fact and might be erroneously interpreted to mean that the available balance may no longer be zero at some future date.

perlboy commented 5 years ago

Within BankingScheduledPaymentInterval dayInInterval and BankingScheduledPaymentRecurrenceLastWeekday lastWeekDay it is stated that The first day of a week is considered to be Sunday.

dayInInterval is specified as a duration in ISO-8601 format. ISO-8601 specifies that the first day of the week is Monday and therefore the Standards guidance is contradictory.

Further, BankingScheduledPaymentRecurrenceLastWeekday and BankingScheduledPaymentInterval appear to have similar purposes but use different data types (Duration vs. Integer) when specifying a day of week.

Finally, the BankingScheduledPayment* namespaces appear to have shared attribues in sub objects (eg. paymentsRemaining) rather than following an attribute structure similar to Product info's use of additionalValue and additionalInfo.

perlboy commented 5 years ago

BankingScheduledPayment specifies an attribute called paymentSet which implies one scheduled payment with multiple split destinations. The specification is a single value of BankingScheduledPaymentSet which is not a multidimensional array.

Should paymentSet be an array of BankingScheduledPaymentPart?

perlboy commented 5 years ago

BankingTransactionDetail, specifically it's sub-object BankingTransactionDetailExtendedData do not appear to follow existing convention for other definitions notably the use of uType does not reference a type specific object (as is the case in BankingPayee and others) but instead overloads a single object with multiple text fields.

In addition, while not publicly available the NPPA does have a Swagger contract available that specifically describes the 4 message types (which are ISO20022 derivatives) transiting the NPP. Of relevance here is pain.a09.001.01 which would provide a 1:1 overlay into the CDR for NPP payloads.

perlboy commented 5 years ago

CommonPAF specifies that it is in AusPost PAF file format but it is missing FLAT_UNIT_TYPE and FLOOR_LEVEL_TYPE

perlboy commented 5 years ago

The declaration for CommonPhysicalAddressWithPurpose misorders the allOf causing code generation to break and not extend (compared with CommonOrganisationDetail). This is known and documented within swagger-codegen issues. Anonymous object should be described last and as per guidance from SmartBear there should be only one of these in OAI2 payloads. FWIW OAI3 solves this much more elegantly with polymorphism and discriminator.

Working:

"CommonOrganisationDetail" : {
  "allOf" : [ {
    "$ref" : "#/definitions/CommonOrganisation"
  }, {
    "type" : "object",
    "required" : [ "physicalAddresses" ],
    "properties" : {
      "physicalAddresses" : {
        "type" : "array",
        "description" : "Must contain at least one address. One and only one address may have the purpose of REGISTERED. Zero or one, and no more than one, record may have the purpose of MAIL. If zero then the REGISTERED address is to be used for mail",
        "items" : {
          "$ref" : "#/definitions/CommonPhysicalAddressWithPurpose"
        }
      }
    }
  } ]
},

Broken:

"CommonPhysicalAddressWithPurpose" : {
  "allOf" : [ {
    "type" : "object",
    "required" : [ "purpose" ],
    "properties" : {
      "purpose" : {
        "type" : "string",
        "description" : "Enumeration of values indicating the purpose of the physical address",
        "enum" : [ "REGISTERED", "MAIL", "PHYSICAL", "WORK", "OTHER" ]
      }
    }
  }, {
    "$ref" : "#/definitions/CommonPhysicalAddress"
  } ]
},
perlboy commented 5 years ago

DiscoveryOutage uses a number of minutes for duration while all other parts of the Standards use ISO-8601 durations. There is little reason to not make this consistent.

ResponseBankingAccountsBalanceById is missing meta

ResponseBankingAccountsBalanceById uses LinksPaginated when it is not a paginated response

ResponseErrorList_errors is not a container for a payload but rather a payload itself and should be renamed to CommonErrorDetail

ResponseDiscoveryStatusData is not a container for a payload but rather a payload itself and should be renamed to CommonDiscoveryStatus

ResponseDiscoveryStatus does not allow for multiple outages to be in progress at the same time. Suggest ResponseDiscoveryStatusData becomes an array of CommonDiscoveryStatus as per DiscoveryOutage

ResponseDiscoveryOutages should be renamed to ResponseDiscoveryOutageList in line with other responses

DiscoveryOutage should be renamed to CommonDiscoveryOutage

ResponseCommonCustomerDetail_data is not a container for a payload but rather a payload itself and should be renamed to CommonCustomerDetail

ResponseCommonCustomer_data is not a container for a payload but rather a payload itself and should be renamed to CommonCustomer

anzbankau commented 5 years ago

Hi James,

As per the 'Additional Pagination Rules' at:

https://consumerdatastandardsaustralia.github.io/standards/#pagination

There is a rule that: A maximum page size of 1000 records is assumed for all end points (unless otherwise stipulated in the end point definition). If a page size greater than this maximum is requested then a HTTP status of 422 Unprocessable Entity SHOULD be returned.

However currently the HTTP response code table states a 422 is not applicable to a GET:

https://consumerdatastandardsaustralia.github.io/standards/#http-response-codes

Could this please be updated.

Thanks.

perlboy commented 5 years ago

We suggest that availableBalance in BankingBalance is made optional rather than mandatory. This is because our term deposits do not have a concept of an available balance. Inputting ‘0’ in this case would misrepresent this fact and might be erroneously interpreted to mean that the available balance may no longer be zero at some future date.

@WestpacOpenBanking isn't this the difference between availableBalance and currentBalance? That is to say a term deposit would have a currentBalance equivalent to the account balance of the term deposit and availableBalance would be 0 until such time as the term deposit term is reached or the consumer elects to terminate early? Allowing the entry to be null would mean that recipients would likely present it as Unknown or Unavailable or equivalent which would seem just as (if not more) misleading...

CDR-API-Stream commented 5 years ago

In a separate issue @rfc6919 provided the following feedback which is reposted here:

https://consumerdatastandardsaustralia.github.io/standards/?shell#requirements-for-data-recipient-implementations

"Revocation of Access Tokens MUST be not be supported"

CDR-API-Stream commented 5 years ago

In a separate issue @rfc6919 provided the following feedback which is reposted here:

per the v0.9.3 spec, if an API provider is not capable of servicing the API version specified in a client x-v header, but is capable of servicing a version >= than requested in x-min-v, it still must return 406 Not Acceptable. presumably the description for x-v should be updated to allow service of versions less than requested, as long as x-min-v is satisfied.

CDR-API-Stream commented 5 years ago

In response to the feedback from @WestpacOpenBanking :

We suggest that availableBalance in BankingBalance is made optional rather than mandatory. This is because our term deposits do not have a concept of an available balance. Inputting ‘0’ in this case would misrepresent this fact and might be erroneously interpreted to mean that the available balance may no longer be zero at some future date.

The specific intent for this fields is that it represents the available balance for withdrawal at the time of the call. In the scenario described a value of '0' is appropriate. Due to the existence of a the currentBalance field this is not expected to be misleading. If it is perceived to be misleading this would be better dealt with in the description of the field rather than by making the field optional.

CDR-API-Stream commented 5 years ago

In response to @perlboy

Within BankingScheduledPaymentInterval dayInInterval and BankingScheduledPaymentRecurrenceLastWeekday lastWeekDay it is stated that The first day of a week is considered to be Sunday.

dayInInterval is specified as a duration in ISO-8601 format. ISO-8601 specifies that the first day of the week is Monday and therefore the Standards guidance is contradictory.

This is a good pickup. We will address this as a defect and align to the ISO standard's usage of first day as Monday.

Further, BankingScheduledPaymentRecurrenceLastWeekday and BankingScheduledPaymentInterval appear to have similar purposes but use different data types (Duration vs. Integer) when specifying a day of week.

This variation is deliberate. The interval in BankingScheduledPaymentInterval can potentially be many days so the ability to represent this interval in terms on years, months or days is more appropriate. As lastWeekDay is constrained to days in the week an integer was seen as more applicable than using a duration field and limiting the value to exclude the majority of the duration syntax.

Finally, the BankingScheduledPayment* namespaces appear to have shared attribues in sub objects (eg. paymentsRemaining) rather than following an attribute structure similar to Product info's use of additionalValue and additionalInfo.

The preference in this case is to be strongly typed rather than use the more generic model used in product reference data. These objects may initially be similar but, as they represent different conceptual objects, they may diverge over time. As such dedicated structures for each interval type have been defined.

CDR-API-Stream commented 5 years ago

In response to @perlboy

BankingScheduledPayment specifies an attribute called paymentSet which implies one scheduled payment with multiple split destinations. The specification is a single value of BankingScheduledPaymentSet which is not a multidimensional array.

Should paymentSet be an array of BankingScheduledPaymentPart?

Yes the paymentSet field should be array. Thanks for highlighting this.

CDR-API-Stream commented 5 years ago

In response to @perlboy

BankingTransactionDetail, specifically it's sub-object BankingTransactionDetailExtendedData do not appear to follow existing convention for other definitions notably the use of uType does not reference a type specific object (as is the case in BankingPayee and others) but instead overloads a single object with multiple text fields.

The uType convention provides reference to a JSON field. In most cases this is an object but it is not necessarily always the case. In this case it is referencing only a single field but was included because it is expected that new types will be added in the future as new NPP overlay services become available.

In addition, while not publicly available the NPPA does have a Swagger contract available that specifically describes the 4 message types (which are ISO20022 derivatives) transiting the NPP. Of relevance here is pain.a09.001.01 which would provide a 1:1 overlay into the CDR for NPP payloads.

As the NPPA API framework is for payment initiation not all fields are consequently available in transaction history (once the payment is complete). Previous feedback to the May draft from various stakeholder has, however, highlighted a number of fields that can be added and these are being considered.

CDR-API-Stream commented 5 years ago

In response to @perlboy

CommonPAF specifies that it is in AusPost PAF file format but it is missing FLAT_UNIT_TYPE and FLOOR_LEVEL_TYPE

This will be investigated.

CDR-API-Stream commented 5 years ago

In response to @perlboy, regarding the misordering of allof in swagger 2. This will be addressed for CommonPhysicalAddressWithPurpose as a defect noting that it doesn't technically alter the standard.

CDR-API-Stream commented 5 years ago

In response to @perlboy

DiscoveryOutage uses a number of minutes for duration while all other parts of the Standards use ISO-8601 durations. There is little reason to not make this consistent.

ResponseBankingAccountsBalanceById is missing meta

ResponseBankingAccountsBalanceById uses LinksPaginated when it is not a paginated response

ResponseErrorList_errors is not a container for a payload but rather a payload itself and should be renamed to CommonErrorDetail

ResponseDiscoveryStatusData is not a container for a payload but rather a payload itself and should be renamed to CommonDiscoveryStatus

ResponseDiscoveryStatus does not allow for multiple outages to be in progress at the same time. Suggest ResponseDiscoveryStatusData becomes an array of CommonDiscoveryStatus as per DiscoveryOutage

ResponseDiscoveryOutages should be renamed to ResponseDiscoveryOutageList in line with other responses

DiscoveryOutage should be renamed to CommonDiscoveryOutage

ResponseCommonCustomerDetail_data is not a container for a payload but rather a payload itself and should be renamed to CommonCustomerDetail

ResponseCommonCustomer_data is not a container for a payload but rather a payload itself and should be renamed to CommonCustomer

Thanks for the detailed review. Most of this is stylistic and will be incorporated. The outage duration will be considered but seems reasonable.

CDR-API-Stream commented 5 years ago

In response to @anzbankau

As per the 'Additional Pagination Rules' at:

https://consumerdatastandardsaustralia.github.io/standards/#pagination

There is a rule that: A maximum page size of 1000 records is assumed for all end points (unless otherwise stipulated in the end point definition). If a page size greater than this maximum is requested then a HTTP status of 422 Unprocessable Entity SHOULD be returned.

However currently the HTTP response code table states a 422 is not applicable to a GET:

https://consumerdatastandardsaustralia.github.io/standards/#http-response-codes

Could this please be updated.

The response code table will be updated as suggested.

perlboy commented 5 years ago

Further, BankingScheduledPaymentRecurrenceLastWeekday and BankingScheduledPaymentInterval appear to have similar purposes but use different data types (Duration vs. Integer) when specifying a day of week. This variation is deliberate. The interval in BankingScheduledPaymentInterval can potentially be many days so the ability to represent this interval in terms on years, months or days is more appropriate. As lastWeekDay is constrained to days in the week an integer was seen as more applicable than using a duration field and limiting the value to exclude the majority of the duration syntax.

If lastWeekDay is intended to be a day of week then the least confusing method would be to declare and reuse a DayOfWeek enumeration which strongly orders the days of week in line with ISO-8601.

Regarding dayInInterval in BankingScheduledPaymentInterval, is this intended to be an arbitrary number of days for the interval period? Ie. If BankingScheduledPaymentInterval represents a monthly payment but the payment is to be conducted on the 5th day of every month interval=P1M and dayInInterval=5 ?

ISO-8601 allows for bounded durations specified using a / separator. Additionally the standard allows for recurring time intervals to be specified using a prefix of R/#. Are these variations intended to be supported?

Finally, the BankingScheduledPayment* namespaces appear to have shared attribues in sub objects (eg. paymentsRemaining) rather than following an attribute structure similar to Product info's use of additionalValue and additionalInfo. The preference in this case is to be strongly typed rather than use the more generic model used in product reference data. These objects may initially be similar but, as they represent different conceptual objects, they may diverge over time. As such dedicated structures for each interval type have been defined.

As per above ISO-8601 allows for bounded timespans and repetition. Since this is facilitated in ISO-8601 BankingScheduledPaymentRecurrenceOnceOff, BankingScheduledPaymentRecurrenceLastWeekday and BankingScheduledPaymentRecurrenceIntervalSchedule all appear to be able to be represented in a single unified object.

Nonetheless, specifically on BankingScheduledPaymentRecurrenceOnceOff there doesn't appear to be a facilitation for nonBusinessDayTreatment. On BankingScheduledPaymentRecurrenceIntervalSchedule the finalPaymentDate could be derived from a bounded ISO-8601 Duration. On BankingScheduledPaymentRecurrenceLastWeekdaythere doesn't appear to be a facilitation for nonBusinessDayTreatment (ie. when the weekday is set to Monday and it's a public holiday).

The uType convention provides reference to a JSON field. In most cases this is an object but it is not necessarily always the case. In this case it is referencing only a single field but was included because it is expected that new types will be added in the future as new NPP overlay services become available.

My feedback has been misunderstood. There is a high probability that there will be additional transaction details that are divergent. In the current proposal extendedData and, on further review BankingTransaction effectively contains all possible payment attributes which inevitably will lead to a sparsely populated and poorly structured object. This means that a developer will need to perform picklist matching to determine the acquisition channel rather than be able to determine that acquisition channel via a strongly typed subobject.

On BankingTransaction, overloading of the base object is occurring rather than using the uType convention followed elsewhere. Specifically of note is that BPAY payments (a trademarked and commercially attached payment provider) have been granted exclusivity over generically named attributes notably billerCode, billerName and crn. This precludes an alternate payments provider using these global attributes and arguably provides a commercial advantage to the incumbent. Additionally reference described as The reference for the transaction provided by the originating institution. Empty string if no data provided appears to be overloaded as it may be used by various origination sources.

BankingTransactionDetail does not appear to sufficiently allow for attributes of even existing payment types such as:

As can be seen through the collapsing of these structures there is an increasing amount of fuzziness for a recipient developer attempting to clearly identify and present transaction information. Such different views are a regular use case within existing internet banking platforms. That is to say that BPAY transactions usually have a different presentation view than direct transfers which have a different presentation view to those which utilise card providers such as VISA or Mastercard.

Pseudo coding an alternate JSON structure for BankingTransaction:

    "BankingTransaction" : {
      "type" : "object",
      "required" : [ "accountId", "amount", "description", "isDetailAvailable", "reference", "status", "type" ],
      "properties" : {
        "accountId" : {
          "type" : "string",
          "description" : "ID of the account for which transactions are provided",
          "x-cds-type" : "ASCIIString"
        },
        "transactionId" : {
          "type" : "string",
          "description" : "A unique ID of the transaction adhering to the standards for ID permanence.  This is mandatory (through hashing if necessary) unless there are specific and justifiable technical reasons why a transaction cannot be uniquely identified for a particular account type",
          "x-cds-type" : "ASCIIString"
        },
        "isDetailAvailable" : {
          "type" : "boolean",
          "description" : "True if extended information is available using the transaction detail end point. False if extended data is not available",
          "x-cds-type" : "Boolean"
        },
        "holderType" : {
          "type" : "string",
          "description" : "The type of the transaction within the Holders sytems",
          "enum" : [ "FEE", "INTEREST_CHARGED", "INTEREST_PAID", "TRANSFER_OUTGOING", "TRANSFER_INCOMING", "PAYMENT", "DIRECT_DEBIT", "OTHER" ]
        },
        "status" : {
          "type" : "string",
          "description" : "Status of the transaction whether pending or posted. Note that there is currently no provision in the standards to guarantee the ability to correlate a pending transaction with an associated posted transaction",
          "enum" : [ "PENDING", "POSTED" ]
        },
        "description" : {
          "type" : "string",
          "description" : "The transaction description as applied by the financial institution"
        },
        "postingDateTime" : {
          "type" : "string",
          "description" : "The time the transaction was posted. This field is Mandatory if the transaction has status POSTED.  This is the time that appears on a standard statement",
          "x-cds-type" : "DateTimeString"
        },
        "valueDateTime" : {
          "type" : "string",
          "description" : "Date and time at which assets become available to the account owner in case of a credit entry, or cease to be available to the account owner in case of a debit transaction entry",
          "x-cds-type" : "DateTimeString"
        },
        "executionDateTime" : {
          "type" : "string",
          "description" : "The time the transaction was executed by the originating customer, if available",
          "x-cds-type" : "DateTimeString"
        },
        "amount" : {
          "type" : "string",
          "description" : "The value of the transaction. Negative values mean money was outgoing from the account",
          "x-cds-type" : "AmountString"
        },
        "currency" : {
          "type" : "string",
          "description" : "The currency for the transaction amount. AUD assumed if not present",
          "x-cds-type" : "CurrencyString"
        },
        "reference" : {
          "type" : "string",
          "description" : "The reference for the transaction provided by the originating institution.  Empty string if no data provided"
        },
        "transactionUType" : {
            "type" : "string",
            "description" : "Type of object included that describes the transaction, reused for TransactionDetail",
            "enum" : [ "BPAY", "PAYID", "WIRE", "DIRECT" ]
          },
          "BPAY" : {
            "$ref" : "#/definitions/BankingTransactionBPAY"
          },
          "PAYID" : {
            "$ref" : "#/definitions/BankingTransactionPayID"
          },
          "WIRE" : {
            "$ref" : "#/definitions/BankingTransactionWire"
          }
          "DIRECT" : {
            "$ref" : "#/definitions/BankingTransactionDirect"
          }
        },

    },

Pseudo coding a JSON structure for BankingTransactionDetail:

    "BankingTransactionDetail" : {
      "allOf" : [ {
        "$ref" : "#/definitions/BankingTransaction"
      }, {
        "type" : "object",
        "required" : [ "transactionUType" ],
        "properties" : {
          "BPAY" : {
            "$ref" : "#/definitions/BankingTransactionDetailBPAY"
          },
          "PAYID" : {
            "$ref" : "#/definitions/BankingTransactionDetailPayID"
          },
          "WIRE" : {
            "$ref" : "#/definitions/BankingTransactionDetailWire"
          }
          "DIRECT" : {
            "$ref" : "#/definitions/BankingTransactionDetailDirect"
          }
        },
        "x-conditional" : [ "WIRE", "BPAY", "WIRE", "DIRECT" ]
      } ]
    }

This structure allows BankingTransactionDetail* to extend the non detail version of the object while rewriting it within the detailed definition. Additionally this structure allows for non-standard payment types to be adopted by banks (for instance an Apple Pay integration) without polluting the namespace. Finally this structure makes transitioning to polymorphism available in OAI3 using the uType as the discriminator.

Thanks for the detailed review. Most of this is stylistic and will be incorporated.

Repeating one of the defined principles: Principle 4: APIs provide a good developer experience Stylistic from the Standards perspective is object pollution for developers using available industry tools. I would suggest that perhaps Standards should consider running swagger-codegen on their specification as a matter of course.

The response code table will be updated as suggested.

Regarding @anzbankau's comment and your response regarding the return of 422 Unprocessable Entity for invalid pagination inputs. This is divergent from the UK OpenBanking specification which states that 400 Bad Request is to be used in the situation defined as Request has malformed, missing or non-compliant JSON body, URL parameters or header fields.. In fact error code 422, defined in HTTP 1.1+, is entirely missing from the UK Standards. Instead the UK Standards prefer a HTTP 1.0 400 Bad Request and this convention is utilised within Engineering artefacts (following clarification at the time with Standards). Is this position now changing?

Thanks.

assem-ali commented 5 years ago

Hi @JamesMBligh , My question is in regards to this part of the header attributes:

x-fapi-customer-ip-address The customer's original IP address if the customer is currently logged in to the data recipient. The presence of this header indicates that the API is being called in a customer present context. Not required for unattended or unauthenticated calls. Conditional
x-cds-User-Agent The customer's original User Agent header if the customer is currently logged in to the data recipient. Mandatory for customer present calls. Not required for unattended or unauthenticated calls. Conditional

In relation to the statement "Not required for unattended or unauthenticated calls", is the intent to say "must not be provided if the customer is not present"? To use the ip-address as an indicator for the customer presence, the above fields must not provided otherwise. do you agree?

CDR-API-Stream commented 5 years ago

In response to @perlboy

These are good suggestions but would require some dialogue. We are in the process of working out the ongoing maintenance operating model and will add these to the backlog once it has been determined.

CDR-API-Stream commented 5 years ago

In response to @assem-ali

In relation to the statement "Not required for unattended or unauthenticated calls", is the intent to say "must not be provided if the customer is not present"? To use the ip-address as an indicator for the customer presence, the above fields must not provided otherwise. do you agree?

The comment is intended to indicate that there is no concept of customer present or not present for the unauthenticated end points as there is no customer authentication or even client authentication to access them.

These headers are therefore only expected to be present for secure end points called in a customer present context.

CDR-API-Stream commented 5 years ago

This thread is being closed. Further comments should be added to issue #79.