support jcasc and renovate plugin

[StefanSpieker]

Started at Contributor summit. Thanks a lot @basil and @nre-ableton. It was quite some fun doing this with you together. I think I finally managed to get it compatible with JCasC, still some fine-tuning and testing to do ...

So after some iterations it was a quite big renovation, including:

• JENKINS-53442 JCasC support
• renovating the Plugin internally to fix some remaining deprecation warnings
• moving the configuration into global config /manage
• adjust the restore styling to the latest design
• adding TOC to the README
• replaced org.apache.commons.lang.StringUtils usage with native Java calls
• improved the test coverage
• added PermissionChecks and CSRF protection

This also comes with BREAKING CHANGES:

• The config filename changed (from thinBackup.xml to org.jvnet.hudson.plugins.thinbackup.ThinBackupPluginImpl.xml) and will be automatically converted after the first start with the new version. The new name complies with the newer naming convention, which was automatically adjusted because of the internal renovation.

Testing done

Submitter checklist

Edit tasklist title

Beta Give feedback Tasklist Submitter checklist, more options

• Rename
• Copy markdown
• Delete tasklist

Delete tasklist

Delete tasklist block?

Are you sure? All relationships in this tasklist will be removed.

Cancel Delete

1. Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!

   Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
2. Ensure that the pull request title represents the desired changelog entry

   Ensure that the pull request title represents the desired changelog entry
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
3. Please describe what you did

   Please describe what you did
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
4. Link to relevant issues in GitHub or Jira

   Link to relevant issues in GitHub or Jira
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
5. Link to relevant pull requests, esp. upstream and downstream changes

   Link to relevant pull requests, esp. upstream and downstream changes
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

   Ensure you have provided tests - that demonstrates feature works or fixes the issue
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove

Add item to Submitter checklist

[sbandaru08]

Hi Stefan Spieker,

I hope this message finds you well. I wanted to touch base regarding the upcoming deployment of the change you have in this PR. Could you provide an estimated timeline for when we can expect this to be fully deployed?

I'm quite excited about the new features. Is there a possibility for me to install a beta version in the meantime? This would allow me to start exploring the new capabilities and provide any early feedback that might be useful.

Looking forward to your response and eager to get started with the new enhancements.

Best regards,
Satish

[StefanSpieker]

Hi.
I still have some minor things to sort out. In general JCasC already works. The screen for restoring a backup does not work yet and I need a solution for converting the settings.
So I will not promise anything, but I guess I will continue on the weekend. If you are interested, I might be able to provide a version to test early next week.

[basil]

Finally getting to this PR after coming back from the Contributor Summit. Sorry it took so long. I hope you don't mind that I took the liberty of formatting the codebase to make this change easier to review.

Great job cleaning up this very old code! think adding an HtmlUnit test for the GlobalConfiguration as in the archetype will help flush out and fix the remaining issues and give us confidence in this refactoring.

[StefanSpieker]

Thanks a lot @basil for all the suggestions! That really helped bring up some confidence that the introduced changes really work. It also helped increase my understanding of how to test within the Jenkins ecosystem. If you have the time, it would be great if you could check if I managed to implement your suggestions correctly.

@sbandaru08 You could give it a try, if you like. My tests were quite successful, I will test it on another system before releasing it. So if you want to give it a try, you can grab it from ci.jenkins.io and upload it manually to your instance.

[mawinter69]

The configuration of the plugin is still on it's own page backupsettings.jelly with the Management link method doSaveSettings acting as proxy to save the configuration by calling each setter.
I think it would be better to migrate that jelly to config.jelly under ThinBackupPluginImpl so that it appears under the system configuration. Saving would then make use of the configure method and use the stapler binding.
There is also one issue with saving configuration. You should implement your own configure method and make sure that the configuration is saved afterwards. Ideally each setter method calls save() at the end and you use BulkChange in the configure method to avoid that each setter calls save

[StefanSpieker]

In many places Jenkins.getInstanceOrNull() is used with a following check for null This should be replaced with Jenkins.get() and remove the null check.

Thanks a lot, this is a technical debt because of intensive mocking usage. I need to move to Jenkinsrule and stop mocking everything. I guess this will also improve the test coverage and improve the tests.

[StefanSpieker]

@mawinter69 Thanks a lot again. I think I have integrated all your suggestions and have learned quite a few things on the way. I will test this a little more and bring a fresh new version 2 out. @sbandaru08: I think it is now really ready to test, if you like and have the time.

[basil]

Looks good overall, and interactive testing works fine. Very nice job on renovating this plugin!

The one thing I would suggest before releasing this (I know, there seems to be an endless list of tasks with this plugin) is to get Jenkins Security Scan enabled on trunk and then confirm this PR is not creating any new security warnings. Old plugins like this often have existing security scan warnings, many of them false positives, but with a substantial rewrite like this, it's always worth checking that new problems aren't being introduced, and the security scan is a great way to do that automatically. If we aren't sure about something, we can always ask for a review from the security team before releasing it, which is always better than a security vulnerability being detected after a release!

[basil]

Perhaps throw new UncheckedIOException(e); would be more desirable than logging and driving on, following the general principle of failing fast, but no strong preference here.

[StefanSpieker]

Looks good overall, and interactive testing works fine. Very nice job on renovating this plugin!

The one thing I would suggest before releasing this (I know, there seems to be an endless list of tasks with this plugin) is to get Jenkins Security Scan enabled on trunk and then confirm this PR is not creating any new security warnings. Old plugins like this often have existing security scan warnings, many of them false positives, but with a substantial rewrite like this, it's always worth checking that new problems aren't being introduced, and the security scan is a great way to do that automatically. If we aren't sure about something, we can always ask for a review from the security team before releasing it, which is always better than a security vulnerability being detected after a release!

Thanks a lot for your constant improvement suggestions. I already learned a lot about how things work and also understand the plugin a lot better! 😉 I really enjoyed polishing this plugin and will try to contribute some learnings to the documentation. I hope that it might be easier for the next one to work on these things.
I'm seeing a couple of Missing POST/RequirePOST annotation and Stapler: Missing permission check. Furthermore, I guess it is fine to fix these warnings. How can I trigger a PR scan?

[basil]

I'm seeing a couple of Missing POST/RequirePOST annotation and Stapler: Missing permission check. Furthermore, I guess it is fine to fix these warnings. How can I trigger a PR scan?

Many (but not all) of those "missing POST annotation" or "missing permission check" warnings are false positive: the very detailed guidance from the security team in the security scan results should help you determine whether that is the case. It is possible but rare for the security scan to turn up legitimate existing issues; an example of this was in Email Extension. The main thing to check is that you aren't introducing any new issues in this PR. You should be able to merge the main branch in, and then GitHub Actions should run the security scan against this PR and flag any new issues, if found.

[StefanSpieker]

Thanks again @basil! The documentation is great. Since I'm only using the methods for validation of an admin form, I decided that it will not harm to restrict that also to ADMINISTER. Adding @POST should also not harm, because the documentation states that the doCheck...() can safely be annotated with that, because the used Jenkins version is new enough and anyhow uses POST.
I do not want to be cheeky, but if you have the time, could you take another look? Or should I ask someone from security to verify that I do not introduce new problems?

[basil]

It looks like you applied the maximum strength fix (requiring POST and adding missing permission checks everywhere) to all security scan warnings, in which case I don't see a need to ask for security team review. Most such checks are probably overkill, but it doesn't hurt. I could see a need for security team review if you were on the fence about whether or not a warning was a false positive, but that doesn't seem to be the case if you've applied the maximum strength fix to all warnings.

[MutenR0sh1]

Thanks for the renovation. Works fine in our setup.

[nre-ableton]

Very nice! 🎉