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

Provide default OIDC static tenant resolver #32864

Merged

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Apr 24, 2023

Fixes #32834.

This PR will help with a more straightforward static tenant resolution, for example, given a typical configuration like:

#default tenant disabled
quarkus.oidc.tenant-enabled=false

#custom tenant 'google'
quarkus.oidc.google.provider=google
quarkus.oidc.google.client-id=...
quarkus.oidc.google.credentials.secret=..

#custom tenant 'github'
quarkus.oidc.github.provider=github
quarkus.oidc.github.client-id=...
quarkus.oidc.github.credentials.secret=..

or

#default tenant 'google'
quarkus.oidc.provider=google
quarkus.oidc.client-id=...
quarkus.oidc.credentials.secret=..

#custom tenant 'github'
quarkus.oidc.github.provider=github
quarkus.oidc.github.client-id=...
quarkus.oidc.github.credentials.secret=..

now, instead of having to write a custom TenantResolver CDI bean doing the boilerplate code checking the last path segment which serves as a tenant id, and checking, in case of web-app, RoutingContext attribute, the users will just have it working immediately.

The last variation above is tested in integration-tests/keycloak-authorization, where the test TenantResolver doing the boilerplate code has been removed (I just needed to tweak the tenant configuration a bit to have the last path segment matching the tenant key).

This default resolution will only be activated if no custom TenantResolver is available.

This will also work well in a demo.

Also CC @FroMage (in case Steph does a similar boilerplate code in Renarde)

@github-actions
Copy link

github-actions bot commented Apr 24, 2023

🙈 The PR is closed and the preview is expired.

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 24, 2023

✔️ 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.

@stuartwdouglas
Copy link
Member

Is it normal to use the last URL segement to select a provider?

And if so I am guessing this is just for the web-app case for the login URL? Not for URLs once you have the token?

@sberyozkin
Copy link
Member Author

@stuartwdouglas Hi,

Is it normal to use the last URL segement to select a provider?

I'd say so, I think Quarkus Renarde does it for example (CC @FroMage ). It is either the path segment (typical I think), or, I guess, a query parameter. So this PR offers a last fallback checking the path segment, only when custom tenants are present and no custom resolver is available, so it is not really interfering with anything.

And if so I am guessing this is just for the web-app case for the login URL? Not for URLs once you have the token?

If we continue with the web-app, the last segment check there works only for the login url, and then, once the session cookie is created, the authenticated user can access the endpoint, there will be no google/etc there in URLs, but the session cookie name has a tenant identifier in it, ex, q_session_google/etc, so another check there is to check a tenant id attribute on the RoutingContext - this attribute is set before the tenant resolution is attempted.

This is exactly how I'd write a custom TenantResolver myself to support web-app :-), check the routing context first and then try the last segment, as the login endpoint would have login/google etc. I think Steph does more or less the same, though in the Renarde case it is more involved - but this is when users just register the custom tenant resolvers

But, if we have the Bearer token coming in, well, I can't see how else but use the path segment as a hint. I think in general, more than one social provider support is more interesting for the web-app case, but yes, this default fallback would take care of bearer token tenant resolution as well if needed. Custom resolvers are always an option

@sberyozkin
Copy link
Member Author

For bearer tokens, one might also use a dynamic tenant resolver and parse the token to check which provider should verify it (instead of expecting a provider name in the path), but this default fallback won't interfere in that case

@lennartj
Copy link

lennartj commented May 14, 2023

but the session cookie name has a tenant identifier in it, ex, q_session_google/etc, so another check there is to check a tenant id attribute on the RoutingContext - this attribute is set before the tenant resolution is attempted.

I would suggest being very careful about using session cookies for tenant resolution, or - at least - informing the users/developers/customers about potential CORS problems with such a setup. My reasoning is as follows:

  1. Browsers do not send cookies in preflight OPTIONS requests.
  2. Therefore, anytime you have the frontend and backend services running on different ports (typical for a development workstation setup, for example), the browser will launch a OPTIONS preflight request ... without the session cookies. This means your session-cookie tenant configuration will struggle (at best) and stop working completely for any XHR calls in more normal situations.
  3. Hence, if you use multi-tenancy in a CORS situation, you will need to configure both the communications setup of the client-side web application and any network proxies to accept odd custom properties to get it working.

My 2 cents: Use only a resource path segment to supply the tenant id. Having tried some other options, the resource path strategy is the only one which really works in a scalable and reusable way with a minimum of fuzz.

@sberyozkin
Copy link
Member Author

sberyozkin commented May 16, 2023

@lennartj Thanks for the feedback,

Use only a resource path segment to supply the tenant id

It does not really work after the authentication has taken place for non-SPA/non-bearer token setup., when you access Quarkus directly from the browser. Application URI space can not be all partitioned and documented to have /myapp/google/*, /myapp/facebook/* etc. It is just /myapp/service etc. /google/ only makes sense for the myapp/login/google etc - for the login endpoint for Quarkus OIDC web-app applications.

See Quarkus Renarde in action please. Or try a multi-tenant setup with quarkus.oidc.application-type=web-app to see what I mean :-)

Apart from that, tenant identifier in the path segment only works for SPA/CLI clients sending bearer tokens to Quarkus. In such cases, indeed CORS considerations should be considered - but the session cookie is not part of these considerations - it is not available there at all.

In any case as far as this PR is concerned, this is just a fallback (which I verified not only with the tests but also with the browser), when no custom resolvers are available

@sberyozkin
Copy link
Member Author

@lennartj I should've asked though, did you already try quarkus.application-type=web-app ? My understanding that your setup does not involve such applications but I might be wrong . I can imagine some SPA-based applications interacting directly with quarkus.application-type=web-app - but please have no concern - this fallback does not force any such involved application depend on the session cookie - it is a fallback only for the direct browser to Quarkus flows when the user is already authenticated and no custom resolvers are available.

@sberyozkin sberyozkin merged commit a4d0f9c into quarkusio:main May 19, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label May 19, 2023
@sberyozkin sberyozkin deleted the oidc_default_static_tenant_resolver branch May 19, 2023 12:28
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone May 19, 2023
if (pathSegments.length > 0) {
String lastPathSegment = pathSegments[pathSegments.length - 1];
if (tenantConfigBean.getStaticTenantsConfig().containsKey(lastPathSegment)) {
return lastPathSegment;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide default OIDC static tenant resolver
5 participants