-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added sequence diagrams with RFC 9101 #94
base: main
Are you sure you want to change the base?
Conversation
Hello @ECORMAC |
Hi Ludovic, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/LGTM
Hi, I've checked with my collegues from Identity and there are a few issues we are yet not sure about:
The main thing here is that, even being a Telefónica's proposal, the use of RFC9101 hasn't been defined or agreed at CAMARA level yet, so I think we should at least wait for the ICM resolution and then continue with this topic. |
I'm not sure how to move forward on this... @mhfoo @fernandopradocabrillo @ECORMAC @AxelNennker any suggestion? |
@bigludo7 The intention is to raise it again for Spring25 meta-release. see ICM issue #128 I will create the issue in ICM again, before 30 Sept and add to the following: |
@bigludo7: As Ming states think we agreed earlier to deal with this in the Spring release. |
to update this PR based on the following: ICM PR 226 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be careful with this PR ... within the proposed public release of ICM 0.3.0 is a direct link to https://github.com/camaraproject/NumberVerification/blob/main/documentation/API_documentation/assets/uml_v0.3.jpg which must be resolved there before you can proceed here with the PR (as this PR would delete the picture).
cc: @camaraproject/identity-and-consent-management_codeowners |
I'm not sure about the status of this PR. We have it for a long time and we hadn't got a consensus from the WG to merge it. I see a lot of value of this contribution as this is really good documentation but of course as mentioned by @hdamker it must be fully compliant with ICM work. |
@hdamker Fixed on ICM side. See camaraproject/IdentityAndConsentManagement#269 (comment). I think just replacing the existing link with the root path of the number verification repository would be fine IMHO. Including the image here would create dependencies between repositories (e.g. to ensure the image is up to date and so on) that I think are simply not needed in this case. |
Why do we need this PR? |
Dependency of ICM to this PR is resolved on ICM side. Up to the team here how to proceed with he PR.
Again me ... during the creation of the PR for r2.2 (#169 to resolve #168) I've seen that the API description had also a link to Within the release PR I changed the link to https://github.com/camaraproject/NumberVerification/blob/r2.2/documentation/API_documentation/assets/uml_v0.3.jpg to avoid that this link will be broken in the future. Please consider how to continue with this figure and the PR.
The YAML isn't only presented in GitHub (btw: GitHub is not rendering YAML files), but e.g. also in Redoc, Swagger or other places outside of GitHub. For that you need a link to a picture file I suppose, but if all these tools can present mermaid ... |
Suggestion to use different filenames and not overwrite the previous valid image. These sequence diagrams are to explicitly indicate that the signed request should be performed on the backend server. |
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Implement RFC 9101 for securing the /authorise endpoint.
Which issue(s) this PR fixes:
Fixes #93
Special notes for reviewers:
Please review the diagrams and provide your comments.
Changelog input
Additional documentation
This section can be blank.