erasmus-without-paper / ewp-specs-api-iias

Specifications of EWP's Interinstitutional Agreements API.
MIT License
4 stars 13 forks source link

Posible error in scenario 6 with the xslt transformation #160

Closed lsanchezpe closed 6 months ago

lsanchezpe commented 8 months ago

Testing my in progress implementation i've foud what i think is an error in the definded flows of the exchanging and aprproving a new IIA.

If we put the focus in the 6th scenario of the officially published scenarios we can found a first part that describes a desired scenario:

  1. A creates an IIA_A
  2. B recives IIA_A and agreed, so creates and send a identical copy (IIA_B)
  3. And approves the received IIA_A (approving rigth after sending its copy).

This work as a charm in v6 with the cooperation-condition-hash calculated just with the cooperation condition info. But with the new hash calculation based in xslt transformation we have found a problem, as the xslt transformation works with the IIA_IDs

A B
A creates revision A0 (Create IIA) with A.iia_id (local) mapped identifiers: A.iia_id (local), - (remote) last known hashes: A0.hash0, - approval hashes: -, -
A sends IIA CNR with A.iia_id (local)
B receives IIA CNR with A.iia_id (remote)
B sends IIA GET with A.iia_id (remote), receives in response A0 (A.iia_id, -, A0.hash0)
B likes the proposed IIA and creates revision B0 identical to A0 with B.iia_id (local) mapped identifiers: B.iia_id (local), A.iia_id (remote) last known hashes: B0.hash0, A0.hash0 approval hashes: -, -
B sends IIA CNR with B.iia_id (local)
A receives IIA CNR with B.iia_id (remote)
A sends IIA GET with B._iia_id (remote), receives in response B0 (A.iia_id, B.iia_id, B0.hash0) mapped identifiers: A.iia_id (local), B.iia_id (remote) last known hashes: A0.hash0*, B0.hash0 approval hashes: -, -
B approves A's copy - sends IIA Approval CNR with A.iia_id (remote) IIA Approval CNR may be sent right after B's IIA CNR. mapped identifiers: B.iia_id (local), A.iia_id (remote) last known hashes: B0.hash0, A0.hash0 approval hashes: -, A0.hash0**

My problem come with the hashes marked as bold:

¿How this situation its supposed to be managed? ¿A should send a CNR before B sends the approval? so then the approval cnr can't be send rigth after the CNR

fioravanti-unibo commented 8 months ago

I think that the proposed scenario is not entirely correct: B should not approve the agreement A0 before it is mutually mapped to B0.

The change in IIAHash when the mapping is created/changed is a feature explicitly documented.

In my opinion, a more correct scenario is:

demilatof commented 8 months ago

think that the proposed scenario is not entirely correct: B should not approve the agreement A0 before it is mutually mapped to B0.

The change in IIAHash when the mapping is created/changed is a feature explicitly documented.

Exactly; I add that before approving A's IIA, B MUST invoke IIA-Get to fetch IIA from A and check that the hash code is the same. In this case it is different, therefore B has to interrupt the approving process

lsanchezpe commented 8 months ago

@fioravanti-unibo, @demilatof Appreciates the responses.

Would you please refer me where can i find this explicitly documented feature? I try to find it but the table i put in my original post is a copy from the official scenario where B didnt waits till this CNR from A.

We also fetch IIA from partner before sending the approval CNR and check that is the same that we have stored from previous invocations, in order to not approve things that are not the same what we are seeing. This the principal reason of my problem, If A didn't send me this previous CNR i wont be able to approve his IIA

fioravanti-unibo commented 8 months ago

the problem of having the mapping of the agreements before the approval is a leit motif of almost any Infrastructure Forum and is discussed in many issues.

BTW, in the XSD of the iia-Get response is stated:

Also, it is REQUIRED to provide partner agreement ID to be able to approve it. Yet, this ID MUST NOT be present if the partner has deleted the IIA.

see:


                                        <xs:element name="iia-id" type="ewp:AsciiPrintableIdentifier" minOccurs="0" maxOccurs="1">
                                            <xs:annotation>
                                                <xs:documentation>
                                                    The partner's unique surrogate ID of this IIA. This is a surrogate ID, so it
                                                    SHOULD NOT be displayed to the user (use `iia-code` for that).

                                                    Since IIA IDs are local (unique within a single HEI, but not within the world),
                                                    each partner is allowed to have his own ID for the same IIA.

                                                    ID used by the partner MUST be provided as soon as it is mapped in the local system.
                                                    Otherwise, an agreement might be interpreted as not mapped, which may lead to duplicates.
                                                    Also, it is REQUIRED to provide partner agreement ID to be able to approve it.
                                                    Yet, this ID MUST NOT be present if the partner has deleted the IIA.

                                                    Server implementers MUST use immutable surrogate keys for their work with EWP.
                                                    https://github.com/erasmus-without-paper/ewp-specs-architecture#ids
                                                </xs:documentation>
                                            </xs:annotation>
                                        </xs:element>

Link

lsanchezpe commented 8 months ago

Thanks for the calrification, i was looking for it in the description of the hash field and in the specification of the approval api.

So in conclussion, the scenario is incorrect as B can't send the approval untill A sends its CNR with its relation

fioravanti-unibo commented 8 months ago

you are welcome, unfortunately we have noticed that sometimes the scenarios are quite imprecise or only handle relatively simple cases.

janinamincer-daszkiewicz commented 8 months ago

This work as a charm in v6 with the cooperation-condition-hash calculated just with the cooperation condition info. But with the new hash calculation based in xslt transformation we have found a problem, as the xslt transformation works with the IIA_IDs

That's true. The sequence of scenarios was created under the idea that 1-7 are for v6.3.0 (as Modify, Delete have been introduced in v6.3.0), in scenario 8 we switch from v6 to v7, scenario 9 is for 7 after transition from 6. Sorry if this intention was not clear.

before approving A's IIA, B MUST invoke IIA-Get to fetch IIA from A and check that the hash code is the same. In this case it is different, therefore B has to interrupt the approving process

I agree that B should invoke IIA get before IIA Approval CNR and make sure that the hash has not changed. In v6.3.0 hash will not change but the check has to be made anyway. I will add this step to the scenario to make it explicit (to all scenarios where there is Approval CNR).

More tricky is the question what should B do if hash is the same, but A has not yet saved B's id in his copy. B knows that on his side there is proper mapping. B knows that he has sent his id to A. B knows that A must not approve B's copy before getting and storing B's id. B can sent Approval CNR to A.

Saying that I will conclude that because v6.3.0 will only live during a transient period before switching to v7, I agree that there is more value in having scenarios for Modify and Delete in v7. I will prepare all those scenarios for version 7.

P.S. @lsanchezpe thank you for rasing this issue.

umesh-qs commented 8 months ago

Thanks for the calrification, i was looking for it in the description of the hash field and in the specification of the approval api.

So in conclussion, the scenario is incorrect as B can't send the approval untill A sends its CNR with its relation

CNR cannot be bound to any process. These are ideal scenario which assumes that CNR works 100%. We do not depend on CNR. Better to do a IIA GET before processing any approvals.

lsanchezpe commented 8 months ago

I agree that B should invoke IIA get before IIA Approval CNR and make sure that the hash has not changed. In v6.3.0 hash will not change but the check has to be made anyway.

I think this can be clarified in the specification of the iia-approval-response.xsd, even if its a basic i think this point can be quite confusing. There we can found:

The digest is present in the IIAs API get response, but MUST be verified by the server before sending the approval. For this purpose, the server has to call the IIAs get API and compare the hash received in the response with the hash independently calculated from that response. If both hashes are identical, the agreement can be approved.

But in the case we are discussing in my first post, at the point marked with **, A will respond A0.hash1 and if B checks this hash with the hash independently calculated from that response both will be identical but different with the previous hash received so there are two different checks.

Regarding this:

More tricky is the question what should B do if hash is the same, but A has not yet saved B's id in his copy. B knows that on his side there is proper mapping. B knows that he has sent his id to A. B knows that A must not approve B's copy before getting and storing B's id. B can sent Approval CNR to A.

We're planning to add an additional check in the validations associated to the previous get and check if A0 hash both IIA_IDs. If thats the case and the hashes pass the validations, we will proced whit the approval CNR; in other case, the appoval process will be interrupted.

janinamincer-daszkiewicz commented 7 months ago

That's true. The sequence of scenarios was created under the idea that 1-7 are for v6.3.0 (as Modify, Delete have been introduced in v6.3.0), in scenario 8 we switch from v6 to v7, scenario 9 is for 7 after transition from 6. Sorry if this intention was not clear. Saying that I will conclude that because v6.3.0 will only live during a transient period before switching to v7, I agree that there is more value in having scenarios for Modify and Delete in v7. I will prepare all those scenarios for version 7.

This is the extended set of scenarios. The first TAB contains change log listing changes as compared to DeleteModifyScenarios from 2023-20-28. Please, review.

Also, please, tell me if we need any other specific scenarios.
lsanchezpe commented 7 months ago

I think you may uploaded the wrong file. As far as I can see, the content of this file is the same of the already plublished on 2023-10-28.

janinamincer-daszkiewicz commented 7 months ago

Ups, you are right. I had two files in the same folder and uploaded the old one. Now the right one comes. 2024-01-20-DeleteModifyScenarios.xlsx