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

Extend GitTagActionTest for more cases #667

Merged
merged 1 commit into from Jan 28, 2019

Conversation

MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Jan 28, 2019

SECURITY-1095 - Add more tests of GitTagAction

Tests are imperfect but better than no tests. The tests are able to run a Freestyle project and create two different tags in the workspace of the project.

The tests are not able to create the tags from the doSubmit method and thus are still unable to assert several important cases.

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No findbugs warnings were introduced with my changes
  • I have interactively tested my changes
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

  • Non-breaking change to improve an existing test

Further comments

Fixes for the original security issue (no CSRF protection on Git Tag Action) are included in git plugin 3.9.2.

Tests are imperfect but better than no tests.  The tests are able to
run a Freestyle project and create two different tags in the workspace
of the project.

The tests are not able to create the tags from the doSubmit method and
thus are still unable to assert several important cases.
Copy link
Member

@romenrg romenrg left a comment

Choose a reason for hiding this comment

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

LGTM.

However, I wonder if the calls to "waitForTagCreation" in lines 119 and 120 should be moved to immediately after each corresponding call to "createTagAction" (lines 113 and 116).

I'm suggesting that since I'm not 100% sure that the creation of the two tags is necessarily sequential (the backoff hints it might not be sequential, but I'm not familiar enough with the code to rule that out).

If the execution is always sequential, there is no problem and this comment can be ignored. However, in case it is not, there might potentially be a race condition causing the asserts in "waitForTagCreation" to occasionally fail, due to the tags potentially being created sometimes in the opposite order.

If that scenario is possible, this could be fixed by moving each of those waits to immediately after the corresponding tag creation, avoiding this potential race condition, since each case will be handled by the backoff of the wait function.

I just run the test a couple of times and they seem to always pass, so maybe the execution is always sequential and there is no risk. However, just in case I have created this PR with the proposed improvement: MarkEWaite#44

@MarkEWaite
Copy link
Contributor Author

Tag creation could be out of order and it should not affect the test. The waitForTagCreation wants to assure that both tags exist. It doesn't care about the order of creation and the assertion which follows it immediately also does not care about the order of creation

@MarkEWaite MarkEWaite merged commit df016c2 into jenkinsci:master Jan 28, 2019
@MarkEWaite MarkEWaite deleted the add-tag-action-test branch January 28, 2019 21:06
@MarkEWaite MarkEWaite added the skip-changelog Exclude from the changelog label Jul 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Exclude from the changelog
Projects
None yet
2 participants