Make GitHubAppCredentials#writeReplace protected for subclasses

[Dohbedoh]

Description

This would allow class that extends GitHubAppCredentials to benefit from the XStream serialization implemented in GitHubAppCredentials

Submitter checklist

• Link to JIRA ticket in description, if appropriate.
• Change is code complete and matches issue description
• Automated tests have been added to exercise the changes
• Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

• Run the changes and verify that the change matches the issue description
• Reviewed the code
• Verified that the appropriate tests have been written or valid explanation given

Documentation changes

• Link to jenkins.io PR, or an explanation for why no doc changes are needed

Users/aliases to notify

@jeromepochat @jenkinsci/github-branch-source-plugin-developers

[jeromepochat]

I tested using GitHubAppCredentials subclass and confirm it benefits from writeReplace implementation.

[jtnord]

TBH this all seems a bit messy to me.
Why would this just not be in a SnapshotTaker for GitHubAppCredentials?
The refresh can make a call to the controller if on an agent or make a local call if it is on the controller... (then it is just the DelegatingGitHubAppCredentials created by the SnapShotTaker that needs to have a readResolve to set a channel (but only if used on an agent, and then making a local call if on a controller).

[Dohbedoh]

hmm I see what you mean. When playing around though, you quickly notice that the DelegatingGitHubAppCredentials does not extend GitHubAppCredentials and that's maybe why things started to get messy. It does not seem like you can snapshot a GitHubAppCredentials to a DelegatingGitHubAppCredentials with the current design ? Maybe it requires some superclass that hold the "source" for refreshing the token. One source implementation would have appId, privateKey, apiUri and owner and the other the encrypted form of those (what DelegatingGitHubAppCredentials has).

FYI, my proposed change is to fix the serialization in an upstream implementation. Without changing anything upstream. Maybe we can address the snapshotting problem as part of a different PR. I can create a JENKINS issue for this.

[Dohbedoh]

https://issues.jenkins.io/browse/JENKINS-73787

[jtnord]

DelegatingGitHubAppCredentials does not extend GitHubAppCredentials and that's maybe why things started to get messy. It does not seem like you can snapshot a GitHubAppCredentials to a DelegatingGitHubAppCredentials with the current design ?

snapshot should return (an implementation of) the interface type (because credential types should be written using interfaces as per the design doc) which in this case would be StandardUsernamePasswordCredentials so AFAICT without writing code this should be possible.
if code really does need a specific type for a GitHubApp credential (which is likely) then the GitHubAppCredential should not be a concrete type but an interface with an implementaion. (doc)

[jtnord]

(for the avoidance of doubt I am not blocking this PR, but I do not feel like the addition of debt onto the debt makes me want to approve it either)

[jglick]

if code really does need a specific type for a GitHubApp credential (which is likely)

No, it is just used as an implementation of StandardUsernamePasswordCredentials that happens to create a “password” on demand.

[dwnusbaum]

TL;DR: Unless I am missing something obvious, this PR seems like the simplest way to fix the issue without causing more problems.

The motivation behind this change is to fix the behavior of a CloudBees-specific subclass of GitHubAppCredentials that comes from a plugin that implements a special CredentialsProvider that stores the relevant secrets externally (e.g. the private key). Right now, those credentials may not be used on agents directly.

The main problem is that we need multiple levels of dynamic behavior for GitHub app credentials:

• We need to support dynamic contextualization of owner via Credentials.forRun and Connector.lookupScanCredentials
  • This requires us to use an instance of GitHubAppCredentials right up until the actual password is going to be used
  • For CloudBees, this means that any CredentialsSnapshotTaker for a GitHubAppCredentials must translate to a type that is still a GitHubAppCredentials (which is not the case for DelegatingGitHubAppCredentials), otherwise owner inference totally breaks when credentials are sent from CloudBees CI Operations Center to controllers (this kind of problem was the motivation behind [JENKINS-73388] Allow alternative implementation for GitHub App credentials #796)
• We want to support dynamic refresh of the token from agents without the agents having access to the GitHub app secrets directly
  • This requires us to replace GitHubAppCredentials with a type like DelegatingGitHubAppCredentials when serializing the credentials to an agent

Making writeReplace protected here allows the CloudBees-specific type to use a CredentialsSnapshotTaker to produce an instance of GitHubAppCredentials where the external secrets have been retrieved, which does not break owner inference for the snapshot, and does not prevent DelegatingGitHubAppCredentials from being used for the credentials and the snapshot on agents.

If we try to use CredentialsSnapshotTaker here and delete writeReplace, things get more complicated. We need a way to tell in our CredentialsSnapshotTaker implementation whether the snapshot is happening because the credentials are being sent to an agent (the only case for OSS Jenkins) or is because the credentials are being sent from OC to controllers (only for CloudBees). This is not just a problem for the special GitHubAppCredentials subtype with CloudBees CI, it also applies to usage of plain GitHubAppCredentials in CloudBees CI (e.g. #616 is somewhat related to this kind of problem). The problem though is that Channel.current() returns null from inside of CredentialsSnapshotTaker.snapshot (at least when checking testAgentRefresh here), so I don't really see any simple way to make this work without writeReplace.

Now, are there bigger changes that could be made to avoid this issue? Maybe. For example, if DelegatingAppCredentials was itself a subclass of GitHubAppCredentials and supported owner inference, then we could always translate directly to it in CredentialsSnapshotTaker. I am not sure if that can be done safely though given the need for agents to be able to refresh the token dynamically. Additionally, the CloudBees-specific subtype of GitHubAppCredentials could maybe be deleted with a redesign of the relevant plugin, but we'd still have CloudBees-specific issues with usage of regular GitHubAppCredentials if we tried to unify GitHubAppCredentialsSnapshotTaker and GitHubAppCredentials.writeReplace.

[dwnusbaum]

I will leave this open for a day or so in case there are any objections or things that I missed in #810 (comment), but otherwise I plan to merge and release this.