-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Refactor logout #3277
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
Refactor logout #3277
Conversation
72c3868 to
d4e848a
Compare
Moving to a draft while I update the tests.
64ba137 to
754f57a
Compare
Codecov Report
@@ Coverage Diff @@ ## main #3277 +/- ## ========================================== + Coverage 46.90% 54.78% +7.87% ========================================== Files 23 23 Lines 1196 1265 +69 Branches 237 286 +49 ========================================== + Hits 561 693 +132 - Misses 451 460 +9 + Partials 184 112 -72
Continue to review full report at Codecov.
|
This lets us re-use the normalized base path so when we expire/clear the cookie we use the same base path.
3c5a6d6 to
8b2c78c
Compare
will increase coverage by 7.87%.
DANG! Look at you 👏
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.
The one thing I'm slightly concerned about is that we have a 1.56 branch in the pipeline already which makes its own set of changes to menubarControl.ts, etc. - how much of this will we have to reimplement/re-fix in the 1.56 branch?
AFAICT its not too much work inside lib/vscode; but would still like any directions if things need changing.
Ah yup actually I initially based this off your branch and switched back to main so I could get it merged in. 😛
All I did to resolve conflicts was to checkout the source (since we want to remove all our changes in that file). We can do that again in the 1.56 branch. Or we could resolve manually, I think the only conflict was the logService line and we'd want to remove the comment added above as well.
Uh oh!
There was an error while loading. Please reload this page.
This should fix logging out on non-localhost domains and non-root paths (and some other cases like when using
--proxy-domain).It also moves the logout to a command and registers it from our client entry so we don't need to patch VS Code. This new command redirects to the logout endpoint where we clear the cookie from the server end.
It also only shows the logout when authentication is enabled.
Fixes #2984
Fixes #2985
Should unblock #2245 (reply in thread) as well