Allow @PermissionChecker methods to authorize secured methods when @TestSecurity annotation is applied and conditionally apply SecurityIdentityAugmentors

[michalvavrik]

• closes: TestSecurity#permissions using SecurityIdentityAugmentor only work with proactive auth #44479
• till now, methods secured with @PermissionAllowed("see") where the see permission was granted by @PermissionChecker("see") checker method always returned 401/403, now the checker method is actually invoked when the @TestSecurity annotation is applied
• SecurityIdentityAugmentors did not augment the identity produced by the @TestSecurity annotation, now users can explicitly list augmentors that should be applied

[sberyozkin]

@michalvavrik Just a quick check, AFAIK, the augmentors can only be applied to the test identity if the user chose to do it with a property, otherwise, given that augmentors can make remote calls, applying them alongside @TestSecurity can be problematic. What is being changed in this PR ?

[michalvavrik]

@michalvavrik Just a quick check, AFAIK, the augmentors can only be applied to the test identity if the user chose to do it with a property

I am -1 to add yet another configuration property. I'll add it if you insist, but I think if we continue this way, "developing with Quarkus" will mean a lot of reading docs for a note why my augmentors are not applied. I prefer as little exceptions as possible.

otherwise, given that augmentors can make remote calls, applying them alongside @TestSecurity can be problematic.

If you don't want to use certain bean in a test environment, please use @UnlessBuildProfile(anyOf = { "test", "dev" }). If augmentors are present, I think it is expected to have them applied and it feels to me like a bug they were not. How is this different to any other CDI bean?

What is being changed in this PR ?

This PR always applys augmentors.

[michalvavrik]

I think I made mistake that I haven't added example with @UnlessBuildProfile(anyOf = { "test", "dev" }) on augmentors. It is kind of a thing I'd expect users to figure, but maybe a small hint would help.

[sberyozkin]

@michalvavrik Sorry, I was not proposing a new property, I thought there was a property in https://quarkus.io/guides/security-testing#mixing-security-tests, but I see now it is done dynamically - if there is no Authorization header - then @TestSecurity populates the credentials...

If you don't want to use certain bean in a test environment, please use @UnlessBuildProfile(anyOf = { "test", "dev" }). If augmentors are present, I think it is expected to have them applied and it feels to me like a bug they were not. How is this different to any other CDI bean?

Well, until now, on the @TestSecurity path, they are not applied. For example, let's say SecurityIdentityAugmentor reads a role admin from DB. But with @TestSecurity users just say @TestSecurity(roles="admin"), they don't want a DB read, this is the whole point of @TestSecurity, that no external connections of any sort are required...

So making augmentors always available can be a breaking change for some of the @TestSecurity cases, with users required to update their code with @UnlessBuildProfile(anyOf = { "test", "dev" }). if they don't want the @TestSecurity tests failing, which appears a bit intrusive for their non-test related code...

I'm not 100% sure how to resolve it cleanly. The description at #44479 refers to this text If you need to test custom permissions, you can add them with io. quarkus. security. identity. SecurityIdentityAugmentor - IMHO we should advise using @PermissionChecker which would avoid the augmentation path, but in any case, what if indeed a user has a simple SecurityIdentityAugmentor, how to apply it...

I guess it is either marking this PR as a breaking change and asking users to add @UnlessBuildProfile(anyOf = { "test", "dev" }). to their augmentors, or may be add TestSecurity augmentors property which would list required SecurityIdentityAugmentor classes ? IMHO the latter option, even though it requires setting an extra test property, allows users to stay in control...

[michalvavrik]

You are right, applying augmentors makes this PR breaking. My personal preference was to apply augmentors, but we can keep status quo and add a new feature: apply augmentors on demand with @TestSecurity(augmentors. It means I need to change this PR so I'll make it draft and ping you when it is ready.

[github-actions]

🙈 The PR is closed and the preview is expired.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

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

✅ 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.

Warning

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

[michalvavrik]

Should be done, ready for review when the time is right for you.

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ 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]

Thanks @michalvavrik