-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix 51888 cors errors are displayed for attachments #53407
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
e8c5c33
b3371ae
28f7a27
433717d
88cf61a
be7f93b
f797f2f
bc0c48b
c7aab35
e5d504d
170e7e8
7ba9f8a
d25f0b0
65fb38c
d9cbf9f
3040f19
cdf9209
bffc444
b9f1240
93a3c05
873dcec
5cbae41
ae15c5b
ee77c8d
bbe9e2e
80fff80
1c7807a
4174956
528a03c
c66c460
7cb8b5f
fe93bc8
c9dc455
d78885f
f026b07
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 |
|---|---|---|
| @@ -1,14 +1,17 @@ | ||
| import React, {useCallback, useContext, useMemo, useState} from 'react'; | ||
| import {withOnyx} from 'react-native-onyx'; | ||
| import React, {useCallback, useContext, useEffect, useMemo, useRef, useState} from 'react'; | ||
| import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator'; | ||
| import {useSession} from '@components/OnyxProvider'; | ||
| import {isExpiredSession} from '@libs/actions/Session'; | ||
| import activateReauthenticator from '@libs/actions/Session/AttachmentImageReauthenticator'; | ||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import BaseImage from './BaseImage'; | ||
| import {ImageBehaviorContext} from './ImageBehaviorContextProvider'; | ||
| import type {ImageOnLoadEvent, ImageOnyxProps, ImageOwnProps, ImageProps} from './types'; | ||
| import type {ImageOnLoadEvent, ImageProps} from './types'; | ||
|
|
||
| function Image({source: propsSource, isAuthTokenRequired = false, session, onLoad, objectPosition = CONST.IMAGE_OBJECT_POSITION.INITIAL, style, ...forwardedProps}: ImageProps) { | ||
| function Image({source: propsSource, isAuthTokenRequired = false, onLoad, objectPosition = CONST.IMAGE_OBJECT_POSITION.INITIAL, style, ...forwardedProps}: ImageProps) { | ||
| const [aspectRatio, setAspectRatio] = useState<string | number | null>(null); | ||
| const isObjectPositionTop = objectPosition === CONST.IMAGE_OBJECT_POSITION.TOP; | ||
| const session = useSession(); | ||
|
|
||
| const {shouldSetAspectRatioInStyle} = useContext(ImageBehaviorContext); | ||
|
|
||
|
|
@@ -37,6 +40,49 @@ function Image({source: propsSource, isAuthTokenRequired = false, session, onLoa | |
| }, | ||
| [onLoad, updateAspectRatio], | ||
| ); | ||
|
|
||
| // accepted sessions are sessions of a certain criteria that we think can necessitate a reload of the images | ||
| // because images sources barely changes unless specific events occur like network issues (offline/online) per example. | ||
| // Here we target new session received less than 60s after the previous session (that could be from fresh reauthentication, the previous session was not necessarily expired) | ||
| // or new session after the previous session was expired (based on timestamp gap between the 2 creationDate and the freshness of the new session). | ||
| const isAcceptedSession = useCallback((sessionCreationDateDiff: number, sessionCreationDate: number) => { | ||
| return sessionCreationDateDiff < 60000 || (sessionCreationDateDiff >= CONST.SESSION_EXPIRATION_TIME_MS && new Date().getTime() - sessionCreationDate < 60000); | ||
| }, []); | ||
|
|
||
| /** | ||
| * trying to figure out if the current session is expired or fresh from a necessary reauthentication | ||
| */ | ||
| const previousSessionAge = useRef<number | undefined>(); | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| const validSessionAge: number | undefined = useMemo(() => { | ||
| // Authentication is required only for certain types of images (attachments and receipts), | ||
| // so we only calculate the session age for those | ||
| if (!isAuthTokenRequired) { | ||
| return undefined; | ||
| } | ||
| if (session?.creationDate) { | ||
| if (previousSessionAge.current) { | ||
| // Most likely a reauthentication happened, but unless the calculated source is different from the previous, the image won't reload | ||
| if (isAcceptedSession(session.creationDate - previousSessionAge.current, session.creationDate)) { | ||
| return session.creationDate; | ||
| } | ||
| return previousSessionAge.current; | ||
| } | ||
| if (isExpiredSession(session.creationDate)) { | ||
| // reset the countdown to now so future sessions creationDate can be compared to that time | ||
| return new Date().getTime(); | ||
| } | ||
| return session.creationDate; | ||
| } | ||
| return undefined; | ||
| }, [session, isAuthTokenRequired, isAcceptedSession]); | ||
| useEffect(() => { | ||
| if (!isAuthTokenRequired) { | ||
| return; | ||
| } | ||
| previousSessionAge.current = validSessionAge; | ||
| }); | ||
|
|
||
| /** | ||
| * Check if the image source is a URL - if so the `encryptedAuthToken` is appended | ||
| * to the source. | ||
|
|
@@ -48,24 +94,44 @@ function Image({source: propsSource, isAuthTokenRequired = false, session, onLoa | |
| } | ||
| const authToken = session?.encryptedAuthToken ?? null; | ||
| if (isAuthTokenRequired && authToken) { | ||
| return { | ||
| ...propsSource, | ||
| headers: { | ||
| [CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken, | ||
| }, | ||
| }; | ||
| if (!!session?.creationDate && !isExpiredSession(session.creationDate)) { | ||
| return { | ||
| ...propsSource, | ||
| headers: { | ||
| [CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken, | ||
| }, | ||
| }; | ||
| } | ||
| if (session) { | ||
| activateReauthenticator(session); | ||
| } | ||
| return undefined; | ||
| } | ||
| } | ||
| return propsSource; | ||
| // The session prop is not required, as it causes the image to reload whenever the session changes. For more information, please refer to issue #26034. | ||
| // but we still need the image to reload sometimes (example : when the current session is expired) | ||
| // by forcing a recalculation of the source (which value could indeed change) through the modification of the variable validSessionAge | ||
| // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps | ||
| }, [propsSource, isAuthTokenRequired]); | ||
| }, [propsSource, isAuthTokenRequired, validSessionAge]); | ||
rlinoz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| useEffect(() => { | ||
| if (!isAuthTokenRequired || source !== undefined) { | ||
| return; | ||
| } | ||
| forwardedProps?.waitForSession?.(); | ||
| }, [source, isAuthTokenRequired, forwardedProps]); | ||
|
|
||
| /** | ||
| * If the image fails to load and the object position is top, we should hide the image by setting the opacity to 0. | ||
| */ | ||
| const shouldOpacityBeZero = isObjectPositionTop && !aspectRatio; | ||
|
|
||
| if (source === undefined && !!forwardedProps?.waitForSession) { | ||
| return undefined; | ||
| } | ||
| if (source === undefined) { | ||
| return <FullScreenLoadingIndicator />; | ||
|
Contributor
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. Loading indicator has |
||
| } | ||
| return ( | ||
| <BaseImage | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
|
|
@@ -77,18 +143,11 @@ function Image({source: propsSource, isAuthTokenRequired = false, session, onLoa | |
| ); | ||
| } | ||
|
|
||
| function imagePropsAreEqual(prevProps: ImageOwnProps, nextProps: ImageOwnProps) { | ||
| function imagePropsAreEqual(prevProps: ImageProps, nextProps: ImageProps) { | ||
| return prevProps.source === nextProps.source; | ||
| } | ||
|
|
||
| const ImageWithOnyx = React.memo( | ||
| withOnyx<ImageProps, ImageOnyxProps>({ | ||
| session: { | ||
| key: ONYXKEYS.SESSION, | ||
| }, | ||
| })(Image), | ||
| imagePropsAreEqual, | ||
| ); | ||
| const ImageWithOnyx = React.memo(Image, imagePropsAreEqual); | ||
|
|
||
| ImageWithOnyx.displayName = 'Image'; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,6 +234,14 @@ function Lightbox({isAuthTokenRequired = false, uri, onScaleChanged: onScaleChan | |
| updateContentSize(e); | ||
| setLightboxImageLoaded(true); | ||
| }} | ||
| waitForSession={() => { | ||
| // only active lightbox should call this function | ||
|
Contributor
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. Can we explain why in the comment please?
Contributor
Author
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. ok |
||
| if (!isActive || isFallbackVisible || !isLightboxVisible) { | ||
| return; | ||
| } | ||
| setContentSize(cachedImageDimensions.get(uri)); | ||
| setLightboxImageLoaded(false); | ||
| }} | ||
| /> | ||
| </MultiGestureCanvas> | ||
| </View> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,6 +111,15 @@ function TestToolMenu() { | |
| /> | ||
| </TestToolRow> | ||
|
|
||
| {/* Sends an expired session to the FE and invalidates the session by the same time in the BE. Action is delayed for 15s */} | ||
| <TestToolRow title={translate('initialSettingsPage.troubleshoot.authenticationStatus')}> | ||
| <Button | ||
| small | ||
| text={translate('initialSettingsPage.troubleshoot.invalidateWithDelay')} | ||
| onPress={() => Session.expireSessionWithDelay()} | ||
| /> | ||
| </TestToolRow> | ||
|
Comment on lines
+115
to
+121
Contributor
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. We need to adjust the existing style when introducing this test tool (because the button contains a long text), otherwise, the button text will be clipped. More details here
Contributor
Author
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. I suggested a shorter but meaningful title but i had to follow the C+ lead #53407 (comment). Thanks for the information |
||
|
|
||
| <TestCrash /> | ||
| </> | ||
| ); | ||
|
|
||
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.
We have a hook called
usePrevious, let use it.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.
ok i'll check that
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.
Did we give up on using
usePrevious?