JENKINS-50122 Authorization should work with basic Workflow jobs

[masterzen]

Not all Jenkins pipeline job have the BranchJob property. In that case
the authorization wouldn't work. This patch makes it work for such job.

[samrocketman]

@jenkinsci/code-reviewers please take a look at this change. @Wadeck thanks for your help in other reviews. I really appreciate your efforts.

[jglick]

So why check types at all?

[masterzen]

I don't know enough about Jenkins internals to be able to answer that question (and I'm not the original author), it's just that restricting to jobs with BranchJobProperty is too restrictive.
Is it possible that not all Job are AbstractProject?

[jglick]

In practice Jobs are

• AbstractProject—freestyle, matrix, Maven, the traditional types
• WorkflowJob
• some obscure other cases, like external monitoring jobs

Generally, you should not downcast without a good reason.

[jglick]

This is the wrong API. Do not depend on workflow-multibranch. scm-api should do what you need.

[masterzen]

@jglick sorry I'm confused.
Is your comment applying to the original code or to my modification of it?

If I understand correctly, you're suggesting to get the SCMSources from the project, is that right?
But it doesn't seem that WorkflowJob is a SCMSourceOwner, which seems to apply only to multi-branch projects (am I correct?)

And thus we could just do something like this:

if (this.item instanceof SCMSourceOwner) {
  project = (SCMSourceOwner)this.item;
  // probably check that there's at least one source here
  scm = (SCMSource) project.getSCMSources().get(0);
} else if (this.item instanceof WorkflowJob) {
  project = (WorkflowJob)this.item; 
  scm = project.getTypicalSCM();
} else if (this.item instanceof AbstractProject) {
  AbstractProject project = (AbstractProject) item;
  scm = project.getScm();
}
...

Would that work and cover all kind of multi-branch projects?

[jglick]

The multibranch folder (MultiBranchProject) containing the branch project (its getParent()) will be an SCMSourceOwner, if you are looking for an SCMSource. You do not need to depend on branch-api either.

If you are looking for an SCM, use SCMTriggerItem.getSCMs. This will work properly for both AbstractProject and WorkflowJob, without a workflow-job dep.

[masterzen]

@jglick from what I understand of the plugin code (I'm not the original author) any type of item can be submitted to this method since it's used to find/check the item ACL based on what ACL exists in github for any repository attached to said item.

So that means not only things that implements SCMTriggerItem (and possibly the MultiBranchProject).

If I followed correctly, we could rewrite this code to be as simple as:

if (this.item instanceof SCMSourceOwner) {
  // covers multi-branch folders and multibranch workflow job
  project = (SCMSourceOwner)this.item;
  // need to check there's at least one 
  scm = (SCMSource) project.getSCMSources().get(0);
} else if (this.item instanceof SCMTriggerItem) {
  // covers all projects that have an SCM (AbstractProject, WorkflowJob etc...)
  project = (SCMTriggerItem)this.item;
  // need to check there's at least one 
  scm = project. getSCMs().get(0);
}

[jglick]

Use SCMTriggerItem.getSCMs rather than an instanceof check, and beware those get(0) calls which suggest edge-case bugs.

getSCMSources returns a list of SCMSources, not SCMs. Depends on what you are actually looking for and why.

[Wadeck]

@samrocketman sorry I am not sufficiantly familiar with the pipeline API to be useful here.

[samrocketman]

No worries @Wadeck thanks for your time.

[sgtcoolguy]

@samrocketman I just upgraded Jenkins from 2.138.1 to 2.138.2 and suddenly any jobs that didn't have a github url associated are giving 404s. I can fix that for one-off jobs to associate them with the most relevant repo, but the big breaking issue here is that GithubOrg Folders suddenly gave 404s to non-admin users. I don't know if this PR is a potential fix for that, but it looks at least related.

I'd love to file a ticket, but https://issues.jenkins-ci.org is timing out for me. I don't know what changed in Jenkins itself to trigger the bad behavior with github auth, but I'd assume the default behavior for a github org folder is to allow view/read on the folder if the user is a member of the associated org.

[samrocketman]

I would have to look. Upgrading Jenkins doesn’t normally affect anything. Upgrading plugins is a different matter. Hard to say without knowing old plugin versions and plugin versions after upgrade. Your best bet is to file a new issue.

Edit: this patch has nothing to do with GitHub org or multi branch pipeline jobs. It is only meant for regular old pipeline jobs.

[pruthvi6767]

@samrocketman Apologies upfront! if this is not the code of conduct for starting a discussion. This is the only way I'm authorized here to start a conversation. I had run in to this problem earlier, the normal pipeline jobs with github commit authorization strategy do not go hand in hand. The githuborgmembershipacl can only fetch permissions to multibranch pipeline jobs based off github org names, repsoitories and branch source properties. I have few questions and appreciate your time if you can answer few of them....

1. Can this #PR be considered valid to allow authorizations for pipeline jobs with neither jobs named as Github Organizations nor include Branch Properties.
2. If not can this be a new feature to consider and contribute to.
3. Is it out of github oauth plugin's scope

[jpigree]

@samrocketman @masterzen @jglick Hi. I just encountered the bug this PR is supposed to fix.

I use this plugin with the "Github commiter authorization strategy" and I have users unable to cancel builds from simple pipelines because those pipelines misses the BranchJobProperty property (confirmed it by running the problematic code in a pipeline).

I also confirm that the fix in this PR works in my case (also confirmed it by running the code in a pipeline).

What needs to be done to finish it? Or as a workaround is there a way to populate the BranchJobProperty in simple pipelines to make the current code work?

Thanks

[masterzen]

@jpigree ,

I was unfortunately unable to do the requested modifications, mostly because I'm really not familiar with Jenkins internals.

But, since we're using pipelines that are not multibranch and have also multiple scms, we're using my fork locally for more than 6 months without issues so far. Unfortunately this code is far from being ready to be submitted as there's no tests...

I'll try to create a PR with those changes if I can find the time in the coming weeks :)

[jpigree]

@masterzen.

Thank you for your quick reply. Yeah. I looked at your fork and it seems you have a few tests already, perhaps not enough.

I am mostly a Python developer but perhaps I can help you with prepping the PR? Sadly, I am not familiar with Jenkins internals too. :(

You need more tests and to take into account the comments they gave you, is that all?

I voted up your issue in their JIRA but we are kinda alone on this. Perhaps people using this feature only uses multibranch pipelines. Or they don't care to have their users not having the CANCEL button.

I wonder what is this "BranchJob" property though. Why standard pipelines do not have it?