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

Fix mixing of the @TestSecurity annotation with HTTP request credentials inside one test method #41174

Conversation

michalvavrik
Copy link
Member

fixes: #41125

Before #40059 it was possible to mix the @TestSecurity with actual credentials. We didn't have it well tested because #40059 didn't fail any tests. With this PR, it is possible again and there are tests and additional documentation.

@michalvavrik michalvavrik force-pushed the feature/fix-test-security-http-credentials-comb branch from f81a1ee to 49c1663 Compare June 12, 2024 20:07

This comment has been minimized.

Copy link

github-actions bot commented Jun 12, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@sberyozkin
Copy link
Member

Thanks @michalvavrik.

Did you have a chance to check why it worked before, was it by pure luck ? I'm just wondering since the updates in the PR are quite involved, so I'd like to understand better what exactly we are fixing here. Thanks

@michalvavrik
Copy link
Member Author

michalvavrik commented Jun 13, 2024

Thanks @michalvavrik. Did you have a chance to check why it worked before, was it by pure luck ?

There is this small section that probably describes it is supposed to work https://quarkus.io/guides/security-testing#mixing-security-tests. It could also mean that you are supposed to test basic in a test method not annotated with @TestSecurity. Based on #41125 we know at least about one affected user. Also I think we don't force anyone, but keeping this option open to use it is positive.

I'm just wondering since the updates in the PR are quite involved, so I'd like to understand better what exactly we are fixing here. Thanks

I tried to explain it in Javadoc, but I can improve that if it is not clear. Let me try here as well. Consider test scenario:

  • endpoint is annotated with @BasicAuthentication or HTTP permission quarkus.http.auth.permission.authenticated.auth-mechanism=basic is applied and authentication is required:
  • endpoint is annotated with @Authenticated and no HTTP permission is applied, then:
    • Quarkus iterates over already sorted mechanisms and the first one that is willing to authenticate wins
    • If no mechanisms authenticates, for example because there is no basic authorization header, test mechanism wins
    • that was there before and I'm adding it back

Please ask more question if it is unclear.

@sberyozkin
Copy link
Member

I think it makes sense, I was just curious why it worked before, thanks @michalvavrik, I'll suggest a few minor doc updates a bit later, otherwise looks good.

@sberyozkin
Copy link
Member

Let's merge once a couple of minor suggestions are done

@michalvavrik michalvavrik force-pushed the feature/fix-test-security-http-credentials-comb branch from 49c1663 to 5164af2 Compare June 13, 2024 17:49
@michalvavrik
Copy link
Member Author

michalvavrik commented Jun 13, 2024

3.12 was branched just yesterday, so I'll add a backport label as it is a bug fix; we can probably stay conservative and do not backport it to 3.11 as 3.12 is out soon...

Copy link

quarkus-bot bot commented Jun 13, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 5164af2.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Jun 13, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5164af2.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@sberyozkin sberyozkin merged commit fa5f801 into quarkusio:main Jun 13, 2024
26 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jun 13, 2024
@michalvavrik michalvavrik deleted the feature/fix-test-security-http-credentials-comb branch June 14, 2024 07:17
@gsmet gsmet modified the milestones: 3.13 - main, 3.12.0 Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

TestSecurity behaviour for OIDC extension's local logout changed since 3.11.0.CR1
3 participants