Skip to content
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

Update yaml and sync with commonalities #60

Conversation

fernandopradocabrillo
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • correction

What this PR does / why we need it:

  1. Align securitySchemes with openID as agreed in I&C
  2. Format scopes correctly according to Commonalities

Which issue(s) this PR fixes:

Fixes (#49) Defines CAMARA scope for the CAMARA version

Special notes for reviewers:

N/A

Changelog input

N/A

Additional documentation

N/A

Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the X-Correlator header need to be defined within the OAS file. At least that is my understanding of the Design Guidelines and I haven't seen it yet in other sub projects.

@bigludo7
Copy link
Collaborator

@hdamker As commented in the issue 49 we have this x-correlator present in number verification API . As we have developer implementing both API seems to me important to have some consistency between them.

@fernandopradocabrillo
Copy link
Collaborator Author

@hdamker I agree with you, the guidelines should be less ambiguous on this issue but, as @bigludo7 said, I also think it is important to have consistency and I prefer it to be defined in the OAS file as we have in the different APIs:

@bigludo7
Copy link
Collaborator

@fernandopradocabrillo Probably we have to wait for camaraproject/Commonalities#57 green light before to merge this one - correct?

@hdamker do you think adding x-Correlator is a blocking issue for this PR?

@ECORMAC
Copy link
Collaborator

ECORMAC commented Sep 19, 2023

When we state to include OpenID schema what exactly are we proposing (e.g. parts of the standard defined schema, all of it?)

@fernandopradocabrillo
Copy link
Collaborator Author

@fernandopradocabrillo Probably we have to wait for camaraproject/Commonalities#57 green light before to merge this one - correct?

@bigludo7 Yes, also think we have to wait.

When we state to include OpenID schema what exactly are we proposing (e.g. parts of the standard defined schema, all of it?)

@ECORMAC the proposal is to use OIDC with a 3-legged grant flow. But no mention to further requirements like id_tokens at the moment.

@hdamker
Copy link
Contributor

hdamker commented Sep 25, 2023

@hdamker do you think adding x-Correlator is a blocking issue for this PR?

I wouldn't call it blocking, but we could carve it out here until clarified in Commonalities. I still think that it is just cluttering the OAS.

@@ -70,8 +70,8 @@ paths:
summary: Verifies if the received hashed/plain text phone number matches the phone number associated with the access token
description: |
Verifies if the specified phone number (either in plain text or hashed format) matches the one that the user is currently using. Only one of the plain or hashed formats must be provided.
- The number verification will be done for the user that has authenticated via mobile network and so their `sub` is in the access token
- It returns true/false depending on if the hashed phone number received as input matches the authenticated user's `device phone number` associated to the access token
- The number verification will be done for the user that has authenticated via mobile network and so their `sub` is in the access token
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we use "OpenID Connect" sub is one of id_token claims.

@fernandopradocabrillo please review

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it depends...

following RFC:

Access tokens can have different formats, structures, and methods of
   utilization (e.g., cryptographic properties) based on the resource
   server security requirements.  Access token attributes and the
   methods used to access protected resources are beyond the scope of
   this specification and are defined by companion specifications such
   as RFC6750.

RFC6750
Access Token

and sub required claim inside ID Token, that is something specific for OpenId Connect, as it is designed for authentication (& authorization).
ID Token

So you can state that:

sub is build in OpenID Connect, as it is a part of id_token, and is optional for OAUTH2.0 as it depends on resource server security requirements.

Therefore, if we're willing to use access_tokens we should use OAUTH2.0 security scheme.

/device-phone-number:
get:
tags:
- Phone number share
summary: Returns the phone number associated with the access token
description: |
Returns the phone number so the API clients can verify the number themselves:
- It will be done for the user that has authenticated via mobile network and so their `sub` is in the access token
- It returns the authenticated user's `device phone number` associated to the access token
- It will be done for the user that has authenticated via mobile network and so their `sub` is in the access token
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

@ECORMAC
Copy link
Collaborator

ECORMAC commented Oct 16, 2023

Agree with David, "sub" is defined in relation to identity tokens (can't find it being mentioned in any spec in relation to access tokens). The current description in the Yaml is confusing, it should be clarified what we are proposing.

@fernandopradocabrillo
Copy link
Collaborator Author

@DT-DawidWroblewski
As discussed in the meeting I have removed references to the sub in the access token

Copy link
Collaborator

@DT-DawidWroblewski DT-DawidWroblewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DT-DawidWroblewski DT-DawidWroblewski merged commit 801a728 into camaraproject:main Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants