-
Notifications
You must be signed in to change notification settings - Fork 218
feat: add basePath option #236
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
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: b6999d9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
e4f2bfe to
448d354
Compare
|
EDIT: Misread spec, well-known paths still needs a rewrite Since 448d354, no need to have router/reverse proxy redirection of ExampleWith an issuer set up as const app = issuer({
basePath: `/auth`,
...
})The resulting JWT payload will be {
"properties": {
"userID": 123
},
"aud": "example",
"iss": "http://localhost:3000/auth",
}Spec compliance[INCORRECT] According to RFC 8414 (OAuth 2.0 Authorization Server Metadata) and OpenID Connect Discovery, the [/INCORRECT] |
cf10064 to
57ed877
Compare
Waiting for PR [#236](sst/openauth#236) to be merged
Waiting for PR [#236](sst/openauth#236) to be merged
* feat(auth): try out setting up OpenAuth Doesn’t work, needs to run from `/` * refactor: add i18n types + small code changes * fix: ui adjustments * feat: better handling (#83) * feat: add tryCatch helper function * feat: add neverthrow + tryCatch helper * feat: reorganise api routes for better endpoints protections * feat: add caddy proxy to dev environment everything can be served directly from `localhost` API routes will be proxied to :1993 and web routes to :3000 incomplete attempt to get OpenAuth working from another root * fix: malformed package.json * chore: add `unstorage` patch for use with OpenAuth * chore: add `openauth` patch Waiting for PR [#236](sst/openauth#236) to be merged * chore: remove unused login and signup routes this is now managed by OpenAuth * feat: add `APP_URL` config option + include api version in `API_BASE` * chore: add logo to public dir * fix: update unstorage adapter * feat: get OpenAuth working properly * feat: handle config errors gracefully Now pretty printing errors for better usability and clarity for user * chore: adjust tests to new config requirements * ci: run on all branches * fix(config): api base base computation * feat: implement logout * refactor: move `setTokens` * feat: implement auth check on API * chore: use unstorage fsDriver instead of memory signing key changes on every server reload is highly irritating * fix: correct config test * chore: remove unused * fix: type mismatch * fix: locale types
|
I misread the spec, Well-Known URIs do need to be at root. Issuer still contains base path, but .well-known links don't include it. |
Misread spec, Well-Known URIs need to be at root. Issuer now links .well-known from root Client now reads .well-known from root
| // Only edit local redirects if baseP | ||
| if (basePath) { | ||
| app.use( | ||
| createMiddleware(async (c, next) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this middleware handle lines 1130-1132 of this same file?
if (provider) return c.redirect(`/${provider}/authorize`)
const providers = Object.keys(input.providers)
if (providers.length === 1) return c.redirect(`/${providers[0]}/authorize`)
Here it's redirecting and this needs to make sure it redirect with the correct basePath
Fixes #125.
Still requires a rewrite from whatever runs at root (e.g. reverse proxy) to be able to serve the
/.well-knownroutes, but I can't think of any way that would be technically possible since OpenAuth wouldn't be managing that root path.Technical approach
Instead of refactoring all redirect calls, and requiring a special syntax or function call to avoid regression, I added a middleware to dynamically rewrite urls.
The DX simplicity should outweigh the minor performance cost.
This middleware is only added if the
basePathoption is specified.The base path isn't passed onto Hono through
app.basePath(), this assumes the different base path is managed by something else (router, reverse proxy, etc.)