Skip to content

Refactor logout - #3277

Merged
code-asher merged 8 commits into
coder:mainfrom
code-asher:logout
May 4, 2021
Merged

Refactor logout#3277
code-asher merged 8 commits into
coder:mainfrom
code-asher:logout

Conversation

@code-asher

@code-ashercode-asher commented May 3, 2021

Copy link
Copy Markdown
Member

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

@code-asher
code-asher requested a review from a team as a code ownerMay 3, 2021 20:26
@code-asher
code-asherforce-pushed the logout branch 2 times, most recently from 72c3868 to d4e848aCompareMay 3, 2021 20:32
@code-asher
code-asher marked this pull request as draft May 3, 2021 20:42
@code-asher

Copy link
Copy Markdown
MemberAuthor

Moving to a draft while I update the tests.

@code-asher
code-asherforce-pushed the logout branch 2 times, most recently from 64ba137 to 754f57aCompareMay 3, 2021 20:59
@codecov

codecovBot commented May 3, 2021

Copy link
Copy Markdown

Codecov Report

Merging #3277 (8b2c78c) into main (9fe459a) will increase coverage by 7.87%.
The diff coverage is 85.71%.

Impacted file tree graph

@@ 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 
Impacted FilesCoverage Δ
src/node/app.ts70.73% <0.00%> (ø)
src/browser/register.ts100.00% <100.00%> (+7.69%)⬆️
src/common/util.ts100.00% <100.00%> (+27.65%)⬆️
src/node/http.ts27.86% <0.00%> (-6.18%)⬇️
src/node/vscode.ts27.50% <0.00%> (-2.64%)⬇️
src/node/wrapper.ts33.12% <0.00%> (-0.43%)⬇️
src/node/entry.ts0.00% <0.00%> (ø)
src/common/emitter.ts100.00% <0.00%> (ø)
src/node/proxy_agent.ts0.00% <0.00%> (ø)
src/browser/pages/vscode.ts0.00% <0.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fe459a...8b2c78c. Read the comment docs.

Comment threadlib/vscode/src/vs/server/browser/client.ts
Comment threadlib/vscode/src/vs/server/browser/client.ts
Comment threadlib/vscode/src/vs/server/browser/client.ts
Comment threadsrc/browser/register.ts
Comment threadtypings/ipc.d.ts
@code-asher
code-asher marked this pull request as ready for review May 4, 2021 18:57
@code-asher
code-asher requested a review from jsjoeioMay 4, 2021 18:57
@jsjoeio

Copy link
Copy Markdown
Contributor

will increase coverage by 7.87%.

DANG! Look at you 👏

@jsjoeiojsjoeio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did a good job. False. You did an awesome job.

@jsjoeiojsjoeio added this to the v3.9.4 milestone May 4, 2021
@jsjoeiojsjoeio added the enhancement Some improvement that isn't a feature label May 4, 2021
@oxy

oxy commented May 4, 2021

Copy link
Copy Markdown

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.

@code-asher

code-asher commented May 4, 2021

Copy link
Copy Markdown
MemberAuthor

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.

@code-asher
code-asher merged commit 75e9e24 into coder:mainMay 4, 2021
@code-asher
code-asher deleted the logout branch May 4, 2021 20:13
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementSome improvement that isn't a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Log out button should not appear if not using password Logout functionality not working as expected

3 participants

@code-asher@jsjoeio@oxy