-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix login issue #9012
Fix login issue #9012
Changes from 4 commits
63c7978
17fd7e1
ec11bb3
0388857
2cb60ee
16981b8
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,59 +1,35 @@ | ||
import { useEffect } from 'react'; | ||
import { useRecoilCallback, useRecoilValue } from 'recoil'; | ||
import { useRecoilValue } from 'recoil'; | ||
|
||
import { useIsLogged } from '@/auth/hooks/useIsLogged'; | ||
import { currentUserState } from '@/auth/states/currentUserState'; | ||
import { currentWorkspaceState } from '@/auth/states/currentWorkspaceState'; | ||
import { useFindManyObjectMetadataItems } from '@/object-metadata/hooks/useFindManyObjectMetadataItems'; | ||
import { objectMetadataItemsState } from '@/object-metadata/states/objectMetadataItemsState'; | ||
import { useLoadMockedObjectMetadataItems } from '@/object-metadata/hooks/useLoadMockedObjectMetadataItems'; | ||
import { useRefreshObjectMetadataItems } from '@/object-metadata/hooks/useRefreshObjectMetadataItem'; | ||
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. syntax: typo in import path - 'useRefreshObjectMetadataItem' should be 'useRefreshObjectMetadataItems' to match the actual hook name |
||
import { WorkspaceActivationStatus } from '~/generated/graphql'; | ||
import { generatedMockObjectMetadataItems } from '~/testing/mock-data/generatedMockObjectMetadataItems'; | ||
import { isDeeplyEqual } from '~/utils/isDeeplyEqual'; | ||
import { isUndefinedOrNull } from '~/utils/isUndefinedOrNull'; | ||
|
||
export const ObjectMetadataItemsLoadEffect = () => { | ||
const currentUser = useRecoilValue(currentUserState); | ||
const currentWorkspace = useRecoilValue(currentWorkspaceState); | ||
const isLoggedIn = useIsLogged(); | ||
|
||
const { | ||
objectMetadataItems: newObjectMetadataItems, | ||
loading: isObjectMetadataLoading, | ||
} = useFindManyObjectMetadataItems({ | ||
skip: !isLoggedIn, | ||
}); | ||
|
||
const updateObjectMetadataItems = useRecoilCallback( | ||
({ set, snapshot }) => | ||
() => { | ||
const toSetObjectMetadataItems = | ||
isUndefinedOrNull(currentUser) || | ||
currentWorkspace?.activationStatus !== | ||
WorkspaceActivationStatus.Active | ||
? generatedMockObjectMetadataItems | ||
: newObjectMetadataItems; | ||
|
||
if ( | ||
!isObjectMetadataLoading && | ||
!isDeeplyEqual( | ||
snapshot.getLoadable(objectMetadataItemsState).getValue(), | ||
toSetObjectMetadataItems, | ||
) | ||
) { | ||
set(objectMetadataItemsState, toSetObjectMetadataItems); | ||
} | ||
}, | ||
[ | ||
currentUser, | ||
currentWorkspace?.activationStatus, | ||
isObjectMetadataLoading, | ||
newObjectMetadataItems, | ||
], | ||
); | ||
const { refreshObjectMetadataItems } = useRefreshObjectMetadataItems(); | ||
const { loadMockedObjectMetadataItems } = useLoadMockedObjectMetadataItems(); | ||
|
||
useEffect(() => { | ||
updateObjectMetadataItems(); | ||
}, [updateObjectMetadataItems]); | ||
if ( | ||
isUndefinedOrNull(currentUser) || | ||
currentWorkspace?.activationStatus !== WorkspaceActivationStatus.Active | ||
) { | ||
loadMockedObjectMetadataItems(); | ||
} else { | ||
refreshObjectMetadataItems(); | ||
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. logic: refreshObjectMetadataItems is an async function but the await is missing here |
||
} | ||
}, [ | ||
currentUser, | ||
currentWorkspace?.activationStatus, | ||
loadMockedObjectMetadataItems, | ||
refreshObjectMetadataItems, | ||
]); | ||
|
||
return <></>; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { objectMetadataItemsState } from '@/object-metadata/states/objectMetadataItemsState'; | ||
import { useRecoilCallback } from 'recoil'; | ||
import { generatedMockObjectMetadataItems } from '~/testing/mock-data/generatedMockObjectMetadataItems'; | ||
import { isDeeplyEqual } from '~/utils/isDeeplyEqual'; | ||
|
||
export const useLoadMockedObjectMetadataItems = () => { | ||
const loadMockedObjectMetadataItems = useRecoilCallback( | ||
({ set, snapshot }) => | ||
() => { | ||
if ( | ||
!isDeeplyEqual( | ||
snapshot.getLoadable(objectMetadataItemsState).getValue(), | ||
generatedMockObjectMetadataItems, | ||
Comment on lines
+12
to
+13
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. style: getLoadable().getValue() may throw if atom is in error state. Consider using try/catch. |
||
) | ||
) { | ||
set(objectMetadataItemsState, generatedMockObjectMetadataItems); | ||
} | ||
}, | ||
[], | ||
); | ||
return { | ||
loadMockedObjectMetadataItems, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { FIND_MANY_OBJECT_METADATA_ITEMS } from '@/object-metadata/graphql/queries'; | ||
import { useApolloMetadataClient } from '@/object-metadata/hooks/useApolloMetadataClient'; | ||
import { objectMetadataItemsState } from '@/object-metadata/states/objectMetadataItemsState'; | ||
import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem'; | ||
import { mapPaginatedObjectMetadataItemsToObjectMetadataItems } from '@/object-metadata/utils/mapPaginatedObjectMetadataItemsToObjectMetadataItems'; | ||
import { useRecoilCallback } from 'recoil'; | ||
import { ObjectMetadataItemsQuery } from '~/generated-metadata/graphql'; | ||
import { isDeeplyEqual } from '~/utils/isDeeplyEqual'; | ||
|
||
export const useRefreshObjectMetadataItems = () => { | ||
const client = useApolloMetadataClient(); | ||
|
||
const refreshObjectMetadataItems = async () => { | ||
const result = await client.query<ObjectMetadataItemsQuery>({ | ||
query: FIND_MANY_OBJECT_METADATA_ITEMS, | ||
variables: {}, | ||
}); | ||
Comment on lines
+14
to
+17
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. logic: Missing error handling for failed GraphQL queries. Add try/catch block.
Comment on lines
+14
to
+17
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. style: Consider adding fetchPolicy to prevent stale cache data during refresh |
||
|
||
const objectMetadataItems = | ||
mapPaginatedObjectMetadataItemsToObjectMetadataItems({ | ||
pagedObjectMetadataItems: result.data, | ||
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. logic: result.data could be undefined here, causing runtime error |
||
}); | ||
|
||
replaceObjectMetadataItemIfDifferent(objectMetadataItems); | ||
}; | ||
|
||
const replaceObjectMetadataItemIfDifferent = useRecoilCallback( | ||
({ set, snapshot }) => | ||
(toSetObjectMetadataItems: ObjectMetadataItem[]) => { | ||
if ( | ||
!isDeeplyEqual( | ||
snapshot.getLoadable(objectMetadataItemsState).getValue(), | ||
toSetObjectMetadataItems, | ||
) | ||
) { | ||
set(objectMetadataItemsState, toSetObjectMetadataItems); | ||
} | ||
}, | ||
[], | ||
); | ||
return { | ||
refreshObjectMetadataItems, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ export class FileController { | |
@Req() req: Request, | ||
) { | ||
const folderPath = checkFilePath(params[0]); | ||
|
||
const filename = checkFilename(params['filename']); | ||
|
||
const workspaceId = (req as any)?.workspaceId; | ||
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. logic: Unsafe type assertion. Consider proper typing of the Request object or middleware to ensure workspaceId exists |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,35 +12,35 @@ import { FileUpload, GraphQLUpload } from 'graphql-upload'; | |
|
||
import { FileFolder } from 'src/engine/core-modules/file/interfaces/file-folder.interface'; | ||
|
||
import { LoginTokenService } from 'src/engine/core-modules/auth/token/services/login-token.service'; | ||
import { BillingSubscription } from 'src/engine/core-modules/billing/entities/billing-subscription.entity'; | ||
import { BillingSubscriptionService } from 'src/engine/core-modules/billing/services/billing-subscription.service'; | ||
import { DomainManagerService } from 'src/engine/core-modules/domain-manager/service/domain-manager.service'; | ||
import { EnvironmentService } from 'src/engine/core-modules/environment/environment.service'; | ||
import { FileUploadService } from 'src/engine/core-modules/file/file-upload/services/file-upload.service'; | ||
import { FileService } from 'src/engine/core-modules/file/services/file.service'; | ||
import { UserWorkspaceService } from 'src/engine/core-modules/user-workspace/user-workspace.service'; | ||
import { User } from 'src/engine/core-modules/user/user.entity'; | ||
import { ActivateWorkspaceInput } from 'src/engine/core-modules/workspace/dtos/activate-workspace-input'; | ||
import { ActivateWorkspaceOutput } from 'src/engine/core-modules/workspace/dtos/activate-workspace-output'; | ||
import { PublicWorkspaceDataOutput } from 'src/engine/core-modules/workspace/dtos/public-workspace-data.output'; | ||
import { UpdateWorkspaceInput } from 'src/engine/core-modules/workspace/dtos/update-workspace-input'; | ||
import { getAuthProvidersByWorkspace } from 'src/engine/core-modules/workspace/utils/getAuthProvidersByWorkspace'; | ||
import { workspaceGraphqlApiExceptionHandler } from 'src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler'; | ||
import { | ||
WorkspaceException, | ||
WorkspaceExceptionCode, | ||
} from 'src/engine/core-modules/workspace/workspace.exception'; | ||
import { workspaceValidator } from 'src/engine/core-modules/workspace/workspace.validate'; | ||
import { AuthUser } from 'src/engine/decorators/auth/auth-user.decorator'; | ||
import { AuthWorkspace } from 'src/engine/decorators/auth/auth-workspace.decorator'; | ||
import { OriginHeader } from 'src/engine/decorators/auth/origin-header.decorator'; | ||
import { DemoEnvGuard } from 'src/engine/guards/demo.env.guard'; | ||
import { UserAuthGuard } from 'src/engine/guards/user-auth.guard'; | ||
import { WorkspaceAuthGuard } from 'src/engine/guards/workspace-auth.guard'; | ||
import { assert } from 'src/utils/assert'; | ||
import { isDefined } from 'src/utils/is-defined'; | ||
import { streamToBuffer } from 'src/utils/stream-to-buffer'; | ||
import { | ||
WorkspaceException, | ||
WorkspaceExceptionCode, | ||
} from 'src/engine/core-modules/workspace/workspace.exception'; | ||
import { PublicWorkspaceDataOutput } from 'src/engine/core-modules/workspace/dtos/public-workspace-data.output'; | ||
import { ActivateWorkspaceOutput } from 'src/engine/core-modules/workspace/dtos/activate-workspace-output'; | ||
import { OriginHeader } from 'src/engine/decorators/auth/origin-header.decorator'; | ||
import { workspaceValidator } from 'src/engine/core-modules/workspace/workspace.validate'; | ||
import { LoginTokenService } from 'src/engine/core-modules/auth/token/services/login-token.service'; | ||
import { DomainManagerService } from 'src/engine/core-modules/domain-manager/service/domain-manager.service'; | ||
import { getAuthProvidersByWorkspace } from 'src/engine/core-modules/workspace/utils/getAuthProvidersByWorkspace'; | ||
import { workspaceGraphqlApiExceptionHandler } from 'src/engine/core-modules/workspace/utils/workspaceGraphqlApiExceptionHandler'; | ||
|
||
import { Workspace } from './workspace.entity'; | ||
|
||
|
@@ -193,9 +193,23 @@ export class WorkspaceResolver { | |
), | ||
); | ||
|
||
let workspaceLogoWithToken = ''; | ||
|
||
if (workspace.logo) { | ||
try { | ||
const workspaceLogoToken = await this.fileService.encodeFileToken({ | ||
workspaceId: workspace.id, | ||
}); | ||
|
||
workspaceLogoWithToken = `${workspace.logo}?token=${workspaceLogoToken}`; | ||
} catch (e) { | ||
workspaceLogoWithToken = workspace.logo; | ||
} | ||
Comment on lines
+205
to
+207
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. style: Consider logging the error before falling back to raw logo URL |
||
} | ||
|
||
return { | ||
id: workspace.id, | ||
logo: workspace.logo, | ||
logo: workspaceLogoWithToken, | ||
displayName: workspace.displayName, | ||
subdomain: workspace.subdomain, | ||
authProviders: getAuthProvidersByWorkspace(workspace), | ||
|
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.
style: check if primaryLogo could also be an empty string - may need similar validation