-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[server] enable auth for snapshot creation #3444
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
Conversation
How to test this
|
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.
41c98f8
to
ec54ff2
Compare
56dd37a
to
a6695a7
Compare
Yes, it is noisy. The problem is that the "Copy URL to Clipboard" will silently fail when clipboard access is not granted in the browser, and VS Code seems to block all other ways to present the URL in the dialog. And it's a bit better if the base URL is just For the time being, we could add an outputChannel and print the URL there, such that users can copy it themselves. In the long run, we should go for another UI that also allows browsing all snapshots of that workspace. Still we have to solve the copy/paste problem. |
I think we can go with it for now and then replace it with proper webview which will fix the clipboard issue as well. @svenefftinge would it be fine? |
Is using a link as Chris proposed not possible within the notification? I would find that more UX-friendly, as it is easier on the eye and easier to copy. |
@JanKoehnlein I read through the code and it seems that links are supported with markdown syntax: https://github.com/microsoft/vscode/blob/a699ffaee62010c4634d301da2bbdb7646b8d1da/src/vs/base/common/linkedText.ts#L26-L28 i.e. |
I tried markdown links in a VS Code message dialog yesterday, but it didn't work. Tried again today, and now it works. Must have slightly messed up the syntax. So now it's a link. Sorry for the noise (in all regards) |
a6695a7
to
0e83f05
Compare
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.
LGTM
The problem is that for snapshot creation, we don't have a snapshot yet, and as such have to provide the workspaceID instead for the create token. This creates an ambiguity between
which has to be resolved. This solution uses a prefix if the subjectID is a workspaceID.
I refrained from creating a
SnapshotCreateResourceGuard
wire that up using aCompositeResourceGuard
as the latter only ORs the composite guards. As such theTokenResourceGuard
would still accept aSnapshotResource
whose snapshotID equals the workspaceID for which the permission has been granted. So our hope that we could solve this with a local additive change was wrong. Alternatively, we could either add aVetoCompositeResourceGuard
or put the entire filtering and matching logic intoTokenResourceGuard
. Both appeared far more invasive to me than the prefix approach.