-
Notifications
You must be signed in to change notification settings - Fork 350
💥 Fix oauth name & username mixup #1050
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
Changes from all commits
2cd06cd
ef546f7
91ab753
0ee5ffa
be01749
e861740
c7cb4bb
0e079ab
0bf77c6
81d2c3c
cf8abd8
91dc0cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
import { describe, expect, it } from "vitest"; | ||
import { TEST_COOKIE, TEST_HUB_URL } from "../test/consts"; | ||
import { oauthLoginUrl } from "./oauth-login-url"; | ||
import { oauthHandleRedirect } from "./oauth-handle-redirect"; | ||
|
||
describe("oauthHandleRedirect", () => { | ||
it("should work", async () => { | ||
const localStorage = { | ||
nonce: undefined, | ||
codeVerifier: undefined, | ||
}; | ||
const url = await oauthLoginUrl({ | ||
clientId: "dummy-app", | ||
redirectUrl: "http://localhost:3000", | ||
localStorage, | ||
scopes: "openid profile email", | ||
hubUrl: TEST_HUB_URL, | ||
}); | ||
const resp = await fetch(url, { | ||
method: "POST", | ||
headers: { | ||
Cookie: `token=${TEST_COOKIE}`, | ||
}, | ||
redirect: "manual", | ||
}); | ||
if (resp.status !== 303) { | ||
throw new Error(`Failed to fetch url ${url}: ${resp.status} ${resp.statusText}`); | ||
} | ||
const location = resp.headers.get("Location"); | ||
if (!location) { | ||
throw new Error(`No location header in response`); | ||
} | ||
const result = await oauthHandleRedirect({ | ||
redirectedUrl: location, | ||
codeVerifier: localStorage.codeVerifier, | ||
nonce: localStorage.nonce, | ||
hubUrl: TEST_HUB_URL, | ||
}); | ||
|
||
if (!result) { | ||
throw new Error("Expected result to be defined"); | ||
} | ||
expect(result.accessToken).toEqual(expect.any(String)); | ||
expect(result.accessTokenExpiresAt).toBeInstanceOf(Date); | ||
expect(result.accessTokenExpiresAt.getTime()).toBeGreaterThan(Date.now()); | ||
expect(result.scope).toEqual(expect.any(String)); | ||
expect(result.userInfo).toEqual({ | ||
sub: "62f264b9f3c90f4b6514a269", | ||
name: "@huggingface/hub CI bot", | ||
preferred_username: "hub.js", | ||
email_verified: true, | ||
email: "[email protected]", | ||
isPro: false, | ||
picture: "https://hub-ci.huggingface.co/avatars/934b830e9fdaa879487852f79eef7165.svg", | ||
profile: "https://hub-ci.huggingface.co/hub.js", | ||
website: "https://github.com/huggingface/hub.js", | ||
orgs: [], | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,100 @@ | ||
import { HUB_URL } from "../consts"; | ||
import { createApiError } from "../error"; | ||
|
||
export interface UserInfo { | ||
/** | ||
* OpenID Connect field. Unique identifier for the user, even in case of rename. | ||
*/ | ||
sub: string; | ||
/** | ||
* OpenID Connect field. The user's full name. | ||
*/ | ||
name: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. except There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the profile ones are, technically we could not send them but we always do res.json({
...getIdTokenInfo(auth.u, [...(auth.oauth?.scope ?? []), "profile"]), |
||
/** | ||
* OpenID Connect field. The user's username. | ||
*/ | ||
preferred_username: string; | ||
coyotte508 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* OpenID Connect field, available if scope "email" was granted. | ||
*/ | ||
email_verified?: boolean; | ||
/** | ||
* OpenID Connect field, available if scope "email" was granted. | ||
*/ | ||
email?: string; | ||
/** | ||
* OpenID Connect field. The user's profile picture URL. | ||
*/ | ||
picture: string; | ||
/** | ||
* OpenID Connect field. The user's profile URL. | ||
*/ | ||
profile: string; | ||
/** | ||
* OpenID Connect field. The user's website URL. | ||
*/ | ||
website?: string; | ||
|
||
/** | ||
* Hugging Face field. Whether the user is a pro user. | ||
*/ | ||
isPro: boolean; | ||
/** | ||
* Hugging Face field. Whether the user has a payment method set up. Needs "read-billing" scope. | ||
*/ | ||
canPay?: boolean; | ||
/** | ||
* Hugging Face field. The user's orgs | ||
*/ | ||
orgs?: Array<{ | ||
/** | ||
* OpenID Connect field. Unique identifier for the org. | ||
*/ | ||
sub: string; | ||
/** | ||
* OpenID Connect field. The org's full name. | ||
*/ | ||
name: string; | ||
/** | ||
* OpenID Connect field. The org's username. | ||
*/ | ||
preferred_username: string; | ||
/** | ||
* OpenID Connect field. The org's profile picture URL. | ||
*/ | ||
picture: string; | ||
|
||
/** | ||
* Hugging Face field. Whether the org is an enterprise org. | ||
*/ | ||
isEnterprise: boolean; | ||
/** | ||
* Hugging Face field. Whether the org has a payment method set up. Needs "read-billing" scope, and the user needs to approve access to the org in the OAuth page. | ||
*/ | ||
canPay?: boolean; | ||
/** | ||
* Hugging Face field. The user's role in the org. The user needs to approve access to the org in the OAuth page. | ||
*/ | ||
roleInOrg?: string; | ||
/** | ||
* HuggingFace field. When the user granted the oauth app access to the org, but didn't complete SSO. | ||
* | ||
* Should never happen directly after the oauth flow. | ||
*/ | ||
pendingSSO?: boolean; | ||
/** | ||
* HuggingFace field. When the user granted the oauth app access to the org, but didn't complete MFA. | ||
* | ||
* Should never happen directly after the oauth flow. | ||
*/ | ||
missingMFA?: boolean; | ||
}>; | ||
} | ||
|
||
export interface OAuthResult { | ||
accessToken: string; | ||
accessTokenExpiresAt: Date; | ||
userInfo: { | ||
id: string; | ||
name: string; | ||
fullname: string; | ||
email?: string; | ||
emailVerified?: boolean; | ||
avatarUrl: string; | ||
websiteUrl?: string; | ||
isPro: boolean; | ||
canPay?: boolean; | ||
orgs: Array<{ | ||
id: string; | ||
name: string; | ||
isEnterprise: boolean; | ||
canPay?: boolean; | ||
avatarUrl: string; | ||
roleInOrg?: string; | ||
}>; | ||
}; | ||
userInfo: UserInfo; | ||
/** | ||
* State passed to the OAuth provider in the original request to the OAuth provider. | ||
*/ | ||
|
@@ -39,12 +111,47 @@ export interface OAuthResult { | |
* There is also a helper function {@link oauthHandleRedirectIfPresent}, which will call `oauthHandleRedirect` if the URL contains an oauth code | ||
* in the query parameters and return `false` otherwise. | ||
*/ | ||
export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<OAuthResult> { | ||
if (typeof window === "undefined") { | ||
throw new Error("oauthHandleRedirect is only available in the browser"); | ||
export async function oauthHandleRedirect(opts?: { | ||
/** | ||
* The URL of the hub. Defaults to {@link HUB_URL}. | ||
*/ | ||
hubUrl?: string; | ||
/** | ||
* The URL to analyze. | ||
* | ||
* @default window.location.href | ||
*/ | ||
redirectedUrl?: string; | ||
/** | ||
* nonce generated by oauthLoginUrl | ||
* | ||
* @default localStorage.getItem("huggingface.co:oauth:nonce") | ||
*/ | ||
nonce?: string; | ||
/** | ||
* codeVerifier generated by oauthLoginUrl | ||
* | ||
* @default localStorage.getItem("huggingface.co:oauth:code_verifier") | ||
*/ | ||
codeVerifier?: string; | ||
}): Promise<OAuthResult> { | ||
if (typeof window === "undefined" && !opts?.redirectedUrl) { | ||
throw new Error("oauthHandleRedirect is only available in the browser, unless you provide redirectedUrl"); | ||
} | ||
if (typeof localStorage === "undefined" && (!opts?.nonce || !opts?.codeVerifier)) { | ||
throw new Error( | ||
"oauthHandleRedirect requires localStorage to be available, unless you provide nonce and codeVerifier" | ||
); | ||
} | ||
|
||
const searchParams = new URLSearchParams(window.location.search); | ||
const redirectedUrl = opts?.redirectedUrl ?? window.location.href; | ||
const searchParams = (() => { | ||
try { | ||
return new URL(redirectedUrl).searchParams; | ||
} catch (err) { | ||
throw new Error("Failed to parse redirected URL: " + redirectedUrl); | ||
} | ||
})(); | ||
|
||
const [error, errorDescription] = [searchParams.get("error"), searchParams.get("error_description")]; | ||
|
||
|
@@ -53,17 +160,17 @@ export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<O | |
} | ||
|
||
const code = searchParams.get("code"); | ||
const nonce = localStorage.getItem("huggingface.co:oauth:nonce"); | ||
const nonce = opts?.nonce ?? localStorage.getItem("huggingface.co:oauth:nonce"); | ||
|
||
if (!code) { | ||
throw new Error("Missing oauth code from query parameters in redirected URL"); | ||
throw new Error("Missing oauth code from query parameters in redirected URL: " + redirectedUrl); | ||
} | ||
|
||
if (!nonce) { | ||
throw new Error("Missing oauth nonce from localStorage"); | ||
} | ||
|
||
const codeVerifier = localStorage.getItem("huggingface.co:oauth:code_verifier"); | ||
const codeVerifier = opts?.codeVerifier ?? localStorage.getItem("huggingface.co:oauth:code_verifier"); | ||
|
||
if (!codeVerifier) { | ||
throw new Error("Missing oauth code_verifier from localStorage"); | ||
|
@@ -119,8 +226,12 @@ export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<O | |
}).toString(), | ||
}); | ||
|
||
localStorage.removeItem("huggingface.co:oauth:code_verifier"); | ||
localStorage.removeItem("huggingface.co:oauth:nonce"); | ||
if (!opts?.codeVerifier) { | ||
localStorage.removeItem("huggingface.co:oauth:code_verifier"); | ||
} | ||
if (!opts?.nonce) { | ||
localStorage.removeItem("huggingface.co:oauth:nonce"); | ||
} | ||
|
||
if (!tokenRes.ok) { | ||
throw await createApiError(tokenRes); | ||
|
@@ -147,49 +258,12 @@ export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<O | |
throw await createApiError(userInfoRes); | ||
} | ||
|
||
const userInfo: { | ||
sub: string; | ||
name: string; | ||
preferred_username: string; | ||
email_verified?: boolean; | ||
email?: string; | ||
picture: string; | ||
website?: string; | ||
isPro: boolean; | ||
canPay?: boolean; | ||
orgs?: Array<{ | ||
sub: string; | ||
name: string; | ||
picture: string; | ||
isEnterprise: boolean; | ||
canPay?: boolean; | ||
roleInOrg?: string; | ||
}>; | ||
} = await userInfoRes.json(); | ||
const userInfo: UserInfo = await userInfoRes.json(); | ||
|
||
return { | ||
accessToken: token.access_token, | ||
accessTokenExpiresAt, | ||
userInfo: { | ||
id: userInfo.sub, | ||
name: userInfo.name, | ||
fullname: userInfo.preferred_username, | ||
email: userInfo.email, | ||
emailVerified: userInfo.email_verified, | ||
avatarUrl: userInfo.picture, | ||
websiteUrl: userInfo.website, | ||
isPro: userInfo.isPro, | ||
orgs: | ||
userInfo.orgs?.map((org) => ({ | ||
id: org.sub, | ||
name: org.name, | ||
fullname: org.name, | ||
isEnterprise: org.isEnterprise, | ||
canPay: org.canPay, | ||
avatarUrl: org.picture, | ||
roleInOrg: org.roleInOrg, | ||
})) ?? [], | ||
}, | ||
userInfo: userInfo, | ||
state: parsedState.state, | ||
scope: token.scope, | ||
}; | ||
|
@@ -207,12 +281,39 @@ export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<O | |
* | ||
* Depending on your app, you may want to call {@link oauthHandleRedirect} directly instead. | ||
*/ | ||
export async function oauthHandleRedirectIfPresent(opts?: { hubUrl?: string }): Promise<OAuthResult | false> { | ||
if (typeof window === "undefined") { | ||
throw new Error("oauthHandleRedirect is only available in the browser"); | ||
export async function oauthHandleRedirectIfPresent(opts?: { | ||
/** | ||
* The URL of the hub. Defaults to {@link HUB_URL}. | ||
*/ | ||
hubUrl?: string; | ||
/** | ||
* The URL to analyze. | ||
* | ||
* @default window.location.href | ||
*/ | ||
redirectedUrl?: string; | ||
/** | ||
* nonce generated by oauthLoginUrl | ||
* | ||
* @default localStorage.getItem("huggingface.co:oauth:nonce") | ||
*/ | ||
nonce?: string; | ||
/** | ||
* codeVerifier generated by oauthLoginUrl | ||
* | ||
* @default localStorage.getItem("huggingface.co:oauth:code_verifier") | ||
*/ | ||
codeVerifier?: string; | ||
}): Promise<OAuthResult | false> { | ||
if (typeof window === "undefined" && !opts?.redirectedUrl) { | ||
throw new Error("oauthHandleRedirect is only available in the browser, unless you provide redirectedUrl"); | ||
} | ||
|
||
const searchParams = new URLSearchParams(window.location.search); | ||
if (typeof localStorage === "undefined" && (!opts?.nonce || !opts?.codeVerifier)) { | ||
throw new Error( | ||
"oauthHandleRedirect requires localStorage to be available, unless you provide nonce and codeVerifier" | ||
); | ||
} | ||
const searchParams = new URLSearchParams(opts?.redirectedUrl ?? window.location.search); | ||
|
||
if (searchParams.has("error")) { | ||
return oauthHandleRedirect(opts); | ||
|
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.
how of curiosity, is there any reason to do a POST instead of a GET (or vice-versa) here?
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.
The POST is to grant the oauth app the authorization. GET works for subsequent calls, but not the first time ever for the user <-> app tuple