Update Upgrade Process to remove Telerik (WIP)

[daguiler]

Closes #4888

Summary

This PR adds a new module to uninstall the Telerik components.

This module can operate unattended, as part of the upgrade process if the user chooses to remove Telerik in the Security tab of the Upgrade Wizard, but it can also run interactively, if dropped on a page.

The module is installed during upgrade even if the user chooses not to remove Telerik. In this case, the user can drop the module on a page later on, and run it interactively.

The module removes Telerik by executing the steps documented in the removal process.

I'll paste some screenshots below in the comments.

[daguiler]

The wording is the same as in the previous PR.

If Telerik is Installed but not used, you get this:

You need to accept the backup checkbox, and then the button gets enabled:

Removal report:

Removal report if previously executed:

[bdukes]

This looks great. It looks like it's a standalone module. Is there a plan for integrating this into the upgrade process? Thanks!

[bdukes]

Also, ideally we'd rebase these changes and #5093 on release/9.11.0 and use that branch as the base for these PRs. It's not a big deal since we don't intend to release a 9.10.3 release, but if we needed to we'd like the flexibility.

[daguiler]

Is there a plan for integrating this into the upgrade process?

Yes, that was my intention, but I don't really know how to do that.
I have a commit already to make this a proper extension (add a manifest, add the Module.build target, license, etc.) and I tested it and it works fine, except the extension is left there as an "available" extension, meaning it doesn't get installed automatically. And also I wouldn't know how to "run" an extension automatically as part of the upgrade process, if that makes any sense and is at all possible. Maybe we can just ship this as an available extension and let users deploy it to a test page and run it manually?

[bdukes]

If you add an eventMessage component to the manifest and implement IUpgradeable you can run logic when the extension is installed. If that's the way we want to go, I would think we'd need some kind of setting that the IUpgradeable implementation would check so that it only runs the uninstall logic if it's an upgrade (and not if someone manually installs the module).

Alternatively, take the uninstall logic, move it into the installer, and run it directly as an install step, instead of inside of a separate module.

[daguiler]

Thanks @bdukes, I wasn't aware of the eventMessage / IUpgradeable tandem.
Maybe the ApplicationStatusInfo.Status property can tell us whether it's an upgrade or a manual installation?

[bdukes]

Yeah, I was thinking Status would probably work 👍🏻

[daguiler]

[david-poindexter]

I just want to say that getting a red cross on each build because of the missing label is demoralizing 😃

Yeah, it was put On Hold to prevent accidental merging prior to the resolution of a chosen path. Thanks for the patience.

[EPTamminga]

@david-poindexter: My opinion is that DNNPlatfporm should take care of its own problems and not do anything with code of others.

So: if the Telerik DLL is the DNNPlatform version, than DNNPlatform should do what is must do.

If the Telerik DLL is NOT the DNNPlatform version: create a warning but don't mess with the installation, that is not the responsibility of the platform.

Compare this with other types of extensions. If other (none platform) extensions have potential security issues because of the use of custom code or external (none platform) libraries, DNNPlatform is not responsible. It would be wrong if the platform starts messing with other peoples code.

[david-poindexter]

@EPTamminga there is unfortunately no solid programmatic way to differentiate the two. Comparing versions, as @mitchelsellers mentioned, has significant ramifications.

[EPTamminga]

@david-poindexter The message that Mitch proposed, would be a workable solution, although the second part of the message (the warning) can be applied to any kind of none-Platform extension.

[mitchelsellers]

The reason for the additional note @EPTamminga is that if you leave DNN stuff that is dependent upon Telerik (Digital Asset Management), RAD Editor Provider, the wrapping components, etc. you are leaving issues on the table that aren't just fixed by having a new Telerik version.

At the end of the day, due to how Telerik was initially included & supported, the usage of Telerik in DNN is riddled with some real negative connotations that are hard to workaround.

Simply doing nothing, MAY be ok, but it might not be, and I don't know that we can properly outline exactly all of those situations. So, the more I think about this, I think we might need to get a bit more verbose and implement the following.

If Nothing is found to use Telerik:

Do as we are now, suggest the removal

If Telerik is found, regardless of version

Check to see if the "Digital Asset Management" is installed, of so, show the following message

Your installation is utilizing Telerik, in the following assemblies, including the Digital Asset Manager module. There are known security issues with the DNN Distributed version of Telerik and the usage of Telerik by DNN modules, such as Digital Asset Management. If you have otherwise complied with Telerik security patching via other means, you should at a minimum uninstall Digital Asset Manager and replace it with the newer Resource Manager module, however, if you have not made any security adjustments the modules listed above should be updated/replaced and then Telerik removed to ensure proper security of your installation.

If "Digital Asset Management" is not found

Your installation is utilizing Telerik in the following assemblies, it does appear that you have removed a portion of the DNN Platform extensions that depended on Telerik. We are unable to remove Telerik for you, however, please ensure that you have in fact properly patched Telerik and removed all DNN Platform distributed Telerik components per the guidance found here https://docs.dnncommunity.org/content/getting-started/setup/telerik-removal/index.html

I don't necessarily like this, but its about the safest way we can provide true guidance? @dnnsoftware/approvers any thoughts?

[EPTamminga]

@mitchelsellers Well said. This gives a proper explanation.

[daguiler]

I have made updates per the comments above.
Plus, I added the found Telerik version number and made tiny wording adjustments. By all means, I can change this back if needed.

It looks like this now:

Telerik installed and used, and DAM present:

Telerik installed and used, DAM not present:

[daguiler]

Updated the Upgrade Wizard as well:

[EPTamminga]

@daguiler What is the message if you have a up to date Telerik.dll?

[mitchelsellers]

We cannot differentiate between “up to date” and not.

[EPTamminga]

@mitchelsellers I meant: a Telerik DLL that was NOT provided by DNNPlatform and has a version that is LATER/HIGHER than a version that was provided in the platform.
A version provided in the Platform is wrong for sure.

[donker]

@mitchelsellers I meant: a Telerik DLL that was NOT provided by DNNPlatform and has a version that is LATER/HIGHER than a version that was provided in the platform. A version provided in the Platform is wrong for sure.

From my understanding the check is twofold: (1) just on dll name and (2) dependencies of other DLLs. So this will NOT pick up on "up to date" Telerik dlls if they have the same name. However ... if any component depends on that new Telerik, then it would be pciked up by the second step.

[daguiler]

Do we need to adjust the Security Check?
Other than including the found Telerik version number, I made no other changes, and this table shows current logic:

Notice the Unverified wording (only seen in case of internal error). Is that sentence still true?
Also the last row is there just for completeness because there is no condition currently that leads to that outcome.

[mitchelsellers]

@daguiler The check is totally fine!

Do we have an idea of next-steps here for this one. Some initial testing was done and we did not see Telerik actually get removed in a few initial tests off of this branch.

[mitchelsellers]

@daguiler Will you be able to finish this one up at all in the near future? Anything we can help with?

[daguiler]

Hi @mitchelsellers
I'm sorry I did not reply sooner.
I changed job and I don't think I'll be able to find time to test this again in the short term.
Last time I checked though, this was working fine.

The uninstall process is designed to work like this:

1. The Upgrade Wizard asks the user whether to remove Telerik or not in the Security tab.
2. The Upgrade Wizard stores this decision as a host setting, named "telerikUninstallOption".
3. The upgrade process starts, and the TelerikRemoval extension is installed during upgrade
4. This extension's manifest includes an "upgrade event message", handled by the UpgradeController class
5. The UpgradeController handler checks for the telerikUninstallOption host setting previously created, and if its value is Y, it proceeds to uninstall Telerik via the TelerikUninstaller class.

[mitchelsellers]

@daguiler Thanks for the update. @dnnsoftware/approvers we might want to call a meeting to review next-steps to help get this one over the finish line

[david-poindexter]

Best wishes in your new job @daguiler

[bradignite]

Just dropping in to say an engineer from Ignite has picked this up and is doing a deep dive into the PR.

[mitchelsellers]

This is replaced by #5166