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

Refactor/win_elevation : Ask for elevation if not elevated #53

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Refactor/win_elevation : Ask for elevation if not elevated #53

merged 3 commits into from
Dec 7, 2023

Conversation

AdrasteonDev
Copy link
Contributor

No description provided.

@jolexxa
Copy link
Member

jolexxa commented Dec 5, 2023

Had a chance to run this on Windows 10 Pro with and without UAC -- Without UAC, everything just worked, no administrator prompts. With UAC turned all the way up, it was able to install Godot, but hung indefinitely when trying to update the environment variable. I was running this on my administrator account.

I'm not sure what needs to be done, but it doesn't seem as robust as the one that spammed the elevation prompts. I'm open to ideas.

@AdrasteonDev
Copy link
Contributor Author

I've just tested it again on my machine and I don't have the same problem with updating environment variables. I don't know why it doesn't work for you with the code from my branch...

Did you have the administrator prompt with UAC turned all the way up ? When I tried for the first time the code from chickensoft-games:refactor/win-elevation, I had a similar problem (without an admin prompt) because the content of "CreateInstance" was not completely defined. But I didn't think this behaviour would be encountered with the version of the code in AdrasteonDev:refactor/win-elevation.

@AdrasteonDev
Copy link
Contributor Author

For your information, while doing other tests, I also noticed a bug with my code that I hadn't seen before for non-administrator users. This does not affect other administrator users.

When elevated, Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData, Environment.SpecialFolderOption.Create) returns the AppData folder of the administrator rather than the current user. Because of this, it installs Godot in the AppData folder of the administrator and not the current user.

I've seen that UacHelper allows to check whether a user is an administrator or not. Maybe I'll see what happens if I use UacHelper again, re-executing the command as an authorised user only if the user is an administrator. This would at least avoid "spamming" the admin prompt for the case of admin users (which should cover most Godot users outside a professional context). I will probably add a commit for that in this branch.

By the way, the bug also affect GodotEnv 1.6.0 for non-administrator users if the command is executed in a Windows shell that has been run as administrator (I don't know if it can be fixed).

Nota bene: No problem for me if at some point you don't want to keep the changes from PR #51 and #53, and want to stick with the current behaviour. In that case, just remember to add a commit to undo the changes from this commit.

@AdrasteonDev
Copy link
Contributor Author

I've just pushed the commit

@jolexxa
Copy link
Member

jolexxa commented Dec 7, 2023

@AdrasteonDev Thank you for the thorough follow-up. I was able to re-test (using UAC all the way up and disabled on my admin account, same as before) and whatever you've done now works. With UAC on, I get a single prompt. With it off, everything works as expected.

I don't mind adding these changes, so I'll merge this into my PR and we'll get that in. I do suspect that handling administrator-ness on Windows might make things more complicated and there's probably more edge cases, but I'm not terribly worried about it.

@jolexxa jolexxa merged commit ac9d465 into chickensoft-games:refactor/win-elevation Dec 7, 2023
7 checks passed
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.

2 participants