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

DefaultJaxRsRolesAllowedImplMethodSecuredTest produces a large number of errors #43017

Closed
mkouba opened this issue Sep 4, 2024 · 11 comments · Fixed by #43018
Closed

DefaultJaxRsRolesAllowedImplMethodSecuredTest produces a large number of errors #43017

mkouba opened this issue Sep 4, 2024 · 11 comments · Fixed by #43018
Assignees
Milestone

Comments

@mkouba
Copy link
Contributor

mkouba commented Sep 4, 2024

Description

And produces 6K+ of useless lines in the log.

Implementation ideas

No response

@geoand
Copy link
Contributor

geoand commented Sep 4, 2024

@mkouba are you looking at it or do you want me to?

@michalvavrik
Copy link
Member

which one, Quarkus REST or RESTEasy Classic one?

@mkouba
Copy link
Contributor Author

mkouba commented Sep 4, 2024

@mkouba are you looking at it or do you want me to?

I'm not. Feel free to take it ;-)

@michalvavrik
Copy link
Member

I see. Yes, that error is expected, when is security check run - it must not get to serialization unless you are authentication/authorized. So @geoand I think you can just silence it (if you want, or it has to wait till weekend, I am busy ATM). It doesn't signal anything security-wise wrong.

@michalvavrik
Copy link
Member

btw, I suppose it will be true for many classes in that package, I didn't realize that.

@mkouba
Copy link
Contributor Author

mkouba commented Sep 4, 2024

Yes, that error is expected, when is security check run - it must not get to serialization unless you are authentication/authorized.

Well, I can see errors like:

Caused by: io.vertx.core.json.DecodeException: Failed to decode:Unexpected close marker '}': expected ']' (for root starting at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1])

Is that really expected?

@michalvavrik
Copy link
Member

Is that really expected?

What test does with invalid payload is:

  • are you authorized to reach that endpoint? then you get to the serializers and serialization fails => 500
  • are you not authorized to reach that endpoint? then you don't get to the serializers and security checks must forbid access => 401/403

It is a way to recognize whether security check is performed by CDI interceptors or eagerly. HTTP statuses are what you care about.

@michalvavrik
Copy link
Member

cc @sberyozkin anyway, just to assure @mkouba

@geoand
Copy link
Contributor

geoand commented Sep 4, 2024

The Jackson error is indeed expected. We simply need to not log it - I'll have a look

@michalvavrik
Copy link
Member

I'll have a look

Thank you. Please check other test classes in that package.

@michalvavrik michalvavrik assigned geoand and unassigned michalvavrik Sep 4, 2024
@geoand geoand closed this as completed in b459c98 Sep 4, 2024
geoand added a commit that referenced this issue Sep 4, 2024
Reduce log clutter in security tests
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 4, 2024
@gsmet gsmet modified the milestones: 3.16 - main, 3.14.3 Sep 6, 2024
gsmet pushed a commit to gsmet/quarkus that referenced this issue Sep 6, 2024
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants