JENKINS-70275 git scm owner is reinitialized if null whenever accessi…

[mhenschke-atl]

This issue can be replicated locally by taking the following steps:

• Create a multibranch pipeline, using folder-level credentials that will be used for checkout
• Restart Jenkins
• Trigger a job using a webhook
  The result is a stack trace in the Jenkins logs related to a 401 on checkout. Inspecting the request in Bitbucket reveals Jenkins has sent a pair of garbled strings instead of real credentials, which probably isn't great but is ultimately a red herring.

This issue relates closely to JENKINS-65541, and has to do with the way we wrap the GitSCMSource. We provide a credential id to the git scm when attempting a retrieve rather than a full credential- it then uses it's owner context (which refers to the containing job or folder) so it can only retrieve credentials it has access to. All items have global access. However, our GitSCMSource doesn't have an owner, as it's wrapped in the BitbucketSCMSource, so the solution to this bug was to ensure we added the same owner when the GitSCMSource is initialized.

The problem comes when Jenkins restarts. We added the setOwner check in the git initialization code, that's called whenever a BitbucketSCMSource is created, but upon restart Jenkins doesn't create a new SCMSource, it deserializes it from the job's config.xml file. An example is shown below:

<?xml version='1.1' encoding='UTF-8'?>
<org.jenkinsci.plugins.workflow.multibranch.WorkflowMultiBranchProject plugin="workflow-multibranch@716.vc692a_e52371b_">
...
        <source class="com.atlassian.bitbucket.jenkins.internal.scm.BitbucketSCMSource" plugin="atlassian-bitbucket-server-integration@3.3.2">
          <id>0f0efe43-ae80-4610-913f-f2ef88d2de24</id>
          <traits>
            <jenkins.plugins.git.traits.BranchDiscoveryTrait plugin="git@5.0.0"/>
          </traits>
          <gitSCMSource>
            <id>0f0efe43-ae80-4610-913f-f2ef88d2de24-git-scm</id>
            <remote>http://localhost:7990/bitbucket/scm/project_1/jenkins.git</remote>
            <credentialsId>a61cbccc-fd6b-480d-98dc-0ff551e2bd1a</credentialsId>
            <traits>
              <jenkins.plugins.git.traits.BranchDiscoveryTrait reference="../../../traits/jenkins.plugins.git.traits.BranchDiscoveryTrait"/>
            </traits>
            <repository>
              <credentialsId>a61cbccc-fd6b-480d-98dc-0ff551e2bd1a</credentialsId>
              <mirrorName></mirrorName>
              <projectKey>PROJECT_1</projectKey>
              <projectName>Project 1</projectName>
              <repositoryName>jenkins</repositoryName>
              <repositorySlug>jenkins</repositorySlug>
              <serverId>26440aa8-7e48-4ca1-afbc-1c506c10a162</serverId>
              <sshCredentialsId></sshCredentialsId>
            </repository>
          </gitSCMSource>
          <repository reference="../gitSCMSource/repository"/>
          <webhookRegistered>false</webhookRegistered>
        </source>
...

Notice that the GitSCMSource serializes the credential id fine, but not the owner- which is why the credential is passed through with no problems to the SCM which attempts to fetch the credential and fails. Hence, the authentication check with Bitbucket fails and the job cannot succeed until the user has re-saved the config, this creating the SCMSource anew with the appropriate owner.

This bug will reoccur every time Jenkins restarts.

You may be asking is "why doesn't this happen with the BitbucketSCMSource as well"? Unlike the GitSCMSource, the Bitbucket source has a real owner- a multibranch pipeline. These extend Item, which exposes an onLoad method- a method called as part of deserialization of the config file, where any implied information (such as the owner) can be added dynamically. You can see the workings in MultiBranchProject#init2- it cycles over all deserialized SCMSources it knows about and simply adds itself to them as owner manually. Our GitSCMSource is not in this list, so it is omitted and we have to do it ourself. It would be really convenient if SCMSource exposed an onLoad method, but it doesn't because this sort of dynamic setting is designed to be done by Projects, not SCMs- which is one more compelling reason to do away with the GitSCMSource in the future.

[dkjellin]

Thank you for digging into and fixing this @mhenschke-atl !

[khughes-atlassian]

Does this only have to happen after the scm source is initialized or are there other scenarios where we might need to set the owner? If the former, should we put this code as part of initializeGitScmSource?

[mhenschke-atl]

The GitSCMSource always needs an owner, so it does need to be in the initialization too. It's done as part of constructing the new source:

        gitSCMSource = new CustomGitSCMSource(remoteConfig.getUrl(), repository);
        gitSCMSource.setBrowser(new BitbucketServer(selfLink));
        gitSCMSource.setCredentialsId(repository.getCloneCredentialsId());
        gitSCMSource.setOwner(getOwner());
        gitSCMSource.setTraits(traits);
        gitSCMSource.setId(getId() + "-git-scm");

One thing to note here is we also set a browser which is not serialized with the rest of the source. I'm actually not 100% sure of what that browser is doing- theoretically it generates links out to source browsers but we already do that manually with BitbucketLink. That said I'm happy to include a null check for it as well, for completeness.

[dkjellin]

The browser provides the links for when you view the changeset in Jenkins. Would be good to include it as well. Either in this PR or in a separate PR

[mhenschke-atl]

The browser is a slightly trickier task, as it relies on the self-link which we don't currently persist anywhere. I might tackle this in a separate PR, it's a larger change and unrelated to this issue.

[dkjellin]

I am slightly concerned that we're removing the browser but... I think we can address that in a separate PR before release.

[mhenschke-atl]

I had a go at renaming this method to make it more reflective of what it's actually doing

[mhenschke-atl]

I'm wondering if the recent JENKINS-60490 reopening is related to this issue too- will perform a local test on it before this merges