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

Pre-register mod metadata during Windows / .deb installation. #13539

Merged
merged 8 commits into from Jul 9, 2017

Conversation

Projects
None yet
5 participants
@pchote
Member

pchote commented Jun 24, 2017

Requiring users to switch to a mod manually before it is available for switching caused a lot of confusion when release-20170421 and release-20170527 were first released. This PR fixes the issue for Windows and Linux (just deb here, but downstream packaging is also expected to adopt this) by pre-registering mods during installation.

A new "system support" directory is defined at:

  • Windows: C:\ProgramData\OpenRA\ (spec)
  • Linux: /var/games/openra/ (spec)
  • macOS: /Library/Application Support/OpenRA/

and a ModMetadata subdirectory is created and populated for ra/td/d2k. These are left read-only for normal users to avoid a malicious user from changing the LaunchPath to something nasty.

Closes #12764 and implements the first steps towards #10374.

@pchote pchote added this to the Playtest featuring updated HitShapes milestone Jun 24, 2017

@rob-v

Not tested yet ingame, small issues. Should I test the installation on Win? I don't know how to create new setup.

Show outdated Hide outdated OpenRA.Game/ExternalMods.cs
Show outdated Hide outdated OpenRA.Game/ExternalMods.cs
Show outdated Hide outdated OpenRA.Game/ExternalMods.cs
Show outdated Hide outdated OpenRA.Game/Platform.cs
Show outdated Hide outdated OpenRA.Game/Platform.cs
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote
Member

pchote commented Jun 26, 2017

@rob-v

small issues

Show outdated Hide outdated OpenRA.Game/ExternalMods.cs
Show outdated Hide outdated OpenRA.Game/UtilityCommands/UnregisterModCommand.cs
Show outdated Hide outdated OpenRA.Game/ExternalMods.cs
Show outdated Hide outdated OpenRA.Game/ExternalMods.cs
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 27, 2017

Member

Fixed.

Member

pchote commented Jun 27, 2017

Fixed.

@rob-v

Small bugs.
Utility can be used to register only standard mods, could it work for all mods? as e.g. cd --register-mod is refused.

Show outdated Hide outdated OpenRA.Game/ExternalMods.cs
Show outdated Hide outdated OpenRA.Game/ExternalMods.cs
Show outdated Hide outdated packaging/windows/OpenRA.nsi
Show outdated Hide outdated OpenRA.Game/ExternalMods.cs
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 28, 2017

Member

e.g. cd --register-mod is refused.

Refused how? I see no reason why this shouldn't work.

Member

pchote commented Jun 28, 2017

e.g. cd --register-mod is refused.

Refused how? I see no reason why this shouldn't work.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 28, 2017

Contributor

There is somewhere check for valid mods: The available mods are: cnc, d2k, modcontent, ra

Contributor

rob-v commented Jun 28, 2017

There is somewhere check for valid mods: The available mods are: cnc, d2k, modcontent, ra

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 28, 2017

Member

The --register-mod command won't be available in CD until they update their engine version to include this PR. The "official mods" engine installation can't see any mod installations, and similarly any mod installations shouldn't be able to see the "official mods" installation (and if they rely on this, then they should expect things to break because this is explicitly not supported).

Member

pchote commented Jun 28, 2017

The --register-mod command won't be available in CD until they update their engine version to include this PR. The "official mods" engine installation can't see any mod installations, and similarly any mod installations shouldn't be able to see the "official mods" installation (and if they rely on this, then they should expect things to break because this is explicitly not supported).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 29, 2017

Member

Side effect of this is that deleted mods won't be removed as it loops mods. It could loop .yaml files instead.

I ended up rewriting most of the ExternalMods registration and cleanup code to address this in a way that I was happy with.

I have put some effort into testing the various edge cases that may happen during normal (or abnormal) game launches, but ran out of time to test the utility commands and packaged versions on this new backend. I'll try and do that tomorrow if you don't spot anything first.

Member

pchote commented Jun 29, 2017

Side effect of this is that deleted mods won't be removed as it loops mods. It could loop .yaml files instead.

I ended up rewriting most of the ExternalMods registration and cleanup code to address this in a way that I was happy with.

I have put some effort into testing the various edge cases that may happen during normal (or abnormal) game launches, but ran out of time to test the utility commands and packaged versions on this new backend. I'll try and do that tomorrow if you don't spot anything first.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 29, 2017

Member

I strongly suggest reviewing by commit, using w=1 on "Read mod registrations from system support dir.".

Member

pchote commented Jun 29, 2017

I strongly suggest reviewing by commit, using w=1 on "Read mod registrations from system support dir.".

@rob-v

Some questions.

ExternalMod activeMod;
if (ExternalMods.TryGetValue(ExternalMod.MakeKey(Mods[modID]), out activeMod))
ExternalMods.ClearInvalidRegistrations(activeMod, ModRegistration.User);

This comment has been minimized.

@rob-v

rob-v Jun 29, 2017

Contributor

ClearInvalidRegistrations is called only once atm - ModRegistration.User and an activeMod, isn't call for System missing?

@rob-v

rob-v Jun 29, 2017

Contributor

ClearInvalidRegistrations is called only once atm - ModRegistration.User and an activeMod, isn't call for System missing?

This comment has been minimized.

@pchote

pchote Jun 29, 2017

Member

This is intentional. The system metadata is managed by the package manager / installer, and it would be bogus to delete arbitrary files from there ourselves.

@pchote

pchote Jun 29, 2017

Member

This is intentional. The system metadata is managed by the package manager / installer, and it would be bogus to delete arbitrary files from there ourselves.

This comment has been minimized.

@pchote

pchote Jun 29, 2017

Member

I guess i'll have to add another utility command to clear invalid System registrations from the Windows installer to cover the case where users don't uninstall things properly. What a pain.

@pchote

pchote Jun 29, 2017

Member

I guess i'll have to add another utility command to clear invalid System registrations from the Windows installer to cover the case where users don't uninstall things properly. What a pain.

This comment has been minimized.

@rob-v

rob-v Jun 29, 2017

Contributor

You could just remove ModRegistration from ClearInvalidRegistrations to make it simpler/clearer and keep it as is as it is quite complex (more versions, more system folders,...) for such not gameplay feature.

@rob-v

rob-v Jun 29, 2017

Contributor

You could just remove ModRegistration from ClearInvalidRegistrations to make it simpler/clearer and keep it as is as it is quite complex (more versions, more system folders,...) for such not gameplay feature.

This comment has been minimized.

@rob-v

rob-v Jun 29, 2017

Contributor

But it is now more consistent and ready for future so might be better.

@rob-v

rob-v Jun 29, 2017

Contributor

But it is now more consistent and ready for future so might be better.

This comment has been minimized.

@pchote

pchote Jun 29, 2017

Member

Modifying or removing the system-level registrations require elevated privileges because the files contain executable paths that could otherwise be modified to perform an arbitrary code exploit. That means that anything we do needs to be from the installer.

@pchote

pchote Jun 29, 2017

Member

Modifying or removing the system-level registrations require elevated privileges because the files contain executable paths that could otherwise be modified to perform an arbitrary code exploit. That means that anything we do needs to be from the installer.

This comment has been minimized.

@rob-v

rob-v Jun 29, 2017

Contributor

I meant removing argument ModRegistration registration and Platform.SystemSupportDir from ClearInvalidRegistrations, not removing system registrations. but it is also ok to keep though never used.

@rob-v

rob-v Jun 29, 2017

Contributor

I meant removing argument ModRegistration registration and Platform.SystemSupportDir from ClearInvalidRegistrations, not removing system registrations. but it is also ok to keep though never used.

// Continue to the next entry if this one is valid
if (File.Exists(m.LaunchPath) && Path.GetFileNameWithoutExtension(path) == modKey &&
!(activeMod != null && m.LaunchPath == activeMod.LaunchPath && m.Id == activeMod.Id && m.Version != activeMod.Version))

This comment has been minimized.

@rob-v

rob-v Jun 29, 2017

Contributor

only note: Can this happen? Register call before ClearInvalidRegistrations overwrites LaunchPath, Id and Version.

@rob-v

rob-v Jun 29, 2017

Contributor

only note: Can this happen? Register call before ClearInvalidRegistrations overwrites LaunchPath, Id and Version.

This comment has been minimized.

@pchote

pchote Jun 29, 2017

Member

Consider a person on windows installing release-20170421. This will write a metadata file ra-release-20170421.yaml pointing to ('ra', 'release-20170421', 'C:\Program Files\OpenRA\OpenRA.exe'). They then install release-20170527 over the top without uninstalling the old version first, so we get a new metadata file ra-release-20170527.yaml pointing to ('ra', 'release-20170527', 'C:\Program Files\OpenRA\OpenRA.exe') written alongside it.

When they then run the game the release-20175027 metadata will update itself, but the release-20170421 entry will not be touched until this line runs and discovers that we have a different version of RA claiming to run at the same launch path as the current version of RA, which we know is impossible.

We can't ignore all except the current version of RA, because we need to support switching between ra release and ra playtest installed at C:\Program Files\OpenRA (playtest)\RedAlert.exe.

@pchote

pchote Jun 29, 2017

Member

Consider a person on windows installing release-20170421. This will write a metadata file ra-release-20170421.yaml pointing to ('ra', 'release-20170421', 'C:\Program Files\OpenRA\OpenRA.exe'). They then install release-20170527 over the top without uninstalling the old version first, so we get a new metadata file ra-release-20170527.yaml pointing to ('ra', 'release-20170527', 'C:\Program Files\OpenRA\OpenRA.exe') written alongside it.

When they then run the game the release-20175027 metadata will update itself, but the release-20170421 entry will not be touched until this line runs and discovers that we have a different version of RA claiming to run at the same launch path as the current version of RA, which we know is impossible.

We can't ignore all except the current version of RA, because we need to support switching between ra release and ra playtest installed at C:\Program Files\OpenRA (playtest)\RedAlert.exe.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 29, 2017

Member

Added the --clear-invalid-mod-registrations utility command for use by the Windows installer. I think this should finally be ready to go.

Updated test installers:
https://www.dropbox.com/s/0ui7c6274cc16tx/OpenRA-modregistration-20170629.exe?dl=0
https://www.dropbox.com/s/04o5fvqhas1sh88/openra_modregistration.20170629_all.deb?dl=0

Member

pchote commented Jun 29, 2017

Added the --clear-invalid-mod-registrations utility command for use by the Windows installer. I think this should finally be ready to go.

Updated test installers:
https://www.dropbox.com/s/0ui7c6274cc16tx/OpenRA-modregistration-20170629.exe?dl=0
https://www.dropbox.com/s/04o5fvqhas1sh88/openra_modregistration.20170629_all.deb?dl=0

@rob-v

rob-v approved these changes Jun 29, 2017 edited

👍
2 small nits, that aren't worth the troubles with commits and are basically ok. I expect confirmation the test setup issue is unrelated.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 29, 2017

Member

I expect confirmation the test setup issue is unrelated.

I can confirm that the crashes happen with a packaged version of bleed, which means that it is either something weird with the system I packaged it on (likely) or some earlier change on bleed (also likely). Another topic for another issue/PR.

Member

pchote commented Jun 29, 2017

I expect confirmation the test setup issue is unrelated.

I can confirm that the crashes happen with a packaged version of bleed, which means that it is either something weird with the system I packaged it on (likely) or some earlier change on bleed (also likely). Another topic for another issue/PR.

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Jul 1, 2017

Member

In all the places doing sources.Distinct() on paths, do we need to consider using a case-insensitive string comparer on Windows?

Member

RoosterDragon commented Jul 1, 2017

In all the places doing sources.Distinct() on paths, do we need to consider using a case-insensitive string comparer on Windows?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 1, 2017

Member

Shouldnt matter in this case since we have full control over the paths going in to sources

Member

pchote commented Jul 1, 2017

Shouldnt matter in this case since we have full control over the paths going in to sources

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 7, 2017

Member

Fixed nits and rebased.

Member

pchote commented Jul 7, 2017

Fixed nits and rebased.

@reaperrr

Couldn't spot any issues (aside from a crash caused by the StyleCop issue, which isn't related to this PR), installation and ModMetadata are added and removed properly by the (un)installer.

@reaperrr reaperrr merged commit 1fa1677 into OpenRA:bleed Jul 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 9, 2017

Contributor

Sorry, I only noticed this now, but upon uninstall, it didn't remove the launcher icons from desktop (on Windows). I tested an earlier AppVeyor artifact from a different branch for comparison and there it worked, so it must have regressed here.

Contributor

reaperrr commented Jul 9, 2017

Sorry, I only noticed this now, but upon uninstall, it didn't remove the launcher icons from desktop (on Windows). I tested an earlier AppVeyor artifact from a different branch for comparison and there it worked, so it must have regressed here.

@pchote pchote deleted the pchote:mod-registration branch Oct 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment