Reset Github state on auth mismatch (#5179)

ruggi · web-flow · commit f2478cbc4244 · 2024-04-08T12:46:55.000+02:00
* reset the github auth state on mismatch

* toast inside else

* promise.resolve

* unit tests

* no user details actions
diff --git a/editor/src/core/shared/github/github.spec.ts b/editor/src/core/shared/github/github.spec.ts
@@ -0,0 +1,192 @@
+import type { ProjectContentTreeRoot } from '../../../components/assets'
+import type { EditorDispatch } from '../../../components/editor/action-types'
+import { emptyGithubData } from '../../../components/editor/store/editor-state'
+import {
+  getRefreshGithubActions,
+  getUserDetailsFromServer,
+  updateUserDetailsWhenAuthenticated,
+} from './helpers'
+import type { GithubOperationContext } from './operations/github-operation-context'
+
+describe('github helpers', () => {
+  let mockFetch = jest.spyOn(global, 'fetch')
+
+  beforeEach(() => {
+    mockFetch = jest.spyOn(global, 'fetch')
+  })
+  afterEach(() => {
+    mockFetch.mockReset()
+  })
+
+  let mockDispatch: EditorDispatch = () => {}
+
+  describe('getUserDetailsFromServer', () => {
+    it('returns null if the response is not ok', async () => {
+      mockFetch.mockResolvedValue(new Response(null, { status: 418 }))
+      const got = await getUserDetailsFromServer()
+      expect(got).toBe(null)
+    })
+
+    it('returns null if the request returns a failure', async () => {
+      mockFetch.mockResolvedValue(new Response(JSON.stringify({ type: 'FAILURE' })))
+      const got = await getUserDetailsFromServer()
+      expect(got).toBe(null)
+    })
+
+    it('returns the user details if the request succeeds', async () => {
+      mockFetch.mockResolvedValue(
+        new Response(JSON.stringify({ type: 'SUCCESS', user: { id: 'that-guy' } })),
+      )
+      const got = await getUserDetailsFromServer()
+      expect(got).toEqual({ id: 'that-guy' })
+    })
+  })
+
+  describe('updateUserDetailsWhenAuthenticated', () => {
+    it('returns false if the auth check fails', async () => {
+      const got = await updateUserDetailsWhenAuthenticated(mockDispatch, Promise.resolve(false))
+      expect(got).toBe(false)
+    })
+
+    it('returns false if there are no user details', async () => {
+      mockFetch.mockResolvedValue(new Response(null, { status: 418 }))
+      const got = await updateUserDetailsWhenAuthenticated(mockDispatch, Promise.resolve(true))
+      expect(got).toBe(false)
+    })
+
+    it('returns true if the user details are there', async () => {
+      mockFetch.mockResolvedValue(
+        new Response(JSON.stringify({ type: 'SUCCESS', user: { id: 'that-guy' } })),
+      )
+      const got = await updateUserDetailsWhenAuthenticated(mockDispatch, Promise.resolve(true))
+      expect(got).toBe(true)
+    })
+  })
+
+  describe('getRefreshGithubActions', () => {
+    function makeMockContext(
+      fetch?: (url: string, options?: RequestInit) => Promise<Response>,
+    ): GithubOperationContext {
+      return {
+        fetch: (url, options) => (fetch ?? global.fetch)?.(url, options),
+        updateProjectContentsWithParseResults: async (): Promise<ProjectContentTreeRoot> => {
+          return {}
+        },
+      }
+    }
+
+    it('returns actions to reset the GH data if GH is not authenticated', async () => {
+      const got = await getRefreshGithubActions(
+        mockDispatch,
+        false,
+        null,
+        null,
+        null,
+        null,
+        null,
+        null,
+        makeMockContext(),
+      )
+      expect(got.length).toBe(1)
+      const promises = await got[0]
+      expect(promises.length).toBe(1)
+      expect(promises[0].action).toBe('UPDATE_GITHUB_DATA')
+      if (promises[0].action === 'UPDATE_GITHUB_DATA') {
+        expect(promises[0].data).toEqual(emptyGithubData())
+      }
+    })
+
+    it("returns actions to reset the GH data if GH is authenticated but there's no user on the server", async () => {
+      mockFetch.mockResolvedValue(new Response(null, { status: 418 }))
+      const got = await getRefreshGithubActions(
+        mockDispatch,
+        true,
+        null,
+        null,
+        null,
+        null, // <- user details
+        null,
+        null,
+        makeMockContext(),
+      )
+      expect(got.length).toBe(1)
+      const promises = await got[0]
+      expect(promises.length).toBe(2)
+      expect(promises[0].action).toBe('SET_GITHUB_STATE')
+      if (promises[0].action === 'SET_GITHUB_STATE') {
+        expect(promises[0].githubState.authenticated).toBe(false)
+      }
+      expect(promises[1].action).toBe('UPDATE_GITHUB_DATA')
+      if (promises[1].action === 'UPDATE_GITHUB_DATA') {
+        expect(promises[1].data).toEqual(emptyGithubData())
+      }
+    })
+
+    it("returns actions to update the GH data if GH is authenticated and there's a user on the server", async () => {
+      mockFetch.mockResolvedValue(
+        new Response(JSON.stringify({ type: 'SUCCESS', user: { id: 'that-guy' } })),
+      )
+      const got = await getRefreshGithubActions(
+        mockDispatch,
+        true,
+        null,
+        null,
+        null,
+        null, // <- user details
+        null,
+        null,
+        makeMockContext(),
+      )
+      expect(got.length).toBe(1)
+      const promises = await got[0]
+      expect(promises.length).toBe(1)
+      expect(promises[0].action).toBe('UPDATE_GITHUB_DATA')
+      if (promises[0].action === 'UPDATE_GITHUB_DATA') {
+        expect(promises[0].data).toEqual({ githubUserDetails: { id: 'that-guy' } })
+      }
+    })
+
+    it('proceeds with the refresh if the GH data is authenticated and the user is there', async () => {
+      mockFetch.mockImplementation(async () => {
+        return new Response(
+          JSON.stringify({
+            type: 'SUCCESS',
+            user: { id: 'that-guy' },
+            repositories: [],
+          }),
+        )
+      })
+
+      const got = await getRefreshGithubActions(
+        mockDispatch,
+        true,
+        null,
+        null,
+        null,
+        { login: 'the-login', avatarURL: 'the-avatar', htmlURL: 'the-html-url', name: 'the-name' }, // <- user details
+        null,
+        null,
+        makeMockContext(mockFetch.getMockImplementation()),
+      )
+      expect(got.length).toBe(2)
+
+      let promises = await got[0]
+      expect(promises.length).toBe(1)
+      expect(promises[0].action).toBe('UPDATE_GITHUB_DATA')
+      if (promises[0].action === 'UPDATE_GITHUB_DATA') {
+        expect(promises[0].data).toEqual({
+          publicRepositories: [],
+        })
+      }
+
+      promises = await got[1]
+      expect(promises.length).toBe(1)
+      expect(promises[0].action).toBe('UPDATE_GITHUB_DATA')
+      if (promises[0].action === 'UPDATE_GITHUB_DATA') {
+        expect(promises[0].data).toEqual({
+          branches: null,
+        })
+      }
+    })
+  })
+})
diff --git a/editor/src/core/shared/github/helpers.ts b/editor/src/core/shared/github/helpers.ts
@@ -19,6 +19,7 @@ import type { EditorAction, EditorDispatch } from '../../../components/editor/ac
 import {
   deleteFile,
   removeFileConflict,
+  setGithubState,
   showToast,
   updateBranchContents,
   updateFile,
@@ -38,7 +39,6 @@ import type {
 import {
   emptyGithubData,
   emptyGithubSettings,
-  FileChecksums,
   githubOperationPrettyName,
   projectGithubSettings,
 } from '../../../components/editor/store/editor-state'
@@ -184,10 +184,18 @@ export async function runGithubOperation(
     dispatch([updateGithubOperations(operation, 'add')], 'everyone')
     result = await logic(operation)
   } catch (error: any) {
-    dispatch(
-      [showToast(notice(`${opName} failed. See the console for more information.`, 'ERROR'))],
-      'everyone',
-    )
+    let actions: EditorAction[] = []
+
+    const userDetails = await getUserDetailsFromServer()
+    if (userDetails == null) {
+      actions.push(...resetGithubStateAndDataActions())
+    } else {
+      actions.push(
+        showToast(notice(`${opName} failed. See the console for more information.`, 'ERROR')),
+      )
+    }
+
+    dispatch(actions, 'everyone')
     console.error(`[GitHub] operation "${opName}" failed:`, error)
     throw error
   } finally {
@@ -287,7 +295,15 @@ export async function getBranchContentFromServer(
   })
 }
 
-export async function getUserDetailsFromServer(): Promise<Array<EditorAction>> {
+function resetGithubStateAndDataActions(): EditorAction[] {
+  return [setGithubState({ authenticated: false }), updateGithubData(emptyGithubData())]
+}
+
+function updateUserDetailsFromServerActions(userDetails: GithubUser): EditorAction[] {
+  return [updateGithubData({ githubUserDetails: userDetails })]
+}
+
+export async function getUserDetailsFromServer(): Promise<GithubUser | null> {
   const url = GithubEndpoints.userDetails()
 
   const response = await fetch(url, {
@@ -301,19 +317,21 @@ export async function getUserDetailsFromServer(): Promise<Array<EditorAction>> {
     const responseBody: GetGithubUserResponse = await response.json()
     switch (responseBody.type) {
       case 'FAILURE':
-        throw new Error(
+        console.error(
           `Error when attempting to retrieve the user details: ${responseBody.failureReason}`,
         )
+        return null
       case 'SUCCESS':
-        return [updateGithubData({ githubUserDetails: responseBody.user })]
+        return responseBody.user
       default:
         const _exhaustiveCheck: never = responseBody
         throw new Error(`Unhandled response body ${JSON.stringify(responseBody)}`)
     }
   } else {
-    throw new Error(
+    console.error(
       `Github: Unexpected status returned from user details endpoint: ${response.status}`,
     )
+    return null
   }
 }
 
@@ -323,12 +341,20 @@ export async function updateUserDetailsWhenAuthenticated(
   authenticationCheck: Promise<boolean>,
 ): Promise<boolean> {
   const authenticationResult = await authenticationCheck
-  if (authenticationResult) {
-    await dispatchPromiseActions(dispatch, getUserDetailsFromServer()).catch((error) => {
-      console.error(`Error while attempting to retrieve Github user details: ${error}`)
-    })
+  if (!authenticationResult) {
+    return false
+  }
+
+  const userDetails = await getUserDetailsFromServer()
+  if (userDetails == null) {
+    // if the user details are not available while the auth is successful, it means that
+    // there a mismatch between the cookies and the actual auth, so return false overall.
+    dispatch(resetGithubStateAndDataActions())
+    return false
   }
-  return authenticationResult
+
+  dispatch(updateUserDetailsFromServerActions(userDetails))
+  return true
 }
 
 const githubFileChangesSelector = createSelector(
@@ -840,7 +866,7 @@ export const startGithubPolling =
     githubPollingTimeoutId = window.setTimeout(pollGithub, 0)
   }
 
-export async function refreshGithubData(
+export function getRefreshGithubActions(
   dispatch: EditorDispatch,
   githubAuthenticated: boolean,
   githubRepo: GithubRepo | null,
@@ -850,14 +876,20 @@ export async function refreshGithubData(
   previousCommitSha: string | null,
   originCommitSha: string | null,
   operationContext: GithubOperationContext,
-): Promise<void> {
+): Array<Promise<Array<EditorAction>>> {
   // Collect actions which are the results of the various requests,
   // but not those that show which Github operations are running.
-  const promises: Array<Promise<Array<EditorAction>>> = []
-  if (githubAuthenticated) {
-    if (githubUserDetails === null) {
-      promises.push(getUserDetailsFromServer())
-    }
+  let promises: Array<Promise<Array<EditorAction>>> = []
+  if (!githubAuthenticated) {
+    promises.push(Promise.resolve([updateGithubData(emptyGithubData())]))
+  } else if (githubUserDetails === null) {
+    const noUserDetailsActions = getUserDetailsFromServer().then((userDetails) => {
+      return userDetails == null
+        ? resetGithubStateAndDataActions()
+        : updateUserDetailsFromServerActions(userDetails)
+    })
+    promises.push(noUserDetailsActions)
+  } else {
     promises.push(getUsersPublicGithubRepositories(operationContext)(dispatch))
     if (githubRepo != null) {
       promises.push(getBranchesForGithubRepository(operationContext)(dispatch, githubRepo))
@@ -886,10 +918,33 @@ export async function refreshGithubData(
     } else {
       promises.push(Promise.resolve([updateGithubData({ branches: null })]))
     }
-  } else {
-    promises.push(Promise.resolve([updateGithubData(emptyGithubData())]))
   }
 
+  return promises
+}
+
+export async function refreshGithubData(
+  dispatch: EditorDispatch,
+  githubAuthenticated: boolean,
+  githubRepo: GithubRepo | null,
+  branchName: string | null,
+  branchOriginChecksums: FileChecksumsWithFile | null,
+  githubUserDetails: GithubUser | null,
+  previousCommitSha: string | null,
+  originCommitSha: string | null,
+  operationContext: GithubOperationContext,
+): Promise<void> {
+  const promises = await getRefreshGithubActions(
+    dispatch,
+    githubAuthenticated,
+    githubRepo,
+    branchName,
+    branchOriginChecksums,
+    githubUserDetails,
+    previousCommitSha,
+    originCommitSha,
+    operationContext,
+  )
   // Resolve all the promises.
   await Promise.allSettled(promises).then((results) => {
     let actions: Array<EditorAction> = []
