Add Imperative AuthenticationProvider and Reactive AuthenticationProvider #1526

sdelamo · 2023-12-07T14:41:46Z

jeremyg484 · 2023-12-12T00:19:44Z

@sdelamo It won't let me actually request you as reviewer since you started the PR.

This is ready for review, but a few things to consider right off the top:

As you will see, I realized one caveat of this approach to adapting the BlockingAuthenticationProvider implementations via @EachBean is that it requires the addition of a qualifier to each implementation if providing more than one. I don't love that, but do you think providing multiple implementations would be common enough to warrant a different approach?

Also, I've only provided the Java implementation of the example in the docs - that's all we have for AuthenticationProvider as well. I'm thinking we should go ahead and provide Groovy and Kotlin versions of both as usual - I'll look at that first thing tomorrow.

sdelamo

As you will see, I realized one caveat of this approach to adapting the BlockingAuthenticationProvider implementations via @EachBean is that it requires the addition of a qualifier to each implementation if providing more than one. I don't love that, but do you think providing multiple implementations would be common enough to warrant a different approach?

I think it is fine to require a Name qualifier. I have changed the BlockingAuthenticationProvider to extend io.micronaut.core.naming.Named and the docs show now the example.

Also, I've only provided the Java implementation of the example in the docs - that's all we have for AuthenticationProvider as well. I'm thinking we should go ahead and provide Groovy and Kotlin versions of both as usual - I'll look at that first thing tomorrow.

Yes, we should provide kotlin and groovy examples always even if previous snippet of the docs did not. I have provided Groovy and Kotlin tests.

* BlockingAuthenticationProvider implements Named * split docs * add multilanguage snippet * remove import * revert use class again

...rc/main/java/io/micronaut/security/authentication/BlockingAuthenticationProviderFactory.java

…ockingAuthenticationProviderFactory.java

dstepanov · 2023-12-13T08:52:02Z

...rc/main/java/io/micronaut/security/authentication/BlockingAuthenticationProviderFactory.java

+ */
+@Factory
+@Internal
+class BlockingAuthenticationProviderFactory {


Can we not call it blocking? It doesn't need to block

I have changed the API ImperativeAuthenticationProvider.

If the user implementation is blocking he should add the @Blocking annotation to the overridden method.

What is the plan? Rename it in v5? Or keep it like this forever?
IMHO I would create a new API non-reactive AuthenticationProvider and ReactiveAuthenticationProvider in a different package and deprecate the old one.

If we were starting from zero, I would not be against the naming to be ReactiveAuthenticationProvider for the publisher api and AuthenticationProvider for the imperative API.

But we already have an API in place, which most users will have implemented in their applications and I am not keen to forcing them to change their code for a better name.

Having the same API name in different packages sounds tricky to me as well.

I don't think having two APIs: the existing AuthenticationProvider which is reactive and a new ImperativeAuthenticationProvider which is not reactive is too bad.

@dstepanov I have deprecated

io.micronaut.security.authentication.AuthenticationProvider

and created two APIs:

Imperative => io.micronaut.security.authentication.provider.AuthenticationProvider

Reactive => io.micronaut.security.authentication.provider.ReactiveAuthenticationProvider

c795eaa

@sdelamo I like this - it's clean and more consistent with the terminology we use elsewhere (i.e., Micronaut Data).

dstepanov · 2023-12-13T08:53:09Z

...rc/main/java/io/micronaut/security/authentication/BlockingAuthenticationProviderFactory.java

+
+        @Override
+        public Publisher<AuthenticationResponse> authenticate(T httpRequest, AuthenticationRequest<?, ?> authenticationRequest) {
+            return Mono.fromCallable(() -> blockingAuthenticationProvider.authenticate(httpRequest, authenticationRequest))


I would expect to use the new API directly in the filter, not have it wrapped in a reactive

Preferable this change should allow not engage any reactive filtering when all the filters are non-reactive

I think that should be attempted in the next major version of Micronaut Security.

I have removed the factory and now I only create a ReactiveAuthenticationProvider if users define any other ReactiveAuthenticationProvider or if the user authentication provider is blocking c795eaa

@Blocking

and @Blocking

…thentication-provider

Only create ReactiveAuthenticationProvider from imperative if there is any ReactiveAuthenticationProvider or if there is a blocking imperative AuthenticationProvider

...aut/security/oauth2/endpoint/token/request/password/OauthPasswordAuthenticationProvider.java

…point/token/request/password/OauthPasswordAuthenticationProvider.java

sdelamo · 2023-12-13T18:58:08Z

@jeremyg484 can you review?

jeremyg484

Looks great, just have some nit picking suggestions for JavaDocs.

security/src/main/java/io/micronaut/security/authentication/AuthenticationProvider.java

security/src/main/java/io/micronaut/security/authentication/Authenticator.java

.../main/java/io/micronaut/security/authentication/provider/ReactiveAuthenticationProvider.java

…thenticationProvider.java Co-authored-by: Jeremy Grelle <[email protected]>

…ovider/ReactiveAuthenticationProvider.java Co-authored-by: Jeremy Grelle <[email protected]>

…thenticator.java Co-authored-by: Jeremy Grelle <[email protected]>

.../main/java/io/micronaut/security/authentication/provider/ReactiveAuthenticationProvider.java

...rity/src/main/java/io/micronaut/security/authentication/provider/AuthenticationProvider.java

...c/main/java/io/micronaut/security/authentication/provider/AuthenticationProviderAdapter.java

...src/main/java/io/micronaut/security/authentication/provider/AuthenticationProviderUtils.java

...in/java/io/micronaut/security/authentication/provider/HttpRequestAuthenticationProvider.java

...io/micronaut/security/authentication/provider/HttpRequestReactiveAuthenticationProvider.java

security/src/main/java/io/micronaut/security/authentication/Authenticator.java

… private This commits moves AuthenticationProviderUtils and AuthenticationProviderAdapter package private to make io.micronaut.security.authentication. AuthenticationProviderUtils and AuthenticationProviderAdapter package private.

sonarqubecloud · 2023-12-14T23:30:21Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
88.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

add BlockingAuthenticationProvider

0ea1959

sdelamo assigned jeremyg484 Dec 7, 2023

sdelamo linked an issue Dec 7, 2023 that may be closed by this pull request

add non reactive API for AuthenticationProvider #1476

Closed

Implementation, tests, and docs.

6420364

jeremyg484 marked this pull request as ready for review December 12, 2023 00:00

sdelamo added 4 commits December 12, 2023 10:32

Merge branch 'master' into blocking-authentication-provider

c76773e

BlockingAuthenticationProvider implements Named

70a3a90

split docs

792dc08

add multilanguage snippet

d0b0647

sdelamo commented Dec 12, 2023

View reviewed changes

sdelamo added 4 commits December 12, 2023 11:25

remove import

278a68a

revert use class again

03ddaf3

Merge branch 'master' into blocking-authentication-provider

758afe2

Blocking authentication provider impr (#1532)

74337d4

* BlockingAuthenticationProvider implements Named * split docs * add multilanguage snippet * remove import * revert use class again

sdelamo requested a review from dstepanov December 12, 2023 19:07

sdelamo commented Dec 12, 2023

View reviewed changes

...rc/main/java/io/micronaut/security/authentication/BlockingAuthenticationProviderFactory.java Outdated Show resolved Hide resolved

Update security/src/main/java/io/micronaut/security/authentication/Bl…

3251149

…ockingAuthenticationProviderFactory.java

sdelamo added the type: enhancement New feature or request label Dec 12, 2023

dstepanov requested changes Dec 13, 2023

View reviewed changes

rename to ImperativeAuthenticationProvider

8660cce

and @Blocking

sdelamo requested a review from dstepanov December 13, 2023 11:50

sdelamo added 2 commits December 13, 2023 13:09

Merge branch 'blocking-authentication-provider-impr' into blocking-au…

e5a6f28

…thentication-provider

deprecated AuthenticationProvider with Publisher

c795eaa

sdelamo changed the title ~~add BlockingAuthenticationProvider~~ Add Imperative AuthenticationProvider and Reactive AuthenticationProvider Dec 13, 2023

Don’t wrap create ReactiveAuthenticationProvider

4b689cb

Only create ReactiveAuthenticationProvider from imperative if there is any ReactiveAuthenticationProvider or if there is a blocking imperative AuthenticationProvider

sdelamo commented Dec 13, 2023

View reviewed changes

...aut/security/oauth2/endpoint/token/request/password/OauthPasswordAuthenticationProvider.java Outdated Show resolved Hide resolved

sdelamo added 3 commits December 13, 2023 16:47

Update security-oauth2/src/main/java/io/micronaut/security/oauth2/end…

4fd4a9b

…point/token/request/password/OauthPasswordAuthenticationProvider.java

remove unused constructor

0fff4a6

remove local variables

650b456

jeremyg484 suggested changes Dec 14, 2023

View reviewed changes

sdelamo and others added 3 commits December 14, 2023 06:38

Update security/src/main/java/io/micronaut/security/authentication/Au…

f9008a2

…thenticationProvider.java Co-authored-by: Jeremy Grelle <[email protected]>

Update security/src/main/java/io/micronaut/security/authentication/pr…

bc4f0ae

…ovider/ReactiveAuthenticationProvider.java Co-authored-by: Jeremy Grelle <[email protected]>

Update security/src/main/java/io/micronaut/security/authentication/Au…

395d89d

…thenticator.java Co-authored-by: Jeremy Grelle <[email protected]>

sdelamo requested a review from jeremyg484 December 14, 2023 05:38

graemerocher requested changes Dec 14, 2023

View reviewed changes

sdelamo added 9 commits December 14, 2023 13:34

httpRequest with requestContext

8001291

annotate with @indexed

c737dd1

httpRequest with requestContext

1f237fe

final AuthenticationProviderAdapter

07c40af

add generic argument type

8c8318d

add default methods

e3a5d1b

add new line to package-info.java

35d8d0a

sonar: Catch Exception instead of Throwable

c9ce456

sonar: refactor test

76bf47e

graemerocher approved these changes Dec 14, 2023

View reviewed changes

jeremyg484 approved these changes Dec 14, 2023

View reviewed changes

define generic types

dbc26ee

sdelamo requested review from graemerocher, dstepanov and jeremyg484 December 14, 2023 21:13

fix javadoc

df280f7

jeremyg484 approved these changes Dec 14, 2023

View reviewed changes

sdelamo merged commit 14f234a into master Jan 4, 2024
15 checks passed

sdelamo deleted the blocking-authentication-provider branch January 4, 2024 21:13

sdelamo mentioned this pull request Jan 25, 2024

revert breaking change #1563

Merged

sdelamo mentioned this pull request Feb 22, 2024

Non-reactive TokenValidator #1602

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Imperative AuthenticationProvider and Reactive AuthenticationProvider #1526

Add Imperative AuthenticationProvider and Reactive AuthenticationProvider #1526

sdelamo commented Dec 7, 2023 •

edited

Loading

jeremyg484 commented Dec 12, 2023

sdelamo left a comment

dstepanov Dec 13, 2023

sdelamo Dec 13, 2023

dstepanov Dec 13, 2023

sdelamo Dec 13, 2023

sdelamo Dec 13, 2023 •

edited

Loading

jeremyg484 Dec 13, 2023

dstepanov Dec 13, 2023

dstepanov Dec 13, 2023

sdelamo Dec 13, 2023

sdelamo Dec 13, 2023 •

edited

Loading

sdelamo commented Dec 13, 2023

jeremyg484 left a comment

sonarqubecloud bot commented Dec 14, 2023 •

edited

Loading

Add Imperative AuthenticationProvider and Reactive AuthenticationProvider #1526

Add Imperative AuthenticationProvider and Reactive AuthenticationProvider #1526

Conversation

sdelamo commented Dec 7, 2023 • edited Loading

jeremyg484 commented Dec 12, 2023

sdelamo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdelamo Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdelamo Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

sdelamo commented Dec 13, 2023

jeremyg484 left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 14, 2023 • edited Loading

Quality Gate passed

sdelamo commented Dec 7, 2023 •

edited

Loading

sdelamo Dec 13, 2023 •

edited

Loading

sdelamo Dec 13, 2023 •

edited

Loading

sonarqubecloud bot commented Dec 14, 2023 •

edited

Loading