Skip to content
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

Add "(Administrator)" suffix to window title when running as administrator in Windows #19707

Closed
daviwil opened this issue Feb 2, 2017 · 29 comments
Assignees
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded workbench-run-as-admin Issues concerning running as administrator
Milestone

Comments

@daviwil
Copy link
Contributor

daviwil commented Feb 2, 2017

  • VSCode Version: 1.8.1, 1.9.0-insiders
  • OS Version: Windows 10 Anniversary

Steps to Reproduce:

  1. Run VS Code / Insiders as administrator by right-clicking the icon and clicking "Run as Administrator"
  2. Observe window title does not indicate that Code is running as administrator

Since running as administrator works pretty well now, the only thing that's missing is an indication that the window has been run as administrator. Visual Studio (and other apps) use the title bar to indicate this:

image

@bpasero bpasero added feature-request Request for new features or functionality workbench labels Feb 2, 2017
@arkon
Copy link

arkon commented Feb 2, 2017

I was hoping to work on this issue and I figured out how to implement it, but it seems that translation strings need to be modified, which are seemingly done by a third party.

The suffix would need to be added within this block, and using some method to determine if the application is being run with Admin privileges.

I suppose this would need to be implemented by the VS Code team?

@bpasero
Copy link
Member

bpasero commented Feb 8, 2017

@arkon I think the biggest issue is that I am not sure how to find out that VS Code runs as "Admin". If someone has an idea, I am happy to add it 👍

@bpasero bpasero added the help wanted Issues identified as good community contribution opportunities label Feb 8, 2017
@arkon
Copy link

arkon commented Feb 8, 2017

@bpasero I linked to a StackOverflow thread about that previously that should work: http://stackoverflow.com/a/37670360

@bpasero
Copy link
Member

bpasero commented Feb 8, 2017

Ugly. Imho this should go into either a native node module or better, into Electron itself (process.isElevated).

@arkon
Copy link

arkon commented Feb 8, 2017

Maybe we should request it upstream for Electron to expose functions to check if we're running as administrator (Windows) or root (UNIX)?

@bpasero
Copy link
Member

bpasero commented Feb 8, 2017

We can, but only with a PR 👍

@arkon
Copy link

arkon commented Mar 27, 2017

@bpasero Seems like they might favour users using a separate node module for checking for elevated privileges rather than having it implemented as part of Electron (electron/electron#8980 (comment)).

@arkon
Copy link

arkon commented Mar 27, 2017

Since they suggested is-elevated, I suppose we could use that here?

@bpasero
Copy link
Member

bpasero commented Mar 27, 2017

Last time I checked these modules seem to spawn a process on Windows at least? That is not a solution I would want. A module should use some native code to find out if the user is Admin or not.

@arkon
Copy link

arkon commented Mar 27, 2017

Hmm, perhaps you could voice your opinion on that electron thread then?

@bpasero
Copy link
Member

bpasero commented Mar 27, 2017

@arkon sorry I missed your reference to that PR, it looks very cool. Any reason you could not make that a native module? VS Code would use it right away.

@arkon
Copy link

arkon commented Mar 27, 2017

@bpasero Is "native module" referring to this?

@bpasero
Copy link
Member

bpasero commented Mar 27, 2017

@arkon exactly! since you already did the hard part of writing the C code, putting this to a native module is not a lot of work imho. See for example this module that we are using which does that: https://github.com/the-ress/node-windows-foreground-love

@arkon
Copy link

arkon commented Mar 27, 2017

@bpasero Ah, I see! I'll try my hand at making a native module then.

@bpasero
Copy link
Member

bpasero commented Mar 28, 2017

Awesome 👍

@arkon
Copy link

arkon commented Mar 28, 2017

I've published native-is-elevated to NPM.

@bpasero bpasero added this to the April 2017 milestone Mar 28, 2017
@bpasero bpasero self-assigned this Mar 28, 2017
@bpasero
Copy link
Member

bpasero commented Mar 28, 2017

Thanks, I will look into this for April!

@daviwil
Copy link
Contributor Author

daviwil commented Mar 30, 2017

Awesome, nice work @arkon!

bpasero added a commit that referenced this issue Apr 2, 2017
@bpasero
Copy link
Member

bpasero commented Apr 2, 2017

I played around with the library and noticed that on Windows calling into the module can take 32ms (on my fast macbook pro, running in a VM). Given our performance work we are doing I am feeling not so easy adding this to the critical startup path (the title is on it). I also noticed that the library is not needed on mac or linux where finding out if the user is root or not is very easy.

I think what we can do is the following:

If someone wants to step in, my work is in 3ac3f1e

@bpasero bpasero modified the milestones: Backlog, April 2017 Apr 2, 2017
@arkon
Copy link

arkon commented Apr 2, 2017

Would it help if native-is-elevated's isElevated() function were to return a promise?

@bpasero
Copy link
Member

bpasero commented Apr 2, 2017

@arkon good point, maybe not a promise but a typical function callback, as with other functions in node that are long running (e.g. fs.stat)

Though could you actually implement that because it seems your call in C land is sync?

@arkon
Copy link

arkon commented Apr 2, 2017

@bpasero It ought to be possible. I found this example of using promises, and it seems like Nan could be useful for implementing callbacks. I'll give it a shot.

EDIT: oh wait, I see what you mean by the call being synchronous. I suppose there'd be no way to make it asynchronous unless it somehow did it in a different thread, but the whole point of this was to avoid spawning something extra... 🤔

@bpasero
Copy link
Member

bpasero commented Apr 2, 2017

Yeah thats what I think too. That is why my initial reaction was to call this method only after a delay. I can look into it, actually requiring the module is not what takes time, it is calling into that method so we can probably keep that module on all platforms.

@bpasero bpasero removed their assignment Apr 7, 2017
@bpasero bpasero removed the help wanted Issues identified as good community contribution opportunities label May 20, 2017
@bpasero bpasero added workbench-run-as-admin Issues concerning running as administrator and removed workbench labels Nov 16, 2017
@bpasero bpasero removed this from the Backlog milestone Nov 17, 2017
bpasero added a commit that referenced this issue Dec 11, 2017
bpasero added a commit that referenced this issue Dec 12, 2017
@bpasero bpasero self-assigned this Dec 12, 2017
@bpasero bpasero added this to the December 2017 milestone Dec 12, 2017
@rwatts3
Copy link

rwatts3 commented Dec 15, 2017

I'm noticing some fallout that may be related to this issue.
I am receiving a popup saying it is not safe to run insiders as an administrator, and it states in the title bar that i'm running as an administrator, even though I didn't choose "Run As Administrator" when I started the program. I can confirm that I have opened insiders normally as I did prior to this release , however I can not find a way to deactivate the warning notification.

@bpasero
Copy link
Member

bpasero commented Dec 16, 2017

Yeah maybe it is too aggressive to show this on Windows where it is more likely that you run as admin. I can take it out again.

@bpasero bpasero added the verification-needed Verification of issue is requested label Dec 20, 2017
@bpasero
Copy link
Member

bpasero commented Dec 20, 2017

Verification:
Run Code as Administrator on Windows and verify that the title includes the information that you are running as admin.

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 26, 2018
@ramya-rao-a
Copy link
Contributor

@bpasero Any reason we are going with the square brackets [] instead of the normal ones () ?

image

@ramya-rao-a ramya-rao-a added the verified Verification succeeded label Jan 31, 2018
@bpasero
Copy link
Member

bpasero commented Feb 1, 2018

@ramya-rao-a no, I just picked that style up from the other decorations we had ("Unsupported" and "Extension Development Host").

image

@ramya-rao-a
Copy link
Contributor

got it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded workbench-run-as-admin Issues concerning running as administrator
Projects
None yet
Development

No branches or pull requests

5 participants