-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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: user has to login every time chrome sidepanel is opened #5544
fix: user has to login every time chrome sidepanel is opened #5544
Conversation
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.
Pull Request Summary
- Authentication State Management: Introduced methods to set token state from cookies and listen for cookie changes to maintain user sessions in the Chrome side panel.
- URL Handling: Modified URL handling in content scripts to use relative paths, simplifying the process and reducing the need for repeated logins.
- New Components: Added
WindowEventEffect
to handle messages from the Chrome extension, and a new settings component to store server and client URLs. - Permissions Update: Updated manifest to include 'cookies' permission and support custom domain hosting.
Notes
- Potential Pitfall: Ensure that the new cookie-based token management does not introduce security vulnerabilities.
- Code Reuse Opportunity: The
WindowEventEffect
component can be reused for other window event handling needs in the future.
Comments
packages/twenty-chrome-extension/src/background/index.ts
- Lines 1 - 3: Unused imports
Crypto
andexchangeAuthorizationCode
have been removed. This is a good cleanup. - Line 22: The
launchOAuth
case has been removed from the message listener. Ensure that this functionality is no longer needed elsewhere in the extension. - Line 30: The
changeSidepanelUrl
case has been removed from the message listener. Verify that this action is not required by any other part of the extension. - Line 56: The
generateRandomString
andgenerateCodeVerifierAndChallenge
functions have been removed. Ensure that these are not used elsewhere in the codebase. - Line 154: The
launchOAuth
function has been removed. Confirm that OAuth functionality is handled appropriately elsewhere. - Line 156: The new
setTokenStateFromCookie
function is added to handle token state updates from cookies. This is a crucial addition for maintaining user authentication state. - Line 168: The
chrome.cookies.onChanged
listener is added to update token state when thetokenPair
cookie changes. This ensures that the extension stays in sync with authentication state changes. - Line 176: The initial check for the
tokenPair
cookie and setting thecookiesRead
flag is added to prevent redundant state updates. This is a good optimization.
packages/twenty-chrome-extension/src/contentScript/extractCompanyProfile.ts
- Line 112: The addition of
await changeSidePanelUrl('/objects/companies');
should be verified to ensure it doesn't introduce any unintended side effects.
packages/twenty-chrome-extension/src/contentScript/extractPersonProfile.ts
- Lines 88 - 90: Using relative paths for
changeSidePanelUrl
improves flexibility and environment compatibility. - Lines 117 - 119: Switching to relative paths for
changeSidePanelUrl
ensures consistency across different environments. - Line 123: Adding a fallback URL
/objects/people
ensures the side panel opens correctly even if the person is not found.
packages/twenty-chrome-extension/src/options/Settings.tsx
- Line 26: The
void
operator is not necessary here. You can simply callgetState();
. - Line 54: The
alt
attribute for the image should be more descriptive, e.g., 'Twenty logo'.
packages/twenty-chrome-extension/src/options/Sidepanel.tsx
- Line 7: The import for
isDefined
is correct, but ensure that the utility function is available and correctly imported from the specified path. - Line 39: The
clientUrl
state initialization can be simplified by directly using the environment variableimport.meta.env.VITE_FRONT_BASE_URL
.
packages/twenty-front/src/effect-components/WindowEventEffect.tsx
- Line 41: Add
navigate
to the dependency array to ensure the effect is updated ifnavigate
changes.
packages/twenty-front/vite.config.ts
- Line 60: Good addition of
REACT_APP_CHROME_EXTENSION_ID
to thedefine
property. This will make sure the variable is available in the application runtime.
// setting host permissions to all http connections will allow | ||
// for people who host on their custom domain to get access to | ||
// extension instead of white listing individual urls | ||
host_permissions: ['https://*/*', 'http://*/*'], |
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.
Seems like it defeats the purpose of permissions, can't we make it dynamic or have the self-hosting user recompile the extension ?
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.
Not realistic to expect self-hosting users to recompile unfortunately, I think most users don't care and prefer the simplest solution which is this one (code is open source / running locally so you could see if it was doing something wrong). I had looked but didn't find any other option.
packages/twenty-front/src/effect-components/WindowEventEffect.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/effect-components/WindowEventEffect.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/window-event/components/WindowEventProvider.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/window-event/components/WindowEventProvider.tsx
Outdated
Show resolved
Hide resolved
navigate(event.data.value); | ||
break; | ||
default: | ||
break; |
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.
error message
packages/twenty-front/src/modules/window-event/components/WindowEventProvider.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/App.tsx
Outdated
<ClientConfigProviderEffect /> | ||
<ClientConfigProvider> | ||
<WindowEventEffect /> | ||
<WindowEventProvider> |
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.
One last thing before we merge in the current state, could we rename this into something more specific so that people understand they probably don't have to deal with this? I'm tempted to call it ChromeExtensionSidecarEffect
and ChromeExtensionSidecarProvider
or something like that for now. If eventually it becomes something more generic we can rename 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.
Okay!
We can pass the auth tokens to our front app via post message, which will also allow us to pass route names to navigate on it
…q#5544) We can pass the auth tokens to our front app via post message, which will also allow us to pass route names to navigate on it
We can pass the auth tokens to our front app via post message, which will also allow us to pass route names to navigate on it