OAuth2AuthHandler.withScope returns new instance rather than updating current instance

[paulswithers]

Version

4.3.1

Context

I upgraded from 4.0.0. There were a few changes I needed to make, which went fine. But when testing OAuth with Webex, I got a message saying I was passing an invalid scope. I checked the URL, and no scope was being passed although I called withScope()

Investigating further, I found OAuth2AuthImpl.withScope() and .withScopes both return a new instance rather than the current instance https://github.com/vert-x3/vertx-web/blob/4.3.1/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/OAuth2AuthHandlerImpl.java#L240. Other fluent methods return the same instance.

Do you have a reproducer?

I don't have a reproducer that calls an external provider because Webex APIs only work with a paid account But you can review by looking at the example in the documentation, https://github.com/vert-x3/vertx-web/blob/master/vertx-web/src/main/java/examples/WebExamples.java#L1386. From what I've stepped through in my code, this won't work as expected, because on line 1402 it will pass a scope to add. But the method called doesn't update the current oauth2 instance, instead is creates a new instance of the class and returns that. If you can debug the code, you should see at line 1405 oauth2 does not have the scopes passed in line 1402. In order for the example to work, it would need to be changed to be oauth2 = oauth2.withScope("profile");.

Steps to reproduce

1. If you can debug the example, you will see it calls withScope() at line 1402
2. After the method has been called, the oauth2 instance at line 1405 won't have any scopes.

Extra

I'm not sure which is your preferred solution. I can work around it by reassigning my OAuth2AuthHandler. I just want to make sure the code I have is best for the future and that the documentation and code align.

[pmlopes]

The javadoc does mention that it does return a new instance. The reason behind that change was mostly to ensure that openapi would work as expected but it seems that the problem is more complex.

In openapi we first instantiate the security handlers (OAuth2 for example) and then at the route path level we know, which scopes are required. This means that may need to have different levels of scope requests according to where the users is calling.

To ensure that the state of the oauth2 handler was stable/immutable, every time someone requires a different set of scopes a new handler is cloned.

This brings us to a next discussion we may need to address on a major release. Perhaps all the mutable state of handlers should be extracted to a Options data object.

Other option is that we make use of the new "metadata" API which seems a great fit for this. For example:

Oauth2Handler oauth2 = OAuth2AuthHandler.create();
router
  .route("/api/openid")
  .putMetadata("scopes", List.of("openid", "email"))
  .putMetadata("flow", "auth_code")
  .handler(oauth2);

router
  .route("/api/opaque")
  .handler(oauth2)

So at the 1st route, an unauthenticated user will be redirected to an IdP requesting a token with "openid" and "email", while the second route will not require any kind of scopes.

For the moment, the workaround to preserve the old behavior is like noticed to replace all references to the handler.

[paulswithers]

Thanks for the detailed explanation. I like the idea of the metadata API. I tested that and it works (I had two endpoints secured, once I added it to both it was fine.)

Is it worth updating the example in the docs? I am happy to make that change and submit a PR, if you wish. Does updating the examples in this repo automatically update the relevant docs page?

[pmlopes]

@paulswithers please, if you can provide a PR updating the docs + examples in the docs it would be really nice!

Thanks!

[fbuetler]

Is it intentional that order and callback are not copied into the new instance in the private constructor after a withScope method call?

If not, I could add it to this PR https://github.com/vert-x3/vertx-web/pull/2337

[pmlopes]

@fbuetler I believe you're right. They should be copied if I'm not mistaken.

[pmlopes]

Now that we're finishing vert.x 5.0.0 and given that this would introduce a behavior change, a PR is ready that follows what has been discussed in this issue.

[pmlopes]

Note that we don't cover flow anymore as vertx-auth has been improved to accept any kind of flow. In the past when dealing with auth_code for frontend UI and for example On-Behalf-Of for service to service, we would require 2 instances of auth providers configured for each flow, now that the Credentials object isn't a raw JSON but a types POJO we can distinguish between the flows and take the right code path.