Skip to content

Commit 5a83944

Browse files
coyotte508coyotte508
authored
💥 Fix oauth name & username mixup (#1050)
- Change format of `userInfo` returned, use raw response from the hub - Make it work in non-browser context (user needs to pass additional args) - useful for automated tests - Add automated tests Due to breaking change, will need major version upgrade In the end I decided to go with the raw response, without transforming it. Because a lot of the fields are standard openID connect fields (eg `preferred_username`), and to make the developer's job easier when they search docs or they already have other OpenID integrations in their app. Even if it's snake_case while the rest of the API is camelCase.
1 parent 713844e commit 5a83944

File tree

7 files changed

+284
-80
lines changed

7 files changed

+284
-80
lines changed

‎packages/hub/src/error.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ export async function createApiError(
1515
if (response.headers.get("Content-Type")?.startsWith("application/json")) {
1616
const json = await response.json();
1717
error.message = json.error || json.message || error.message;
18+
if (json.error_description) {
19+
error.message = error.message ? error.message + `: ${json.error_description}` : json.error_description;
20+
}
1821
error.data = json;
1922
} else {
2023
error.data = { message: await response.text() };

‎packages/hub/src/lib/create-repo.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ import { toRepoId } from "../utils/toRepoId";
99
export async function createRepo(
1010
params: {
1111
repo: RepoDesignation;
12+
/**
13+
* If unset, will follow the organization's default setting. (typically public, except for some Enterprise organizations)
14+
*/
1215
private?: boolean;
1316
license?: string;
1417
/**

‎packages/hub/src/lib/oauth-handle-redirect.spec.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import { describe, expect, it } from "vitest";
2+
import { TEST_COOKIE, TEST_HUB_URL } from "../test/consts";
3+
import { oauthLoginUrl } from "./oauth-login-url";
4+
import { oauthHandleRedirect } from "./oauth-handle-redirect";
5+
6+
describe("oauthHandleRedirect", () => {
7+
it("should work", async () => {
8+
const localStorage = {
9+
nonce: undefined,
10+
codeVerifier: undefined,
11+
};
12+
const url = await oauthLoginUrl({
13+
clientId: "dummy-app",
14+
redirectUrl: "http://localhost:3000",
15+
localStorage,
16+
scopes: "openid profile email",
17+
hubUrl: TEST_HUB_URL,
18+
});
19+
const resp = await fetch(url, {
20+
method: "POST",
21+
headers: {
22+
Cookie: `token=${TEST_COOKIE}`,
23+
},
24+
redirect: "manual",
25+
});
26+
if (resp.status !== 303) {
27+
throw new Error(`Failed to fetch url ${url}: ${resp.status} ${resp.statusText}`);
28+
}
29+
const location = resp.headers.get("Location");
30+
if (!location) {
31+
throw new Error(`No location header in response`);
32+
}
33+
const result = await oauthHandleRedirect({
34+
redirectedUrl: location,
35+
codeVerifier: localStorage.codeVerifier,
36+
nonce: localStorage.nonce,
37+
hubUrl: TEST_HUB_URL,
38+
});
39+
40+
if (!result) {
41+
throw new Error("Expected result to be defined");
42+
}
43+
expect(result.accessToken).toEqual(expect.any(String));
44+
expect(result.accessTokenExpiresAt).toBeInstanceOf(Date);
45+
expect(result.accessTokenExpiresAt.getTime()).toBeGreaterThan(Date.now());
46+
expect(result.scope).toEqual(expect.any(String));
47+
expect(result.userInfo).toEqual({
48+
sub: "62f264b9f3c90f4b6514a269",
49+
name: "@huggingface/hub CI bot",
50+
preferred_username: "hub.js",
51+
email_verified: true,
52+
email: "[email protected]",
53+
isPro: false,
54+
picture: "https://hub-ci.huggingface.co/avatars/934b830e9fdaa879487852f79eef7165.svg",
55+
profile: "https://hub-ci.huggingface.co/hub.js",
56+
website: "https://github.com/huggingface/hub.js",
57+
orgs: [],
58+
});
59+
});
60+
});

‎packages/hub/src/lib/oauth-handle-redirect.ts

Lines changed: 173 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,100 @@
11
import { HUB_URL } from "../consts";
22
import { createApiError } from "../error";
33

4+
export interface UserInfo {
5+
/**
6+
* OpenID Connect field. Unique identifier for the user, even in case of rename.
7+
*/
8+
sub: string;
9+
/**
10+
* OpenID Connect field. The user's full name.
11+
*/
12+
name: string;
13+
/**
14+
* OpenID Connect field. The user's username.
15+
*/
16+
preferred_username: string;
17+
/**
18+
* OpenID Connect field, available if scope "email" was granted.
19+
*/
20+
email_verified?: boolean;
21+
/**
22+
* OpenID Connect field, available if scope "email" was granted.
23+
*/
24+
email?: string;
25+
/**
26+
* OpenID Connect field. The user's profile picture URL.
27+
*/
28+
picture: string;
29+
/**
30+
* OpenID Connect field. The user's profile URL.
31+
*/
32+
profile: string;
33+
/**
34+
* OpenID Connect field. The user's website URL.
35+
*/
36+
website?: string;
37+
38+
/**
39+
* Hugging Face field. Whether the user is a pro user.
40+
*/
41+
isPro: boolean;
42+
/**
43+
* Hugging Face field. Whether the user has a payment method set up. Needs "read-billing" scope.
44+
*/
45+
canPay?: boolean;
46+
/**
47+
* Hugging Face field. The user's orgs
48+
*/
49+
orgs?: Array<{
50+
/**
51+
* OpenID Connect field. Unique identifier for the org.
52+
*/
53+
sub: string;
54+
/**
55+
* OpenID Connect field. The org's full name.
56+
*/
57+
name: string;
58+
/**
59+
* OpenID Connect field. The org's username.
60+
*/
61+
preferred_username: string;
62+
/**
63+
* OpenID Connect field. The org's profile picture URL.
64+
*/
65+
picture: string;
66+
67+
/**
68+
* Hugging Face field. Whether the org is an enterprise org.
69+
*/
70+
isEnterprise: boolean;
71+
/**
72+
* 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.
73+
*/
74+
canPay?: boolean;
75+
/**
76+
* Hugging Face field. The user's role in the org. The user needs to approve access to the org in the OAuth page.
77+
*/
78+
roleInOrg?: string;
79+
/**
80+
* HuggingFace field. When the user granted the oauth app access to the org, but didn't complete SSO.
81+
*
82+
* Should never happen directly after the oauth flow.
83+
*/
84+
pendingSSO?: boolean;
85+
/**
86+
* HuggingFace field. When the user granted the oauth app access to the org, but didn't complete MFA.
87+
*
88+
* Should never happen directly after the oauth flow.
89+
*/
90+
missingMFA?: boolean;
91+
}>;
92+
}
93+
494
export interface OAuthResult {
595
accessToken: string;
696
accessTokenExpiresAt: Date;
7-
userInfo: {
8-
id: string;
9-
name: string;
10-
fullname: string;
11-
email?: string;
12-
emailVerified?: boolean;
13-
avatarUrl: string;
14-
websiteUrl?: string;
15-
isPro: boolean;
16-
canPay?: boolean;
17-
orgs: Array<{
18-
id: string;
19-
name: string;
20-
isEnterprise: boolean;
21-
canPay?: boolean;
22-
avatarUrl: string;
23-
roleInOrg?: string;
24-
}>;
25-
};
97+
userInfo: UserInfo;
2698
/**
2799
* State passed to the OAuth provider in the original request to the OAuth provider.
28100
*/
@@ -39,12 +111,47 @@ export interface OAuthResult {
39111
* There is also a helper function {@link oauthHandleRedirectIfPresent}, which will call `oauthHandleRedirect` if the URL contains an oauth code
40112
* in the query parameters and return `false` otherwise.
41113
*/
42-
export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<OAuthResult> {
43-
if (typeof window === "undefined") {
44-
throw new Error("oauthHandleRedirect is only available in the browser");
114+
export async function oauthHandleRedirect(opts?: {
115+
/**
116+
* The URL of the hub. Defaults to {@link HUB_URL}.
117+
*/
118+
hubUrl?: string;
119+
/**
120+
* The URL to analyze.
121+
*
122+
* @default window.location.href
123+
*/
124+
redirectedUrl?: string;
125+
/**
126+
* nonce generated by oauthLoginUrl
127+
*
128+
* @default localStorage.getItem("huggingface.co:oauth:nonce")
129+
*/
130+
nonce?: string;
131+
/**
132+
* codeVerifier generated by oauthLoginUrl
133+
*
134+
* @default localStorage.getItem("huggingface.co:oauth:code_verifier")
135+
*/
136+
codeVerifier?: string;
137+
}): Promise<OAuthResult> {
138+
if (typeof window === "undefined" && !opts?.redirectedUrl) {
139+
throw new Error("oauthHandleRedirect is only available in the browser, unless you provide redirectedUrl");
140+
}
141+
if (typeof localStorage === "undefined" && (!opts?.nonce || !opts?.codeVerifier)) {
142+
throw new Error(
143+
"oauthHandleRedirect requires localStorage to be available, unless you provide nonce and codeVerifier"
144+
);
45145
}
46146

47-
const searchParams = new URLSearchParams(window.location.search);
147+
const redirectedUrl = opts?.redirectedUrl ?? window.location.href;
148+
const searchParams = (() => {
149+
try {
150+
return new URL(redirectedUrl).searchParams;
151+
} catch (err) {
152+
throw new Error("Failed to parse redirected URL: " + redirectedUrl);
153+
}
154+
})();
48155

49156
const [error, errorDescription] = [searchParams.get("error"), searchParams.get("error_description")];
50157

@@ -53,17 +160,17 @@ export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<O
53160
}
54161

55162
const code = searchParams.get("code");
56-
const nonce = localStorage.getItem("huggingface.co:oauth:nonce");
163+
const nonce = opts?.nonce ?? localStorage.getItem("huggingface.co:oauth:nonce");
57164

58165
if (!code) {
59-
throw new Error("Missing oauth code from query parameters in redirected URL");
166+
throw new Error("Missing oauth code from query parameters in redirected URL: " + redirectedUrl);
60167
}
61168

62169
if (!nonce) {
63170
throw new Error("Missing oauth nonce from localStorage");
64171
}
65172

66-
const codeVerifier = localStorage.getItem("huggingface.co:oauth:code_verifier");
173+
const codeVerifier = opts?.codeVerifier ?? localStorage.getItem("huggingface.co:oauth:code_verifier");
67174

68175
if (!codeVerifier) {
69176
throw new Error("Missing oauth code_verifier from localStorage");
@@ -119,8 +226,12 @@ export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<O
119226
}).toString(),
120227
});
121228

122-
localStorage.removeItem("huggingface.co:oauth:code_verifier");
123-
localStorage.removeItem("huggingface.co:oauth:nonce");
229+
if (!opts?.codeVerifier) {
230+
localStorage.removeItem("huggingface.co:oauth:code_verifier");
231+
}
232+
if (!opts?.nonce) {
233+
localStorage.removeItem("huggingface.co:oauth:nonce");
234+
}
124235

125236
if (!tokenRes.ok) {
126237
throw await createApiError(tokenRes);
@@ -147,49 +258,12 @@ export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<O
147258
throw await createApiError(userInfoRes);
148259
}
149260

150-
const userInfo: {
151-
sub: string;
152-
name: string;
153-
preferred_username: string;
154-
email_verified?: boolean;
155-
email?: string;
156-
picture: string;
157-
website?: string;
158-
isPro: boolean;
159-
canPay?: boolean;
160-
orgs?: Array<{
161-
sub: string;
162-
name: string;
163-
picture: string;
164-
isEnterprise: boolean;
165-
canPay?: boolean;
166-
roleInOrg?: string;
167-
}>;
168-
} = await userInfoRes.json();
261+
const userInfo: UserInfo = await userInfoRes.json();
169262

170263
return {
171264
accessToken: token.access_token,
172265
accessTokenExpiresAt,
173-
userInfo: {
174-
id: userInfo.sub,
175-
name: userInfo.name,
176-
fullname: userInfo.preferred_username,
177-
email: userInfo.email,
178-
emailVerified: userInfo.email_verified,
179-
avatarUrl: userInfo.picture,
180-
websiteUrl: userInfo.website,
181-
isPro: userInfo.isPro,
182-
orgs:
183-
userInfo.orgs?.map((org) => ({
184-
id: org.sub,
185-
name: org.name,
186-
fullname: org.name,
187-
isEnterprise: org.isEnterprise,
188-
canPay: org.canPay,
189-
avatarUrl: org.picture,
190-
roleInOrg: org.roleInOrg,
191-
})) ?? [],
192-
},
266+
userInfo: userInfo,
193267
state: parsedState.state,
194268
scope: token.scope,
195269
};
@@ -207,12 +281,39 @@ export async function oauthHandleRedirect(opts?: { hubUrl?: string }): Promise<O
207281
*
208282
* Depending on your app, you may want to call {@link oauthHandleRedirect} directly instead.
209283
*/
210-
export async function oauthHandleRedirectIfPresent(opts?: { hubUrl?: string }): Promise<OAuthResult | false> {
211-
if (typeof window === "undefined") {
212-
throw new Error("oauthHandleRedirect is only available in the browser");
284+
export async function oauthHandleRedirectIfPresent(opts?: {
285+
/**
286+
* The URL of the hub. Defaults to {@link HUB_URL}.
287+
*/
288+
hubUrl?: string;
289+
/**
290+
* The URL to analyze.
291+
*
292+
* @default window.location.href
293+
*/
294+
redirectedUrl?: string;
295+
/**
296+
* nonce generated by oauthLoginUrl
297+
*
298+
* @default localStorage.getItem("huggingface.co:oauth:nonce")
299+
*/
300+
nonce?: string;
301+
/**
302+
* codeVerifier generated by oauthLoginUrl
303+
*
304+
* @default localStorage.getItem("huggingface.co:oauth:code_verifier")
305+
*/
306+
codeVerifier?: string;
307+
}): Promise<OAuthResult | false> {
308+
if (typeof window === "undefined" && !opts?.redirectedUrl) {
309+
throw new Error("oauthHandleRedirect is only available in the browser, unless you provide redirectedUrl");
213310
}
214-
215-
const searchParams = new URLSearchParams(window.location.search);
311+
if (typeof localStorage === "undefined" && (!opts?.nonce || !opts?.codeVerifier)) {
312+
throw new Error(
313+
"oauthHandleRedirect requires localStorage to be available, unless you provide nonce and codeVerifier"
314+
);
315+
}
316+
const searchParams = new URLSearchParams(opts?.redirectedUrl ?? window.location.search);
216317

217318
if (searchParams.has("error")) {
218319
return oauthHandleRedirect(opts);

0 commit comments

Comments
 (0)