Skip to content

Conversation

@EhabY
Copy link
Collaborator

When opening a workspace via URI without a token, ensure we prompt for login and set the deployment before attempting to open. Extract URI handler into dedicated module with route-based dispatch.

Closes#680

When opening a workspace via URI without a token, ensure we prompt for login and set the deployment before attempting to open. Extract URI handler into dedicated module with route-based dispatch.
Copy link
Member

@code-ashercode-asher left a comment

Choose a reason for hiding this comment

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

Awesome, verified the token prompt shows up when the token is invalid or missing.

Comment on lines +163 to +165
if(token!==null){
awaitsecretsManager.setSessionAuth(safeHostname,{ url, token });
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we wait to set the token until after ensureLoggedIn? To avoid potentially storing an invalid or blank token.

Not a huge deal since usually the token should be set and valid, I just noticed that if I am already logged in and the URI contains an invalid or blank token, I get partially logged out (my workspace list is empty but I still look logged-in) and if I reload VS Code I am completely logged out. Just seems unfortunate to lose the perfectly good token I had stored previously.

Actually ensureLoggedIn already sets auth so I suppose we can skip it here. Although, it also sets it before validating. Maybe something we can visit later.

thrownewError("Failed to login to deployment from URI");
}

awaitdeploymentManager.setDeploymentIfValid({
Copy link
Member

Choose a reason for hiding this comment

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

Why not call setDeployment directly? The documentation on the function says "Use this when you already have the user from a successful login" which seems to fit here.

Comment on lines +88 to +91
if(deployment.user){
awaitthis.setDeployment({ ...deployment,user: deployment.user});
returntrue;
}
Copy link
Member

Choose a reason for hiding this comment

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

Continuing with the previous comment, if we already have a user, could we just call setDeployment directly?

The comment on setDeploymentIfValid says it validates auth so if we do keep this, we might mention that calling it with an already-validated deployment is the same as calling setDeployment directly and no additional validation is performed in that case.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing login prompt if URI handler is missing the token

2 participants

@EhabY@code-asher