-
Notifications
You must be signed in to change notification settings - Fork 224
Fix Google SSO/SAML authentication; fixes #1973 #2261
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
base: develop
Are you sure you want to change the base?
Conversation
|
I posted something similar in the issue and forgot to include it when I submitted the PR- I have no idea if this will break other stuff and in full transparency, I used Claude Code to figure out why this was happening. I took a look at the code was originally added in 2ad39ff just to try to understand why it was added in the first place. I did test to see if TickTick authentication still works and it does so unless it was more than the auth, it seems to be good. I had Claude do some analysis of why this shouldn't break things (as if the excessive emoji use didn’t give it away). What that commit did: If further changes need to be made, I can take a look but this honestly isn’t my area of expertise. |
|
I took at shot at simplifying things. I left it as it's own commit for now in case it needs to be backed out. |
|
@mbentley Thanks a lot for looking into this and raising this PR. At first glance it seems very legitimate, but I would need to have a closer look at it to check it it could have side effects. In the meantime, can you just make sure to run the build+lint locally and push the fixes so that the builds can work through it? @kris7t If you have a bit of time, I'd be happy to have your input on why was this implemented in the first place (since it seems this was you who made this), and if you think such a logic is still necessary? Thanks |
|
Ah sorry about that, I was working from my laptop without all of the required tools, only using containers so the husky hooks weren't executing. Fixed the formatting and squashed the commits. |
Signed-off-by: Matt Bentley <[email protected]>
|
This was implemented I think because Google had some validation for the User-Agent string (they really don't like people log in to Google with Electron apps directly and they'd prefer we used OAuth) that this forwarding hack worked around. In the meantime, Google probably updated its validation, and now it seems Electron is working differently, so we can't use the same hack for handling forwarding... Note that using OAuth (as required by Google) is impossible here, because that would only give Ferdium access to the user's Google data, but it wouldn't give the browser session inside Ferdium a valid Google session cookie (this separation of privileges is the point of OAuth). SSO with Google requires a valid Google session cookie inside the browser container. |
|
I just wanted to check back in for any feedback on anything that potentially should be done differently or if this is good as is. I was reminded of this annoyance today, having to manually execute the workaround for SSO 😉 |
SpecialAro
left a comment
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.
I haven't tested this but LGTM!
|
Just chipping in on that: I am afraid the removal of the I haven't been able to test it fully since I do not have access to the type of accounts having the problem, but using this fix hasn't affected anything on my side anyway. So if my remark above if of no concern, then I'm happy for this to be merged. |
|
Sorry for the AI generated analysis - someone will want to double check these points but this is what Claude found: "The userAgent(..) method is called in other parts of the code, and in such cases, it will never receive the userAgentWithoutChromeVersion value" Places the
"Are we sure that the will-navigate event will be fired every time we actually care about changing this user agent?" The old code relied on the exact same events// OLD CODE
_addWebviewEvents(webview: ElectronWebView): void {
this._willNavigateListener = event => this._handleNavigate(event.url, true);
webview.addEventListener('will-navigate', this._willNavigateListener);
this._didNavigateListener = event => this._handleNavigate(event.url);
webview.addEventListener('did-navigate', this._didNavigateListener);
}My code uses the exact same events// NEW CODE
this._willNavigateListener = event => this._handleNavigate(event.url);
webview.addEventListener('will-navigate', this._willNavigateListener);
this._didNavigateListener = event => this._handleNavigate(event.url);
webview.addEventListener('did-navigate', this._didNavigateListener);The Google SSO flow is a series of redirects. Every redirect triggers Summary
The concern stems from thinking the getter was doing something special. It wasn't—the getter was just a computed property. The actual user agent change always happened via direct assignment to |
|
I just tested this commit and it works for me for using Google workspace in combination with keycloak. Edit: Now running for a week and still no issues |
Pre-flight Checklist
Please ensure you've completed all of the following.
Description of Change
Skip the reload because it will cancel POST requests which breaks SSO/SAML auth
Motivation and Context
This fixes #1973
Screenshots
n/a
Checklist
pnpm prepare-code)pnpm testpassesRelease Notes
Fixes SSO/SAML authentication issues