Skip to content

Update location-verification.yaml for Spring25#281

Merged
jlurien merged 6 commits intomainfrom
location-verification-alignment-spring25
Jan 22, 2025
Merged

Update location-verification.yaml for Spring25#281
jlurien merged 6 commits intomainfrom
location-verification-alignment-spring25

Conversation

@jlurien
Copy link
Collaborator

@jlurien jlurien commented Dec 17, 2024

What type of PR is this?

  • enhancement/feature

What this PR does / why we need it:

Updates location-verification API with the changes derived from Commonalities and ICM updates
x-camara-guidelines: 0.5.0

Which issue(s) this PR fixes:

Fixes #276, #277

Special notes for reviewers:

There are still some open discussions in Commonalities that may impact this PR, so we will keep it open until there is a final version for x-camara-guidelines: 0.5.0

Changelog input

* Error schemas updated with enums
* Some error status and codes have been updated
* Section with guidelines about how to identify the device from access token and request body has been updated

Additional documentation

@jlurien jlurien self-assigned this Dec 17, 2024
@jlurien jlurien added the Spring25 Meta-release Spring25 label Dec 17, 2024
@github-actions
Copy link

github-actions bot commented Dec 17, 2024 •
edited
Loading

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ ACTION actionlint 2 0 0.03s
✅ OPENAPI spectral 3 0 4.91s
✅ REPOSITORY git_diff yes no 0.01s
✅ REPOSITORY secretlint yes no 0.8s
✅ YAML yamllint 3 0 0.71s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

alpaycetin74
alpaycetin74 reviewed Dec 18, 2024
View reviewed changes
code/API_definitions/location-verification.yaml Outdated
message: Request not authenticated due to missing, invalid, or expired credentials.
GENERIC_401_AUTHENTICATION_REQUIRED:
summary: Generic Authentication Required
description: New authentication is needed, authentication is no longer valid
Copy link
Collaborator

@alpaycetin74 alpaycetin74 Dec 18, 2024

Choose a reason for hiding this comment

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

is it possible to explain the difference between "authentication is no longer valid" and "expired credentials" in more detail ?

Copy link
Collaborator Author

@jlurien jlurien Jan 10, 2025

Choose a reason for hiding this comment

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

There is an issue in Commonalities to discuss this: camaraproject/Commonalities#368

IMO, UNAUTHENTICATED is the most generic one, which would cover all cases where the access token is not longer valid (typically because it is expired). AUTHENTICATION_REQUIRED is more specific, when a full new authentication from scratch is needed. But we could use the generic one and rely on the message for further description.

bigludo7
bigludo7 previously approved these changes Jan 8, 2025
View reviewed changes
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM

@bigludo7 bigludo7 mentioned this pull request Jan 8, 2025
Merged
jlurien
jlurien commented Jan 14, 2025
View reviewed changes
Copy link
Collaborator Author

@jlurien jlurien left a comment

Choose a reason for hiding this comment

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

  • Test plan should be updated in the same PR

@bigludo7 bigludo7 self-requested a review January 15, 2025 16:10
@jlurien
Copy link
Collaborator Author

jlurien commented Jan 16, 2025 •
edited
Loading

@bigludo7 @maxl2287

I have updated the PR with new changes:

  • Updated the test plan. For that I have copied the fixed version of the test plan artifact in Commonalities, now still proposed as PR, but likely to be merged. We would probably need to apply as well to location-retrieval and check what applies to geofencing

  • From the spec, I have already removed 403 INVALID_TOKEN_CONTEXT, and 401 AUTHENTICATION_REQUIRED. Discussions around this are not fully closed but it seems this will be the consensus.

  • Also applied the fix on the note about networkAccessIdentifier support

bigludo7
bigludo7 reviewed Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

One small comment on the test definition else good for me.

code/Test_definitions/location-verification.feature
Scenario: None of the provided device identifiers is supported by the implementation
Given that some types of device identifiers are not supported by the implementation
And the request body property "$.device" only includes device identifiers not supported by the implementation
When the HTTP "POST" request is sent
Copy link
Collaborator

@bigludo7 bigludo7 Jan 17, 2025

Choose a reason for hiding this comment

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

We should probably add: And the header "Authorization" is set to a valid access token which does not identify a single device

Copy link
Collaborator Author

@jlurien jlurien Jan 20, 2025

Choose a reason for hiding this comment

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

Yes, you are right, but this is copied from Commonalities artifact, so I will have to update there as well

@bigludo7 bigludo7 mentioned this pull request Jan 17, 2025
Merged
@jlurien jlurien mentioned this pull request Jan 20, 2025
Merged
2 tasks
bigludo7
bigludo7 approved these changes Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM

@jlurien jlurien merged commit 03d928f into main Jan 22, 2025
1 check passed
@maxl2287 maxl2287 mentioned this pull request Jan 23, 2025
Closed
@jlurien jlurien deleted the location-verification-alignment-spring25 branch September 1, 2025 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bigludo7 bigludo7 bigludo7 approved these changes

@maxl2287 maxl2287 maxl2287 approved these changes

+1 more reviewer

@alpaycetin74 alpaycetin74 alpaycetin74 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jlurien jlurien

Labels

Spring25 Meta-release Spring25

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error Alignement with Commonalities

4 participants

@jlurien @alpaycetin74 @bigludo7 @maxl2287