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

Add header authentication for SSO #78

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add header authentication for SSO #78

wants to merge 5 commits into from

Conversation

MaxLeiter
Copy link
Owner

Closes #11

@MaxLeiter
Copy link
Owner Author

@alyx @adyanth pinging in case you're willing / able to give this a review

@adyanth
Copy link

adyanth commented Apr 20, 2022

Not a regular user of Typescript, but will add my thoughts :)

Copy link

@adyanth adyanth left a 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!

async (req, res, next) => {
async (req, res) => {
if (config.header_auth) {

Copy link

@adyanth adyanth Apr 20, 2022

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?

Copy link
Owner Author

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) {
Copy link

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.

Copy link
Owner Author

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

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")
Copy link

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'.
Copy link

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.

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

Successfully merging this pull request may close these issues.

SSO Support?
3 participants