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

Fix admin profile config and support APPDATA. #42

Closed
wants to merge 1 commit into from

Conversation

rkitover
Copy link

Only use ALLUSERPROFILE for home dir when USERPROFILE is not set, even
for admin users.
Fix #34.

Also on Windows when looking for the rc, look in getenv("APPDATA")
instead of getenv("XDG_CONFIG_HOME"), which is mostly equivalent.

Signed-off-by: Rafael Kitover [email protected]

@lhmouse
Copy link
Owner

lhmouse commented May 31, 2022

The ALLUSERSPROFILE thing is by intention. If nano is started as a non-elevated process by an admin account, IsUserAnAdmin() returns false, and the user's .nanorc is used. If nano is started as an elevated process, All Users/.nanorc is used. This is probably not satisfactory (Administrator/.nanorc would be better) but it matches the behavior on Linux, where a sudo'd process reads /root/.nanorc instead of the user's nanorc.

@rkitover
Copy link
Author

rkitover commented Jun 1, 2022

@akuropka Your input here would be appreciated.

The ALLUSERSPROFILE thing is by intention. If nano is started as a non-elevated process by an admin account, IsUserAnAdmin() returns false, and the user's .nanorc is used. If nano is started as an elevated process, All Users/.nanorc is used. This is probably not satisfactory (Administrator/.nanorc would be better) but it matches the behavior on Linux, where a sudo'd process reads /root/.nanorc instead of the user's nanorc.

Consider the usual case, user installs Windows and his user is an admin user, in an elevated shell his home directory is the same but the .nanorc is not read. This is very confusing, see #34.

@rkitover
Copy link
Author

rkitover commented Jun 1, 2022

How about checking if the directory pointed to by getenv("USERPROFILE") exists, and if not using getenv("ALLUSERPROFILE").

@lhmouse
Copy link
Owner

lhmouse commented Jun 1, 2022

Consider the usual case, user installs Windows and his user is an admin user, in an elevated shell his home directory is the same but the .nanorc is not read. This is very confusing, see #34.

It's the same on Linux; a sudo'd nano reads /root/.nanorc instead of the one of who calls sudo (unless it's root, of course).

As said above, it's probably better to use %USERPROFILE%/../Administrator/.nanorc here. But anyway, the user's .nanorc should be ignored.

@rkitover
Copy link
Author

rkitover commented Jun 1, 2022

It is not the same on Linux, a Linux user is aware that sudo runs as root, while Windows allows actual admin users with their own profile directory. UNIX ports usually do not do anything special here.

@rkitover
Copy link
Author

rkitover commented Jun 1, 2022

Also the Administrator user is not equivalent to another admin user, they are separate users.

@lhmouse
Copy link
Owner

lhmouse commented Jun 1, 2022

It is not the same on Linux, a Linux user is aware that sudo runs as root, while Windows allows actual admin users with their own profile directory. UNIX ports usually do not do anything special here.

This is correct. However, please take a look at the default .nanorc file. There is a line saying:

## In root's .nanorc you might want to use:

which suggests a different color scheme be used for nano running as root for distinction.

Also the Administrator user is not equivalent to another admin user, they are separate users.

One notable difference is that if a user logs in as Administrator, they by default create elevated processes using the Run command.

One reason for not using Administrator/.nanorc is that this account is disabled by default, and does not have a profile directory unless it is enabled and logged in once.

@rkitover
Copy link
Author

rkitover commented Jun 1, 2022

I just want my nanorc to work, regardless of whether I am in an unelevated or elevated shell, without having to place copies or symlinks to files in weird places on the filesystem. I suspect other people have an interest in this too.

which suggests a different color scheme be used for nano running as root for distinction.

Is this really important right now? Perhaps in the future, when your project makes an installer package with a default nanorc for elevated users in a standard location. But in the current situation, this is a huge inconvenience for no gain.

One reason for not using Administrator/.nanorc is that this account is disabled by default, and does not have a profile directory unless it is enabled and logged in once.

And for the reason that Administrator is not equivalent to root on UNIX. Few people even use this account anymore in the last two decades.

As getenv("ALLUSERSPROFILE") on my system is C:\ProgramData it would make more sense to use %ALLUSERSPROFILE%\nano\nanorc as the equivalent of /etc/nanorc like OpenSSH for Windows does, and make a default %ALLUSERSPROFILE%\nano\elevated_nanorc for elevated users but use it ONLY if they don't have a profile nanorc of their own.

@akuropka
Copy link

akuropka commented Jun 1, 2022

I am wondering why not just using USERPROFILE since this always points to the current user matters not if elevated or not. At this point we need to acknowledge the elevation system of Windows is different to the superuser of Linux.

However, I created a batch wrapper starting nano with the --rcfile option. This works out of the box, matters not which environment or if elevated or not.

@rkitover
Copy link
Author

rkitover commented Jun 3, 2022

However, I created a batch wrapper starting nano with the --rcfile option. This works out of the box, matters not which environment or if elevated or not.

That's what I'll end up doing for my installer script and possibly the chocolatey package if I ever update that if I can't get any traction here.

@lhmouse
Copy link
Owner

lhmouse commented Jun 4, 2022

It is necessary to have a distinct color scheme for elevated instances. This is not specific to nano; almost all modern text editors have (Administrator) annotation in their titlebars, if run elevated. The Linux behavior may be unintuitive, but there has to be a solution, rather than nothing at all.

@rkitover
Copy link
Author

rkitover commented Jun 4, 2022

Ok, but what is your solution to the much more annoying problem of your nanorc not being read in an elevated shell.

@lhmouse
Copy link
Owner

lhmouse commented Jun 4, 2022

Why don't you make a copy of your nanorc to where an elevated instance will read, just like what others do on Linux?

@rkitover
Copy link
Author

rkitover commented Jun 4, 2022

Because I don't want to maintain or install two copies, and there is no reason to do this. I had some suggestions for more reasonable locations and behavior above.

@lhmouse
Copy link
Owner

lhmouse commented Jun 5, 2022

Because I don't want to maintain or install two copies, and there is no reason to do this. I had some suggestions for more reasonable locations and behavior above.

While I understand your emergency, this behavior does not come without a reason. If maintaining two copies of nanorc is an issue for you, please file a feature request at https://savannah.gnu.org/bugs/?group=nano, or send a message to [email protected].

@rkitover
Copy link
Author

rkitover commented Jun 5, 2022

While I understand your emergency, this behavior does not come without a reason. If maintaining two copies of nanorc is an issue for you, please file a feature request at https://savannah.gnu.org/bugs/?group=nano, or send a message to [email protected].

This has nothing to do with Linux or nano as I explained in great detail here already. The behavior of your port is wrong and your code is wrong. Why are you purposefully insisting on a huge inconvenience for your users? There isn't even any documentation for this.

@FrankHB
Copy link

FrankHB commented Jun 5, 2022

Because I don't want to maintain or install two copies, and there is no reason to do this. I had some suggestions for more reasonable locations and behavior above.

While I understand your emergency, this behavior does not come without a reason. If maintaining two copies of nanorc is an issue for you, please file a feature request at https://savannah.gnu.org/bugs/?group=nano, or send a message to [email protected].

It is not clear which exact targets are supported by the upstream project. They will have legitimate reasons to reject any changes relying on the targets not ever supposed to be support officially, unless you merge this project to the upstream.

@FrankHB
Copy link

FrankHB commented Jun 5, 2022

it matches the behavior on Linux, where a sudo'd process reads /root/.nanorc instead of the user's nanorc.

You are talking about sudo, not su. Are you really sure this is independent to sudo's options and/or the configurations in /etc/sudoers?

It is true that impersonating in an elevated process would be dangnerous for an editor which may occasionally overwrite some files unexpectedly, but the problem of the resulted file permissions in such cases should already be advertised and users should have been educated to use it anywhere. Ruling out the user profile directory (indicated by HOME, USERPROFILE or whatever) by enforcing its change during the impersonating by default seems stupid in general. This is just like that you refuse to use scoping rules (with inheriting) in resolving varaible names in the progamming language by default, but eval with explicit environment (or implicitly the global environment, like root for su and sudo) everywhere instead. (OK, there are actual languages do this, e.g. JavaScript, Lua, and many shell languages.)

The default behavior (with default configurations) chosen by sudo is specific to itself. This is never a system-wide requirement. Except to be closer to su, I see it inconvenient to many cases, where users actually want a impersonated shell instead a real su session.

@rkitover
Copy link
Author

rkitover commented Jun 5, 2022

The real issue here is that this original code is a misguided effort to match the behavior on Linux, but is wrong because Windows works differently.

On Linux, sudo -E will preserve the value of HOME but the real home directory as returned by getpwent() etc. will still be /root.

On Windows, an admin user that is elevated does not become another user, he is simply elevated and both USERPROFILE and his profile directory as returned by Windows APIs remains the same, hence the default home directory should remain USERPROFILE.

Please fix this, either merge my PR or do your own fix. I can work with you on improving the whole situation with finding the config file, if you like. I had some suggestions above.

@lhmouse
Copy link
Owner

lhmouse commented Jun 6, 2022

The real issue here is that this original code is a misguided effort to match the behavior on Linux, but is wrong because Windows works differently.

How do you define 'wrong'? Something that looks wrong to you might not look so to others.

On Linux, sudo -E will preserve the value of HOME but the real home directory as returned by getpwent() etc. will still be /root.

This is valid. There are indeed issues in the current approach: All Users might be a bad candidate for elevated processes, and relying on environment variables is unsafe.

But, as explained many times already, it is necessary for an elevated instance to load nanorc from a location that does not depend on the current user (one possible candidate could even be C:\WIndows\.nanorc).

On Windows, an admin user that is elevated does not become another user, he is simply elevated and both USERPROFILE and his profile directory as returned by Windows APIs remains the same, hence the default home directory should remain USERPROFILE.

Please fix this, either merge my PR or do your own fix. I can work with you on improving the whole situation with finding the config file, if you like. I had some suggestions above.

The XDG_CONFIG_HOME part looks good to me; I suggest you remove the code path not in _WIN32 though. The other won't be accepted. If you don't like it you are free to fork.

Only use ALLUSERPROFILE for home dir when USERPROFILE is not set, even
for admin users.
Fix lhmouse#34.

Also on Windows when looking for the rc, look in getenv("APPDATA")
instead of getenv("XDG_CONFIG_HOME"), which is mostly equivalent.

Signed-off-by: Rafael Kitover <[email protected]>
@rkitover
Copy link
Author

rkitover commented Jun 6, 2022

The real issue here is that this original code is a misguided effort to match the behavior on Linux, but is wrong because Windows works differently.

How do you define 'wrong'? Something that looks wrong to you might not look so to others.

This looks wrong to anyone but you, because you refuse to admit that your code is wrong and needs to be fixed.

It's wrong because:

  • It makes no sense.
  • No other UNIX port does this.
  • It does not enhance security in any way.

On Linux, sudo -E will preserve the value of HOME but the real home directory as returned by getpwent() etc. will still be /root.

This is valid. There are indeed issues in the current approach: All Users might be a bad candidate for elevated processes, and relying on environment variables is unsafe.

ALLUSERSPROFILE is actually C:\ProgramData, it should be C:\ProgramData\nano, and that should be where the equivalent of /etc/nanorc is.

Why is using an environment variable unsafe? You are just using another environment variable. If you prefer, you can use an API to get the user's profile directory.

But, as explained many times already, it is necessary for an elevated instance to load nanorc from a location that does not depend on the current user (one possible candidate could even be C:\WIndows\.nanorc).

Why should it not depend on the current user, that's exactly what it does on Linux. The root user uses root's home directory.

On Windows, an admin user that is elevated does not become another user, he is simply elevated and both USERPROFILE and his profile directory as returned by Windows APIs remains the same, hence the default home directory should remain USERPROFILE.
Please fix this, either merge my PR or do your own fix. I can work with you on improving the whole situation with finding the config file, if you like. I had some suggestions above.

The XDG_CONFIG_HOME part looks good to me; I suggest you remove the code path not in _WIN32 though. The other won't be accepted. If you don't like it you are free to fork.

That would still use the user's profile directory, C:\Users\<user>\AppData\Roaming\nano, without the other part it makes no sense. I removed the #ifndef.

I don't have time to fork this crap, I have enough to do, I was only interested in writing an installer script and possibly updating the chocolatey package, if this pile of crap would even be usable and not subject to incomprehensible undocumented behavior.

Bad enough it doesn't support Unicode, for an editor that's pretty bad.

@lhmouse
Copy link
Owner

lhmouse commented Jun 6, 2022

This looks wrong to anyone but you, because you refuse to admit that your code is wrong and needs to be fixed.

It's wrong because:

  • It makes no sense.
  • No other UNIX port does this.
  • It does not enhance security in any way.

It's too subjective. I mostly work on Linux, and it is reasonable to tell whether the editor is elevated (checking the the EUID) or not; OTOH, which UID that has effected the process is less important. I am not saying that sudo leave the UID unchanged; looking at nano source, there is no reference to getuid(), only geteuid().

Note it is possible to change the EUID. You may try this one yourself:

lh_mouse@lhmouse-xps ~ $ sudo UID=$UID EUID=0 bash -p
lhmouse-xps /home/lh_mouse # echo $UID $EUID
1000 0

This starts an elevated shell like on Windows. The real UID is me. I can start nano without becoming root, and it still reads /root/.nanorc.

So, an instance, whose UID is not root, but is impersonating root, reads the root's nanorc. It's probably less common, but it does exist.

Why is using an environment variable unsafe? You are just using another environment variable. If you prefer, you can use an API to get the user's profile directory.

Because I don't think it is unsafe here. Someone else may think so.

That would still use the user's profile directory, C:\Users\<user>\AppData\Roaming\nano, without the other part it makes no sense. I removed the #ifndef.

Thank you for the update.

I don't have time to fork this crap, I have enough to do, I was only interested in writing an installer script and possibly updating the chocolatey package, if this pile of crap would even be usable and not subject to incomprehensible undocumented behavior.

Bad enough it doesn't support Unicode, for an editor that's pretty bad.

MSVCRT doesn't support UTF-8. In order to get UTF-8 support it is necessary to link against UCRT and attach a manifest. Haven't tried it though.

@rkitover
Copy link
Author

rkitover commented Jun 6, 2022

This starts an elevated shell like on Windows. The real UID is me. I can start nano without becoming root, and it still reads /root/.nanorc.

So, an instance, whose UID is not root, but is impersonating root, reads the root's nanorc. It's probably less common, but it does exist.

This is not the same thing as an elevated shell on Windows. Windows does not change the UID or EUID, if it even has the concept of EUID, the user remains the same but runs with admin privileges.

I don't see how any of this is relevant.

Vim/NVim on Windows does not do anything like this, nothing does anything like this.

Why is using an environment variable unsafe? You are just using another environment variable. If you prefer, you can use an API to get the user's profile directory.

Because I don't think it is unsafe here. Someone else may think so.

You first said it's unsafe now you're saying it's safe.

MSVCRT doesn't support UTF-8. In order to get UTF-8 support it is necessary to link against UCRT and attach a manifest. Haven't tried it though.

MSYS2 now has the UCRT64 environment for using MINGW with UCRT.

https://www.msys2.org/docs/environments/

Usage is almost exactly the same as MINGW64.

@rkitover
Copy link
Author

Ok, I'm closing this.

I recommend making this change, but for my purposes I'll just find the config in a .bat file.

If you want help redoing the config locations more sanely, regardless of whether you want to use a separate admin config or not, I made some suggestions above, let me know.

@rkitover rkitover closed this Jun 15, 2022
@lhmouse
Copy link
Owner

lhmouse commented Jun 16, 2022

Would please propose a PR for the change for XDG_CONFIG_HOME? Actually I was unsure what it meant and left it alone; but your commit was of course explanatory and desirable, and shall be appreciated.

@rkitover
Copy link
Author

I will open an issue with some suggestions, and we can go from there.

Sign up for free to 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.

nanorc not found in elevated shell
4 participants