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

Mrc-5901 Fix Excepion handling packit #135

Merged
merged 8 commits into from
Oct 25, 2024
Merged

Conversation

absternator
Copy link
Contributor

@absternator absternator commented Oct 23, 2024

The following PR fixes multiple issues to do with exception handling. The following are fixed:

  1. If incorrect method passed for route (eg POST for /hello but we don't have)
  2. if route not found 404
  3. catches and throws exceptions from exceptionhandler caught in filters
  4. When packitException thrown and no key found in error bundle

Testing:

  1. try route that is not found eg. /random
  2. try wrong HTTP type eg. POST for /packets
  3. try messing with bearer token to get bearer token errors
  4. Passin in auth header but don't add 'Bearer' first

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.22%. Comparing base (6ad0ede) to head (411cc93).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #135   +/-   ##
=======================================
  Coverage   97.22%   97.22%           
=======================================
  Files         131      131           
  Lines        1225     1225           
  Branches      339      339           
=======================================
  Hits         1191     1191           
  Misses         33       33           
  Partials        1        1           
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@plietar plietar left a comment

Choose a reason for hiding this comment

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

Just two notes, feel free to fix them or do them later or not at all. Happy to merge as it is.

): SecurityFilterChain
{
httpSecurity
.cors { it.configurationSource(getCorsConfigurationSource()) }
.csrf { it.disable() }
.addFilterBefore(filterChainExceptionHandler, LogoutFilter::class.java)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? I didn't think the api server had any concept of logout, the client just throws away their JWT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah its so that any errors caught in filters can be caught be ExceptionHandler... eg. tokenAuthentication.... i just decided to place this filter before LogoutFilter because it is called quite early in the filter chain

"/role/testRole",
HttpMethod.DELETE,
getTokenizedHttpEntity(data = createTestRoleBody),
String::class.java
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that this operation should return a 403 permission denied, not a 401.

403 is if the request has valid credentials, but they don't have the right scopes.
401 is if the request doesn't have credentials at all, or they are invalid.

Presumably this applies to all our endpoints.

I'm not too hung up about fixing it now, but I think we should eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yip that is true.. yip will make a ticket for this

Dont show service user on mange pages
@absternator absternator merged commit 6eb90f3 into main Oct 25, 2024
3 of 4 checks passed
@absternator absternator deleted the mrc-5901-401-insteadof-404 branch October 25, 2024 07:58
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.

2 participants