PCF-400 Query https://firefox-ci-tc.services.mozilla.com/login/oauth/credentials to verify token and request credentials #648

esanuandra · 2024-05-13T16:14:41Z

This PR fetches the Taskcluster credentials (with an access token that has expiration date) using access token Bearer.

netlify · 2024-05-13T16:14:56Z

✅ Deploy Preview for mozilla-perfcompare ready!

Name	Link
🔨 Latest commit	`f69924a`
🔍 Latest deploy log	https://app.netlify.com/sites/mozilla-perfcompare/deploys/66632a93341a460008310976
😎 Deploy Preview	https://deploy-preview-648--mozilla-perfcompare.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-05-13T16:17:30Z

Codecov Report

Attention: Patch coverage is 74.41860% with 11 lines in your changes missing coverage. Please review.

Project coverage is 91.41%. Comparing base (d13b29d) to head (f69924a).

Files	Patch %	Lines
src/logic/credentials-storage.ts	50.00%	9 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             beta     #648      +/-   ##
==========================================
- Coverage   91.80%   91.41%   -0.39%     
==========================================
  Files          68       69       +1     
  Lines        1587     1620      +33     
  Branches      285      291       +6     
==========================================
+ Hits         1457     1481      +24     
- Misses        104      111       +7     
- Partials       26       28       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

julienw

hey andra, thanks for this work!
here are a few comments, I hope they make sense.

In addition to the comments on the code itself, there's another missing bit IMO.

Here is the current workflow:

The user is in tab A and clicks on the "login" button in the modal
open a new tab B to taskcluster
in tab B the user logs in on task cluster
in tab B taskcluster redirects on our callback
in tab B perfcompare requests and retrieves the access token to the taskcluster server
in tab B perfcompare requests and retrieves the user credentials to the taskcluster server
in tab B perfcompare stores the token to localStorage
... the missing link

For step 8, we can add an event listener on the "storage" event in tab A so that we're notified with the user credentials change. We can add it in step 1.
(see MDN: https://developer.mozilla.org/en-US/docs/Web/API/Window/storage_event)

I'm thinking that if step 5 stores the access token to the localStorage, then step 6-7 could be done in tab A... it might be a better design actually, because (as I write below in another comment) in the future I'd like that we don't need to open a tab if we already have an unexpired access token.

So here is how the workflow could look like (bold for changes):

The user is in tab A and clicks on the "login" button in the modal.
adds an event listener for "storage", and opens a new tab B to taskcluster
in tab B the user logs in on task cluster
in tab B taskcluster redirects on our callback
in tab B perfcompare requests and retrieves the access token to the taskcluster server
in tab B perfcompare stores the access token to localStorage, then closes the tab with window.close
in tab A, perfcompare is notified from the "storage" event.
in tab A perfcompare requests and retrieves the user credentials to the taskcluster server
in tab A perfcompare stores the token to localStorage
in tab A perfcompare does the retrigger request

I don't request to do this workflow change in this PR, but it could be good to do it in a next PR, if you don't mind.

Tell me what you think (and if I'm missing anything)!

julienw · 2024-05-22T12:32:23Z

src/logic/taskcluster.ts

+  };
+
+  // fetch Taskcluster credentials using token Bearer
+  const response = await fetchData(


fetchData returns Promise but my understanding is that the object returned here is different, with different properties, so you need a different type than ResponseToken.

See above my suggestion about how to change fetchData, that should make it easier to control the return value.

The type for the user credentials will be necessary as well when we'll get the data from the localStorage (cf my other comment about creating a separate file).

src/components/TaskclusterAuth/loader.ts

julienw · 2024-05-22T12:36:11Z

src/components/TaskclusterAuth/loader.ts


-  return tokenBearer;
+  sessionStorage.setItem('userCredentials', JSON.stringify(accessToken));


this one should be in localStorage because we want to keep it for a longer time, not just for the duration of the browser session.

I'm thinking it could be good to encapsulate this logic in a separate file, so that we can type inputs and outputs.

This could be a file src/logic/credentials-storage.ts (for example) with functions such as:

function storeUserCredentials(UserCredentials credentials) { localStorage.setItem('userCredentials', JSON.stringify(credentials)); } function retrieveUserCredentials(): UserCredentials { return JSON.parse(localStorage.getItem('userCredentials')); } // same for storeTaskclusterToken and retrieveTaskclusterToken

I'm thinking these functions could be in the existing file logic/taskcluster.ts too, as you wish.

julienw · 2024-05-22T12:46:57Z

src/logic/taskcluster.ts

 interface RequestOptions {
-  method: string;
-  body: URLSearchParams;
+  method?: string;
+  body?: URLSearchParams;
  headers: {
+    Authorization?: string;
    'Content-Type': string;
  };
 }


Instead of having fetchData doing the request, my suggestion would be to have a function to check error cases:

function checkTaskclusterResponse(response: Response) { if (!response.ok) { if (response.status === 400) { throw new Error( `Error when requesting Taskcluster: ${await response.text()}`, ); } else { throw new Error( `Error when requesting Taskcluster: (${response.status}) ${response.statusText}`, ); } }}

Then you can move the rest of the function (fetch call + call to json()) back to retrieveTaskclusterToken and retrieveTaskclusterAccessToken, and you don't need to have this RequestOptions type anymore.

julienw · 2024-05-22T12:47:49Z

src/logic/taskcluster.ts

@@ -133,3 +134,23 @@ export async function retrieveTaskclusterToken(rootUrl: string, code: string) {

  return response;
 }
+
+export async function retrieveTaskclusterAccessToken(


nit: please call this retrieveTaskclusterUserCredentials so that it's better distinguished with the other function.

julienw · 2024-05-22T12:54:01Z

src/__tests__/TaskclusterAuth/TaskclusterCallback.test.tsx

+    });
+    mockedGetLocationOrigin.mockImplementation(() => 'http://localhost:3000');
+
+    expect(window.fetch).toHaveBeenLastCalledWith(


The test problem is that we're not waiting enough in this test. Indeed the test is synchronous but the code is asynchronous, so the second fetch call hasn't happened yet!
The fix is to wait for the rendering.
For example you can add:

await screen.findByText(/Getting Taskcluster credentials/);

As you see the component will be displayed only after the loader is completely rendered. If you want it to render before and change the text, you could use this guide: https://reactrouter.com/en/main/guides/deferred
But I think this is for a later PR, let's make it work end to end first!

julienw · 2024-05-28T18:55:21Z

src/types/types.ts

  string,
  { expires: string; credentials: { clientId: string; accessToken: string } }
 >;
+
+export type TokenBearer = Record<
+  string,
+  { access_token: string; token_type: 'Bearer' }
+>;


I think these 2 types are incorrect, they're not Record<string, XXX>, but directly XXX.

export type UserCredentials = { expires: string; credentials: { clientId: string; accessToken: string } }; export type TokenBearer = { access_token: string; token_type: 'Bearer' };

A Record is like a Map (but using an object), also called a dictionary. This isn't what it is here.

I just noticed that the Treeherder implementation stores the userCredentials in this way using the url.

userCredentials "https://firefox-ci-tc.services.mozilla.com": { expires: ... credentials: { clientId: ... accessToken: ... } }

I believe that's why the Record for userCredentials. Should we still go with removing the Record for userCredentials?

julienw

Thanks!
here are a few more comments to fix before landing, thanks again for your work Andra

src/types/types.ts

src/logic/token-storage.ts

src/logic/taskcluster.ts

src/logic/token-storage.ts

src/__tests__/TaskclusterAuth/TaskclusterCallback.test.tsx

julienw

Looking at this with fresh eyes, here are some last minute changes, sorry about that.

Also have you seen my longer comment in #648 (review) ? Any comment about that? Thanks

julienw · 2024-06-03T10:12:31Z

src/components/TaskclusterAuth/loader.ts

+    taskclusterCode,
+  );
+
+  storeToken({ [tokenResponse.token_type]: tokenResponse });


This should look like this:

Suggested change

storeToken({ [tokenResponse.token_type]: tokenResponse });

storeToken({ [rootUrl]: tokenResponse });

but please look at my other comments as well, as I believe the signature will be changed and therefore this will look like this:

Suggested change

storeToken({ [tokenResponse.token_type]: tokenResponse });

storeToken(rootUrl, tokenResponse);

julienw · 2024-06-03T10:17:57Z

src/logic/credentials-storage.ts

+export function storeUserCredentials(credentials: UserCredentials) {
+  localStorage.setItem('userCredentials', JSON.stringify(credentials));
+}


The goal for storing a Record is that we can add separate entries for different URLs. Therefore I believe the implementation should probably look lile this:

Suggested change

export function storeUserCredentials(credentials: UserCredentials) {

localStorage.setItem('userCredentials', JSON.stringify(credentials));

}

export function storeUserCredentials(rootUrl: string, credentials: UserCredentialsResponse) {

const allCredentialsAsString = localStorage.userCredentials;

const allCredentials = (allCredentialsAsString ? JSON.parse(allCredentialsAsString) : {}) as UserCredentials;

allCredentials[rootUrl] = credentials;

localStorage.userCredentials = JSON.stringify(allCredentials);

}

Similar for for storeToken.

julienw · 2024-06-03T10:21:08Z

src/logic/credentials-storage.ts

+export function retrieveUserCredentials(): UserCredentials {
+  return JSON.parse(
+    localStorage.getItem('userCredentials') as string,
+  ) as UserCredentials;
+}


Likewise, the retrieve functions could look lile this:

Suggested change

export function retrieveUserCredentials(): UserCredentials {

return JSON.parse(

localStorage.getItem('userCredentials') as string,

) as UserCredentials;

}

export function retrieveUserCredentials(rootUrl: string): UserCredentialsResponse | null {

const allCredentialsAsString = localStorage.userCredentials;

if (!allCredentialsAsString) {

return null;

}

const allCredentials = JSON.parse(allCredentialsAsString) as UserCredentials;

return allCredentials[rootUrl] ?? null;

}

and similarly for the other retrieve function.

julienw

Let's land this and move forward!

julienw · 2024-06-05T15:52:30Z

src/logic/credentials-storage.ts

+  if (allCredentialsAsString) return null;
+
+  const allCredentials = JSON.parse(allCredentialsAsString) as UserCredentials;
+  return allCredentials[rootUrl];


nit

Suggested change

return allCredentials[rootUrl];

return allCredentials[rootUrl] ?? null;

(same as above)

julienw · 2024-06-05T15:53:12Z

src/logic/credentials-storage.ts

+  if (allUserTokensAsString) return null;
+
+  const allTokens = JSON.parse(allUserTokensAsString) as UserToken;
+  return allTokens[rootUrl];


nit

Suggested change

return allTokens[rootUrl];

return allTokens[rootUrl] ?? null;

otherwise that can return undefined -- typescript doesn't check this by default.
Not a super big deal but let's not lie to typescript.

julienw · 2024-06-05T16:27:57Z

src/types/types.ts

+export type TokenBearerResponse = {
+  access_token: string;
+  token_type: 'Bearer';
+};
+
+export type TokenBearer = Record<string, TokenBearerResponse>;
+
+export type TokenResponse = TokenBearerResponse;
+
+export type UserToken = Record<string, TokenResponse>;


it looks like you have the same types twice:
TokenBearerResponse == TokenResponse
TokenBearer == UserToken

Please pick just one pair (I'd suggest TokenBearerResponse / TokenBearer)

Optional nit: it would be good to move the directionary types to credentials-storage.ts because they're used only there, they're not global.

Optional nit: Rename:

CredentialsResponse => UserCredentials
UserCredentials => UserCredentialsDictionary
TokenBearerResponse => TokenBearer
TokenBearer => TokenBearerDictionary

(with the Dictionary types in credentials-storage.ts as suggested above, and the other types kept here)

Thank you! I was confused about how to keep them.

esanuandra added the 🚧 WIP 🚧 Work in progress: do not merge label May 13, 2024

esanuandra force-pushed the fetch-taskcluster-credentials branch from ebef7d2 to 5fd086b Compare May 21, 2024 14:53

esanuandra removed the 🚧 WIP 🚧 Work in progress: do not merge label May 21, 2024

esanuandra force-pushed the fetch-taskcluster-credentials branch from 5fd086b to e3642f7 Compare May 21, 2024 14:59

esanuandra requested a review from julienw May 22, 2024 08:16

julienw reviewed May 22, 2024

View reviewed changes

julienw reviewed May 28, 2024

View reviewed changes

esanuandra force-pushed the fetch-taskcluster-credentials branch from 2a43d43 to 9ad5597 Compare May 29, 2024 14:38

esanuandra requested a review from julienw May 29, 2024 14:43

esanuandra added the Ready For Review label May 29, 2024

julienw reviewed May 30, 2024

View reviewed changes

esanuandra force-pushed the fetch-taskcluster-credentials branch from 84707cf to d12b2e2 Compare May 31, 2024 14:20

esanuandra requested a review from julienw June 3, 2024 08:05

julienw reviewed Jun 3, 2024

View reviewed changes

esanuandra force-pushed the fetch-taskcluster-credentials branch from 850c7ff to cb68695 Compare June 5, 2024 14:10

julienw approved these changes Jun 5, 2024

View reviewed changes

esanuandra force-pushed the fetch-taskcluster-credentials branch from 0775dd8 to 09bdd23 Compare June 6, 2024 15:11

esanuandra added 11 commits June 7, 2024 18:43

fetch taskcluster credentials

5b18bc7

Add tests

67893d7

Fix unit test with findByText

a70d253

Rename function to retrieveTaskclusterUserCredentials

5888d91

Remove fetchData and RequestOptions

b176575

Save userCredentials to localStorage

42b8450

Encapsulate logic for userCredentials

68e9d73

Encapsulate storing for tokenBearer

4321ec3

Fix type for UserCredentials

3c3533e

Fix lint

5f626cb

Address nit changes

7f6c67f

esanuandra added 5 commits June 7, 2024 18:43

Fix unit test

cca9dc0

Refactor store/retrieveUserCredentials

1c37f8a

Refactor store/retrieveUserToken

b2d80dd

Fix test

b94c2c3

Rename types for token and usercredentials

f69924a

esanuandra force-pushed the fetch-taskcluster-credentials branch from 09bdd23 to f69924a Compare June 7, 2024 15:43

esanuandra merged commit e3f14f5 into mozilla:beta Jun 7, 2024
8 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PCF-400 Query https://firefox-ci-tc.services.mozilla.com/login/oauth/credentials to verify token and request credentials #648

PCF-400 Query https://firefox-ci-tc.services.mozilla.com/login/oauth/credentials to verify token and request credentials #648

esanuandra commented May 13, 2024 •

edited

Loading

netlify bot commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

julienw left a comment •

edited

Loading

julienw May 22, 2024

julienw May 22, 2024

julienw May 22, 2024

julienw May 22, 2024

julienw May 22, 2024

julienw May 22, 2024

julienw May 28, 2024

esanuandra May 29, 2024

julienw left a comment

julienw left a comment

julienw Jun 3, 2024

julienw Jun 3, 2024

julienw Jun 3, 2024

julienw left a comment

julienw Jun 5, 2024

julienw Jun 5, 2024

julienw Jun 5, 2024

esanuandra Jun 6, 2024


		return tokenBearer;
		sessionStorage.setItem('userCredentials', JSON.stringify(accessToken));

	storeToken({ [tokenResponse.token_type]: tokenResponse });
	storeToken({ [rootUrl]: tokenResponse });

	storeToken({ [tokenResponse.token_type]: tokenResponse });
	storeToken(rootUrl, tokenResponse);

-export function storeUserCredentials(credentials: UserCredentials) {
-  localStorage.setItem('userCredentials', JSON.stringify(credentials));
-}
+export function storeUserCredentials(rootUrl: string, credentials: UserCredentialsResponse) {
+  const allCredentialsAsString = localStorage.userCredentials;
+  const allCredentials = (allCredentialsAsString ? JSON.parse(allCredentialsAsString) : {}) as UserCredentials;
+  allCredentials[rootUrl] = credentials;
+  localStorage.userCredentials = JSON.stringify(allCredentials);
+}

	return allCredentials[rootUrl];
	return allCredentials[rootUrl] ?? null;

	return allTokens[rootUrl];
	return allTokens[rootUrl] ?? null;

PCF-400 Query https://firefox-ci-tc.services.mozilla.com/login/oauth/credentials to verify token and request credentials #648

PCF-400 Query https://firefox-ci-tc.services.mozilla.com/login/oauth/credentials to verify token and request credentials #648

Conversation

esanuandra commented May 13, 2024 • edited Loading

netlify bot commented May 13, 2024 • edited Loading

✅ Deploy Preview for mozilla-perfcompare ready!

codecov bot commented May 13, 2024 • edited Loading

Codecov Report

julienw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw left a comment

Choose a reason for hiding this comment

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esanuandra commented May 13, 2024 •

edited

Loading

netlify bot commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

julienw left a comment •

edited

Loading