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

feat: add path prefixes for routers #1015

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FerdinandvHagen
Copy link
Contributor

Description

Implements path prefixes for both public and admin routers so hanko can be hosted also on a subpath.
E.g. AWS Application Load Balancers do not offer the option to rewrite the path - so hosting Hanko would always require an additional system (e.g. nginx) if hosted on a subpath on a shared domain.

With these small changes it's possible to host hanko on a subpath.

Tests

Please let me know if there is a proper way to test this (that makes sense).

Implements path prefixes for both public and admin routers so hanko can be hosted also on a subpath.
@FlxMgdnz FlxMgdnz requested review from like-a-bause and lfleischmann and removed request for like-a-bause and lfleischmann September 12, 2023 07:30
@like-a-bause
Copy link
Contributor

I think you could add another test run to the e2e tests with a path prefix configured, but that seem a little overkill.

@FreddyDevelop
Copy link
Contributor

The wellKnownHandler must also be available under the configured path prefix otherwise the frontend could not get the config.

Also when the statusHandler in the public router is affected by the path prefix, then the statusHandler from the admin router should also be affected (for consistency).

@FerdinandvHagen
Copy link
Contributor Author

I guess well-known would need to be configurable then.
Because the reason I'm intending to use the feature for would be to host Hanko, Backend Services and the Frontend SPA on the same domain.

In this case - because of how the endpoints in Hanko are named you are running the risk of a naming collision on the endpoints (and the config for the load balancer becomes messy). Hence I would place the public routes e.g. under a common path like /api/auth.

But the .well-known endpoints I want to keep at their base path alongside any other .well-known handlers.

For the admin API it could also make sense to have a path-prefix but it's functionality will be limited. However I would keep it separate from the public one?

@FreddyDevelop
Copy link
Contributor

FreddyDevelop commented Oct 9, 2023

Having another prefix only for the well-known route can become complex to configure, because you must also change the frontend-sdk and configure it correctly. Currently it uses the base-url (given at the register call) and appends the necessary path, so the fontend-sdk needs another path-prefix only for the well-known routes.
With only one prefix (for the public and the well-known routes) nothing needs to be changed in the frontend-sdk because you can just append it to the base-url.

The admin API path-prefix should be kept separated from the public one.

@FlxMgdnz FlxMgdnz requested review from lfleischmann and removed request for like-a-bause October 20, 2023 10:08
@lfleischmann lfleischmann removed their request for review October 20, 2023 10:14
@FreddyDevelop
Copy link
Contributor

@FerdinandvHagen I thought a little bit more about this, maybe we can add a new config option (e.g. path_prefix_affects_well_known: true) which defaults to true. When it is set to false we should log a warning that hanko-elements will no longer work, because it can not get the config. Or hanko-elements must also be changed in order to still work with this change.

@FerdinandvHagen
Copy link
Contributor Author

Sounds also fine with me - even though the config might get more and more complicated.
I will see when I find time to update the PR - and add some tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants