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

"Require admin rights" option for Windows exes #341

Merged
merged 7 commits into from
Jan 15, 2024

Conversation

Lilianne-Blaze
Copy link
Contributor

@Lilianne-Blaze Lilianne-Blaze commented Jan 7, 2024

Probably needs some more testing just in case, works on three Win 10/11 machines here

Closes #340

import java.io.InputStream;
import java.util.Arrays;

public class FileUtils {
Copy link
Collaborator

Choose a reason for hiding this comment

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

final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to. It's a bag for copy-paste funcs anyway.

Comment on lines 80 to 84
// intentionally non-static non-final so it can be hacked with reflection if someone really needs to
private String DEF_REQADMMAN_RES = "META-INF/resources/manifest-requireAdminRights-v1.xml";

// intentionally non-static non-final so it can be hacked with reflection if someone really needs to
private String DEF_REQADMMAN_FILE = "target/manifest-requireAdminRights.xml";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not to make them parameters with default values? Users can set them via pom.xml without a need to hack via reflection.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And should this point to target/classes? In other case the file won't be included in the final JAR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to, but if a user needs to change it they can just use "manifest" parameter and get exactly same functionality?
I was thinking more about an ability to quickly patch it in a subclass in case there are some compatibility issues with that particular manifest. It works and I used it many times before, but I can't say I'm 100% certain it's fully compliant with whatever Microsoft intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't matter, it's for system's use, not the program's. Would /target/classes/META-INF be ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will be a better place

File manFile = new File(basedir, DEF_REQADMMAN_FILE);
byte[] manBytes = FileUtils.readResourceAsBytes(DEF_REQADMMAN_RES);

FileUtils.writeBytesIfDiff(manFile, manBytes);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not to always overwrite the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No point in touching it if it's already there and correct

@lukaszlenart
Copy link
Collaborator

conflict

@Lilianne-Blaze
Copy link
Contributor Author

conflict

Done

@@ -78,6 +79,12 @@ public class Launch4jMojo extends AbstractMojo {
@Parameter(defaultValue = "net.sf.launch4j", required = true)
private String launch4jGroupId;

// intentionally non-static non-final so it can be hacked with reflection if someone really needs to
private String DEF_REQADMMAN_RES = "META-INF/resources/manifest-requireAdminRights-v1.xml";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I probably still missing something here. Maven automatically process anything from src/main/resources into target/classes. So if there is src/main/resources/META-INF/manifest-requireAdminRights-v1.xml it will be automatically copied into target/classes/META-INF/manifest-requireAdminRights-v1.xml - this is controlled by Maven Resource Plugin.

Isn't this enough to achieve what you want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically this is strictly QoL, it can be done with unmodified version by creating (or more likely copy-pasting from Google) a manifest.xml file, then setting the "manifest" param to point to that file.
This patch simply makes it a quick, newbie friendly true/false switch.

People are still asking how to use Launch4j to require admin even though it's easy to find - with this you can add "we officially support admin mode now" in a big juicy letters in the version history.

That's why I included the warnings "make sure this is what you want, write your own if you can", that file can specify half a dozen other options I won't pretend to understand fully. Something about DPI with high resolutions for example.
The file itself doesn't have to be included in the resulting exe-jar at all, it is merged as Windows resources, along with .ico file and configuration strings for Launch4j "head" part.

The relevant part is this:
https://github.com/mirror/launch4j/blob/c48081b48b4483663395e5736ed310904a611cc2/src/net/sf/launch4j/RcBuilder.java#L352

Other than that it's normally passed transparently by Launch4j plugin to Launch4j core to windres.exe" called by Launch4j core.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yet I cannot understand why a custom copy/paste operation is needed as this is already supported by the plugin.

Another thing if this doesn't have to be included in the final JAR and only integrated in the final exe file I would store the file in src/etc and do not copy it to other place. The manifest parameter should point to the file and that's all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure changing anything in /src would be a good idea (with the exception of plugins such as code formatters etc)
If I'm understanding you correctly then

  1. when the param is set to true, the manifest would be created somewhere in /src , an user that just wants it to work would have to idea why that file suddenly appeared and what to think about it. And their Git would propose to commit it which makes no sense.
  2. when it's switched back to false, that file would needlessly remain there, again possibly creating confusion.
    2a) or it could get deleted, but deleting stuff from /src by a plugin seems even worse than adding.
  3. worse, if a user decided to customize it, their changes would be overwriten on next build.

I think, considering the above, that would defeat the "on/off, just works" idea.

But maybe just create it in %temp% ?
Leaving it in /META-INF seems reasonable but then it could confuse users when they try to delete it from a resulting exe-jar using 7z or another ZIP software, and then don't understand why the exe still asks for admin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, but this is the plugin and not a user's project. So you need a way to distribute the manifest-requireAdminRights-v1.xml file with the plugin so it must be integrated into plugin's JAR. Then in mojo there must be a logic to read the file from resources via getClass().getResource("manifest-requireAdminRights-v1.xml") and copy into a folder (can be %temp) while preparing the exe file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I'm getting confused

1 Right now it's in here
launch4j-maven-plugin\src\main\resources\META-INF\resources\manifest-requireAdminRights-v1.xml

2 When plugin gets built it gets copied to here
launch4j-maven-plugin\target\classes\META-INF\resources\manifest-requireAdminRights-v1.xml

3 And packed here
launch4j-maven-plugin-2.4.1-test.jar\META-INF\resources\manifest-requireAdminRights-v1.xml

4 When user project is built it gets copied (if needed) here
demo-project\target\classes\META-INF\manifest-requireAdminRights-v1.xml

5 Then packed here
demo.jar\META-INF\manifest-requireAdminRights-v1.xml

6 Then linked to exe-jar from demo-project\target\classes\META-INF\manifest-requireAdminRights-v1.xml

Which step(s) exactly would you like to change and how?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right I got tricked by too many resources in the path :D

@lukaszlenart lukaszlenart merged commit c3a9f46 into orphan-oss:master Jan 15, 2024
4 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.

Please add "require admin" option (patch included)
2 participants