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

feat: fix auth double render #162

Merged
merged 2 commits into from
Nov 10, 2024

Conversation

zugdev
Copy link
Contributor

@zugdev zugdev commented Nov 8, 2024

Resolves #161

const gitHubUser: null | GitHubUser = await getGitHubUser();
  if (gitHubUser) {
    await trackReferralCode();
    await displayGitHubUserInformation(gitHubUser);
  }

  const accessToken = getGitHubAccessToken();
  if (!accessToken) {
    renderGitHubLoginButton();
  }

The problem with this code order is that getGitHubAccessToken() is the function that checks and clears expired tokens. Therefore GitHub user info is shown, and only afterwards we check if expired and show login button. To reproduce the error you can simply change accessToken to null:

 const accessToken = null;

Though I'd like to simplify auth sometime it's quicker to just switch the order back into correct place. I think we should increase our token expiry date though, 60 mins seems very short.

Here is the reproduced bug with a null accessToken:

currentCode.mp4

To QA this solution one can comment the following lines

//  if (accessToken) {
//    return accessToken;
//  }

in getGitHubAccessToken():

export async function getGitHubAccessToken(): Promise<string | null> {
// better to use official function, looking up localstorage has flaws
const oauthToken = await checkSupabaseSession();
const expiresAt = oauthToken?.expires_at;
if (expiresAt) {
if (expiresAt < Date.now() / 1000) {
localStorage.removeItem(`sb-${SUPABASE_STORAGE_KEY}-auth-token`);
return null;
}
}
const accessToken = oauthToken?.provider_token;
if (accessToken) {
return accessToken;
}
return null;
}

and run with a clean cache. Whenever you login, GitHub info will never be shown.

@zugdev zugdev requested a review from 0x4007 as a code owner November 8, 2024 02:14
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Nov 8, 2024

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was some reason I changed the order I forgot the bug. I suppose you don't recall either.

Wasn't it throwing the error in the modal due to this?

@0x4007 0x4007 merged commit b5a84f7 into ubiquity:development Nov 10, 2024
1 of 2 checks passed
@0x4007
Copy link
Member

0x4007 commented Nov 10, 2024

Also let's change the token expiry time to max

@zugdev
Copy link
Contributor Author

zugdev commented Nov 10, 2024

Wasn't it throwing the error in the modal due to this?

No that was service worker related.

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.

do not display user info if access token is expired
2 participants