-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add header authentication for SSO #78
base: main
Are you sure you want to change the base?
Conversation
Not a regular user of Typescript, but will add my thoughts :) |
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.
Using both token and header for authentication might be an anti-pattern (if the reverse proxy sends traffic without the header due to an issue, the token will still be valid and allow access). You can switch between one or the other.
Rest looks good, thanks!
server/src/routes/auth.ts
Outdated
async (req, res, next) => { | ||
async (req, res) => { | ||
if (config.header_auth) { | ||
|
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.
Is this supposed to return here/return something?
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.
Accidentally left it in. They should never need the /signin or /signup routes.
await user.save() | ||
} | ||
|
||
if (!token) { |
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.
Is there a reason for using both header and token auth? (Unless there is a dependency of the token)
If header auth is selected, there is no need for a token. The middleware can verify the header on each request. If header auth is not enabled, proceed as usual with signing the token on login and validating that in the middleware.
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.
Good call. I wrapped this in an if-else and added
req.user = user
next()
if the user is using header auth
client/lib/hooks/use-signed-in.ts
Outdated
import { useEffect } from "react" | ||
import useSharedState from "./use-shared-state" | ||
|
||
|
||
const useSignedIn = () => { | ||
const [signedIn, setSignedIn] = useSharedState( | ||
"signedIn", | ||
typeof window === "undefined" ? false : !!Cookies.get("drift-token") | ||
) | ||
const token = Cookies.get("drift-token") |
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.
Point to note, if the header auth is used, this can/will be null. This is fine, since the middleware should ignore the header altogether.
- `HEADER_AUTH`: if true, enables authenthication via the HTTP header specified in `HEADER_AUTH_KEY` which is generally populated at the reverse-proxy level. | ||
- `HEADER_AUTH_KEY`: if `HEADER_AUTH` is true, the header to look for the users username (like `Auth-User`) | ||
- `HEADER_AUTH_ROLE`: if `HEADER_AUTH` is true, the header to look for the users role ("user" | "admin", at the moment) | ||
- `HEADER_AUTH_WHITELISTED_IPS`: comma-separated list of IPs users can access Drift from using header authentication. Defaults to '127.0.0.1'. |
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.
Yes, this should meet my needs.
Closes #11