Skip to content

Conversation

@ImperatorS79
Copy link
Contributor

@ImperatorS79 ImperatorS79 commented Jun 2, 2019

This PR will also need a PR in phoenicis in the display/edit shortcut code.

However, I did not manage to send custom environment variable to the shortcut create method. The environmentVar variable is always undefined. I tried to send [[DXVK_HUD, 1], [COUCOU, 27]].

It aims to fix #869 .

Merge Phoenicis/Scripts:master into ImperatorS79/Scripts:master
updated Hearthstone (download application directly) (PhoenicisOrg#383)
@plata
Copy link
Collaborator

plata commented Jun 3, 2019

@ImperatorS79
Copy link
Contributor Author

When creating a shortcut from a script (using new WineShortcut) , I tried to send the environment variable to the create method, but inside the code of the create method the variable is still undefined.

@plata
Copy link
Collaborator

plata commented Jun 3, 2019

Have you tried to add some print outs to see where it gets lost?

@ImperatorS79
Copy link
Contributor Author

If I send the json object {DXVK_HUD: "1", COUCOU: "27"} directly to the function, it says it cannot parse value for DXVK_HUD :/.

@plata
Copy link
Collaborator

plata commented Jun 5, 2019

Does this also solve PhoenicisOrg/phoenicis#192 ?

@ImperatorS79
Copy link
Contributor Author

No, it does not ^^.

@plata
Copy link
Collaborator

plata commented Jun 5, 2019

Ok, I thought it might work to add "/usr/bin/ck-launch-session", "" as environment.

@ImperatorS79
Copy link
Contributor Author

That is not the way to solve the problem, you need to use xinit to start another x-server, and the line you are talking about is just a special thing to do on ubuntu before xinit.

And what you propose will not work due to the fact the code use envronment.put().

This was referenced Jun 6, 2019
@ImperatorS79 ImperatorS79 changed the title Read environment variable from shortcut Read environment variables and trust level from shortcut Jun 7, 2019
@plata
Copy link
Collaborator

plata commented Jun 7, 2019

Okok, was just a quick thought when looping over the open issues.

}

if (userData["trustLevel"] == "0x20000" && distribution == "staging") {
var runasArgs = org.apache.commons.lang.ArrayUtils.addAll(["/trustlevel:0x20000", executable], args);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is really ugly. Please split it in two:

const ArrayUtils = Java.type("org.apache.commons.lang.ArrayUtils");
const runAsArgs = ArrayUtils.addAll(["/trustlevel:0x20000", executable], args);

@plata @qparis @ImperatorS79 ideally we do this change for every usage of Java classes and move the const JavaClass = Java.type(...); parts to the beginning of the files directly beneath the includes.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, Ideally, we should avoid calling directly java objects and use beans whenever it is possible.
This would make the scripts more agnostic of the main application

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 have just copied the few lines above ^^.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't directly calling Java objects basically the same as using (Java) beans? Both are Java objects underneath with the only difference that beans do not require Java.type(...).

If we want to do this in a clean way we will need to remove any kind of Java objects/classes from the script repository and access JavaScript libraries instead. If we do this the only interaction between the Java side and the Script side would be through method invocations from the Java side.

Copy link
Member

@qparis qparis Jun 14, 2019

Choose a reason for hiding this comment

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

I see two advantages of using beans:

  • It could be implemented in any language :) Basically it would just represent a method that gets a property from an application context map. It makes our scripts more portable and testable in a future.
  • Later, we can take advantage of it to add security feature. (I.e. block all access to classes outside phoenicis)

Copy link
Member

@qparis qparis Jun 14, 2019

Choose a reason for hiding this comment

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

Not totally. I would prefer Native.type instead of Java.type. In addition, and more important than the naming in my point of view: When you allow guest classes, you may need to reimplement the universe to port your script. If we have a clear and managed set of Beans interface you have to implement, it is a lot easier.

If we don't use any kind of Java objects from inside the scripts, we can simply forbid access to all kinds of Java objects from inside the scripts. I believe this would lead to the best security, right?

Forbid acess to all kinds of Java objects, except beans that handle correctly the security (like filesystem access, etc...)

Copy link
Member

Choose a reason for hiding this comment

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

But I think it is something we could discuss about in another issue

Copy link
Collaborator

@madoar madoar Jun 14, 2019

Choose a reason for hiding this comment

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

Yes. Still for now I would prefer to move all Java class imports to the beginning of the script files. This also makes it easier for us to find the Java object usages later on, because they are all in one place.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fair

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ImperatorS79 can you please try

const runAsArgs = ["/trustlevel:0x20000", executable].concat(args)

alternatively? This will avoid the Java dependency totally.

@madoar
Copy link
Collaborator

madoar commented Jun 22, 2019

@ImperatorS79 it seems like this PR needs to be rebased because of my changes done to the shortcut reader.

@ImperatorS79
Copy link
Contributor Author

I will wait for the quick script changes to be merged first.

@plata
Copy link
Collaborator

plata commented Jun 22, 2019

That's done.

@ImperatorS79
Copy link
Contributor Author

Seems to still work.

if (userData["trustLevel"] == "0x20000" && distribution == "staging") {
const runAsArgs = ["/trustlevel:0x20000", executable].concat(args);
userData["trustLevel"] = "0"; //avoid infinite loop
return this.run("runas", runasArgs, workingDir, captureOutput, wait, userData);
Copy link
Collaborator

Choose a reason for hiding this comment

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

runAsArgs

@ImperatorS79 ImperatorS79 merged commit a1cb1a2 into PhoenicisOrg:master Jun 28, 2019
@ImperatorS79 ImperatorS79 deleted the EnvVar branch August 24, 2019 16:33
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.

Run Wine app with custom environment variables

4 participants