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

stdlib/os: add isAdmin #17012

Merged
merged 9 commits into from
Mar 7, 2021
Merged

stdlib/os: add isAdmin #17012

merged 9 commits into from
Mar 7, 2021

Conversation

rominf
Copy link
Contributor

@rominf rominf commented Feb 11, 2021

Tested on Windows 10 as a normal user and as an administrator; on Fedora 33 as a normal user and as a root.

(EDIT(timotheecour))
fixes nim-lang/RFCs#329

@rominf rominf force-pushed the rominf-os-isAdmin branch 6 times, most recently from 8abc38e to 6bda383 Compare February 11, 2021 18:44
tests/stdlib/tos.nim Outdated Show resolved Hide resolved
lib/windows/winlean.nim Show resolved Hide resolved
@rominf rominf force-pushed the rominf-os-isAdmin branch 3 times, most recently from a78191d to abb5127 Compare February 11, 2021 19:19
@rominf
Copy link
Contributor Author

rominf commented Feb 11, 2021

@timotheecour I don't like printing isAdmin() result into the console; on the other hand, it tests that the code compiles and runs without exceptions...

lib/pure/os.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member

@timotheecour I don't like printing isAdmin() result into the console; on the other hand, it tests that the code compiles and runs without exceptions...

i think this was already addressed by:

  let isAzure = existsEnv("TF_BUILD") # xxx factor with testament.specs.isAzure
  if isAzure: doAssert isAdmin()

but in other situations, instead of echo you can use:

discard someAPI() # no warning unused

for that, or even:

let ok = someAPI() # not as good, would issue a warning

lib/pure/os.nim Outdated Show resolved Hide resolved
lib/pure/os.nim Outdated Show resolved Hide resolved
lib/pure/os.nim Outdated Show resolved Hide resolved
rominf added a commit to rominf/Nim that referenced this pull request Feb 13, 2021
rominf and others added 4 commits February 13, 2021 01:21
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
lib/pure/os.nim Outdated Show resolved Hide resolved
@rominf rominf requested a review from dom96 February 14, 2021 09:31
@Varriount
Copy link
Contributor

What would a "classic use case" of this function be?

@rominf
Copy link
Contributor Author

rominf commented Feb 14, 2021

What would a "classic use case" of this function be?

See: nim-lang/RFCs#329 (checking symlinks creation on Windows)

@Varriount
Copy link
Contributor

Varriount commented Feb 14, 2021

What would a "classic use case" of this function be?

See: nim-lang/RFCs#329 (checking symlinks creation on Windows)

Being root or Administrator shouldn't be conflated with having the ability to do something. It's quite possible to allow users other than the Administrator to create symlinks on Windows.

I don't think that this function should be added, precisely because of this misconception. There are relatively few situations where one actually needs to know whether the user is root/administrator. Usually, one should assume that the user has permission to do something, and appropriately handle the cases where permission is denied.

lib/pure/os.nim Show resolved Hide resolved
lib/windows/winlean.nim Outdated Show resolved Hide resolved
@rominf
Copy link
Contributor Author

rominf commented Mar 3, 2021

Thanks, @timotheecour, and @Araq for pointing me at my mistakes and for your patience!

I've been busy these days and finally got some time to fix the code.

@Varriount
Copy link
Contributor

While I appreciate the improvements made, I would still like to know: what use-cases does this addition address? Especially given the fact that it can be easily misused.

@Araq
Copy link
Member

Araq commented Mar 4, 2021

Sorry, but this code should go into its own module. Suggestion: extensions / admin. Yes, this "extensions" directory would be new, no, it won't move to std. Yes, I am changing my mind, we need more than just std.

@timotheecour
Copy link
Member

timotheecour commented Mar 4, 2021

Sorry, but this code should go into its own module. Suggestion: extensions / admin. Yes, this "extensions" directory would be new, no, it won't move to std. Yes, I am changing my mind, we need more than just std.

I'm fine (and in fact in favor) of a separate module (modules are cheap, giant modules like times and os should be broken down and re-export their sub-modules, even if we implement cyclic imports), however:

  • this module should be importable from std/os without introducing cyclic dependencies in case it's needed there (eg use cases: better error messages, plausible approach for copyDir semantics for syminks on windows, along with Add isWindowsDeveloperModeActive RFCs#343 etc)
  • extensions/ at the root needs an RFC, and in fact it sounds very similar to Fusion and stdlib evolution RFCs#310 which wasn't popular; in any case stdx would be much better IMO

In the meantime, we can move this to a new module lib/std/private/osutils and "decide later", at least the implementation will be there and could be used within the confines of nim repo (plus whoever is ok with breaking changes) and doesn't have to wait on deciding on stdx vs extensions vs os

changelog.md Outdated Show resolved Hide resolved
tests/stdlib/tos.nim Outdated Show resolved Hide resolved
DWORD* = int32
PDWORD* = ptr DWORD
LPINT* = ptr int32
ULONG_PTR* = uint
PULONG_PTR* = ptr uint
HDC* = Handle
HGLRC* = Handle
BYTE* = cuchar
Copy link
Member

Choose a reason for hiding this comment

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

ok for now, but we should address nim-lang/RFCs#344 before next release and move it to wintypes

Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

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

finished review; after comments are addressed we can decide what to do with #17012 (comment) (keep in os, move to std/private/osutils, other)

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
@rominf
Copy link
Contributor Author

rominf commented Mar 7, 2021

While I appreciate the improvements made, I would still like to know: what use-cases does this addition address? Especially given the fact that it can be easily misused.

For example, some installation scripts that write into /usr, /etc or C:\Program files, start daemons/services usually check admin rights before execution.

@rominf
Copy link
Contributor Author

rominf commented Mar 7, 2021

@timotheecour Comments are addressed.

Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

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

LGTM.
for #17012 (comment), IMO extensions / admin isn't warranted, because it's conceivable isAdmin can be useful for procs in std/os. That said, I'd support splitting std/os in future work into multiple submodules that would be imported and re-exported by std/os (can be done transparently); modules are cheap.

@Araq Araq merged commit 31424b3 into nim-lang:devel Mar 7, 2021
ringabout pushed a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
* stdlib/os: add isAdmin

* uint8 -> cuchar, assert isAdmin on Azure

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

* Update lib/pure/os.nim docs

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

* Address comments on nim-lang#17012

* Raise on errors in nim-lang#17012

* Check the result of FreeSid in nim-lang#17012

* Change case in nim-lang#17012

* Fix memory leak in nim-lang#17012

* Address comments in nim-lang#17012

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* stdlib/os: add isAdmin

* uint8 -> cuchar, assert isAdmin on Azure

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

* Update lib/pure/os.nim docs

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

* Address comments on nim-lang#17012

* Raise on errors in nim-lang#17012

* Check the result of FreeSid in nim-lang#17012

* Change case in nim-lang#17012

* Fix memory leak in nim-lang#17012

* Address comments in nim-lang#17012

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add os.isAdmin to tell whether we are admin (windows) or root (posix)
6 participants