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.

Sign up for GitHub

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

Jump to bottom

Handle API authentication errors for the API services #1034

Merged
merged 4 commits into from
Jan 16, 2025
Merged

Handle API authentication errors for the API services #1034

merged 4 commits into from
Jan 16, 2025

Conversation

arahmarchak
Copy link
Contributor

@arahmarchak arahmarchak commented Dec 11, 2024 •
edited
Loading

What this PR does / why we need it:

  • Expand the fallback_controller with some auth_error functions
  • Add additional error views with useful feedback messages
  • Add the gen_jwt_token_with_wrong_signature function in the JWTTestHelper
  • Add the necessary auth tests and adapt existing ones
  • Add new errors to the OpenAPI schema of every app

Which issue(s) this PR fixes:

Fixes: #1031

Special notes for your reviewer:

Does this PR introduce a user-facing change?
  • Yes
  • No

Additional documentation e.g. usage docs, diagrams, etc.:

None

@codecov Codecov
Copy link

codecov bot commented Dec 11, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.23%. Comparing base (4b5f5d4) to head (e7c1eb7).
Report is 22 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1034      +/-   ##
==========================================
- Coverage   69.36%   69.23%   -0.14%     
==========================================
  Files         275      275              
  Lines        7152     7193      +41     
==========================================
+ Hits         4961     4980      +19     
- Misses       2191     2213      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arahmarchak arahmarchak force-pushed the enhance-jwt-claim-error-messages branch from 53e9942 to c651335 Compare December 12, 2024 09:51
Annopaolo
Annopaolo reviewed Jan 10, 2025
View reviewed changes
apps/astarte_realm_management_api/lib/astarte_realm_management_api_web/views/error_view.ex Outdated
%{
errors: %{
detail:
"Authorization failed due to an invalid path. Ensure the realm name and endpoint are correctly specified in the request"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: the path does not contain an Astarte endpoint, since this is Realm Management API. See https://docs.astarte-platform.org/astarte/latest/api/?urls.primaryName=Realm%20Management%20API for more

@arahmarchak arahmarchak force-pushed the enhance-jwt-claim-error-messages branch 2 times, most recently from 4de7590 to 11d3e7d Compare January 13, 2025 09:17
@arahmarchak arahmarchak marked this pull request as ready for review January 13, 2025 09:18
@arahmarchak arahmarchak requested a review from Annopaolo January 13, 2025 09:18
@arahmarchak arahmarchak changed the title [WIP] Handle API authentication errors for the API services Handle API authentication errors for the API services Jan 13, 2025
@arahmarchak arahmarchak force-pushed the enhance-jwt-claim-error-messages branch from 11d3e7d to a29db4c Compare January 13, 2025 12:11
eddbbt
eddbbt suggested changes Jan 13, 2025
View reviewed changes
...realm_management_api/lib/astarte_realm_management_api_web/controllers/fallback_controller.ex
@@ -130,11 +130,37 @@ defmodule Astarte.RealmManagement.APIWeb.FallbackController do
end

# This is called when no JWT token is present
def auth_error(conn, {:unauthenticated, _reason}, _opts) do
Copy link
Contributor

@eddbbt eddbbt Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'll keep a catch-all error handler anyway if there are no other good reasons to remove it.
This goes for all the other services too

@arahmarchak arahmarchak force-pushed the enhance-jwt-claim-error-messages branch from a29db4c to 4e16867 Compare January 13, 2025 14:28
@arahmarchak arahmarchak requested a review from eddbbt January 13, 2025 14:29
@arahmarchak arahmarchak force-pushed the enhance-jwt-claim-error-messages branch 4 times, most recently from a7974b2 to 773fd4c Compare January 14, 2025 15:04
Annopaolo
Annopaolo requested changes Jan 14, 2025
View reviewed changes
Copy link
Collaborator

@Annopaolo Annopaolo left a comment

Choose a reason for hiding this comment

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

I only looked superficially at the code and it looks good, I'd suggest adding the new errors to the OpenAPI schema of every app (apps/$APP_NAME/priv/static/$APP_NAME.yaml)

@arahmarchak
Copy link
Contributor Author

arahmarchak commented Jan 15, 2025 •
edited
Loading

I only looked superficially at the code and it looks good, I'd suggest adding the new errors to the OpenAPI schema of every app (apps/$APP_NAME/priv/static/$APP_NAME.yaml)

@Annopaolo Since I'm going to change the .yaml files, should I change the contact email as well?
Current email: [email protected]

@arahmarchak arahmarchak force-pushed the enhance-jwt-claim-error-messages branch from 773fd4c to 2f7728e Compare January 15, 2025 14:17
@arahmarchak arahmarchak requested a review from Annopaolo January 15, 2025 14:18
@arahmarchak arahmarchak force-pushed the enhance-jwt-claim-error-messages branch 4 times, most recently from d8f415f to e23c961 Compare January 16, 2025 09:13
@arahmarchak
Handle API authentication errors for the RM API
9041d5e 
- Expand the Fallback controller with some failed
  auth functions
- Add additional error views with useful feedback
  messages
- Add the gen_jwt_token_with_wrong_signature
  function in the JWTTestHelper
- Add the necessary auth tests and adapt existing
  ones
- Add new errors to the OpenAPI schema

Signed-off-by: Armin Ahmetović <[email protected]>
@arahmarchak
Handle API authentication errors for the AppEngine API
bf1fdb7 
- Expand the Fallback controller with some failed
  auth functions
- Add additional error views with useful feedback
  messages
- Add the gen_jwt_token_with_wrong_signature
  function in the JWTTestHelper
- Add the necessary auth tests and adapt existing
  ones
- Add new errors to the OpenAPI schema

Signed-off-by: Armin Ahmetović <[email protected]>
@arahmarchak
Handle API authentication errors for the Housekeeping API
96822dc 
- Expand the Fallback controller with some failed
  auth functions
- Add additional error views with useful feedback
  messages
- Add the gen_jwt_token_with_wrong_signature
  function in the JWTTestHelper
- Add the necessary auth tests and adapt existing
  ones
- Add new errors to the OpenAPI schema

Signed-off-by: Armin Ahmetović <[email protected]>
@arahmarchak
Handle API authentication errors for the Pairing API
e7c1eb7 
- Expand the Fallback controller with some failed
  auth functions
- Add additional error views with useful feedback
  messages
- Add the gen_jwt_token_with_wrong_signature
  function in the JWTTestHelper
- Add the necessary auth tests and adapt existing
  ones
- Add new errors to the OpenAPI schema

Signed-off-by: Armin Ahmetović <[email protected]>
@arahmarchak arahmarchak force-pushed the enhance-jwt-claim-error-messages branch from e23c961 to e7c1eb7 Compare January 16, 2025 11:09
eddbbt
eddbbt approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@eddbbt eddbbt left a comment

Choose a reason for hiding this comment

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

lgtm

@Annopaolo
Copy link
Collaborator

I only looked superficially at the code and it looks good, I'd suggest adding the new errors to the OpenAPI schema of every app (apps/$APP_NAME/priv/static/$APP_NAME.yaml)

@Annopaolo Since I'm going to change the .yaml files, should I change the contact email as well? Current email: [email protected]

Let's change it in another PR!

@Annopaolo Annopaolo merged commit e6a0933 into astarte-platform:master Jan 16, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddbbt eddbbt eddbbt approved these changes

@Annopaolo Annopaolo Annopaolo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Differentiate jwt and claim errors in the API authentication
3 participants
@arahmarchak @Annopaolo @eddbbt