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.

Sign up for GitHub

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

JENKINS-50122 Authorization should work with basic Workflow jobs #99

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

JENKINS-50122 Authorization should work with basic Workflow jobs #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public ACL getACL(@Nonnull AbstractItem item) {

@Nonnull
public ACL getACL(@Nonnull Job<?,?> job) {
if(job instanceof WorkflowJob && job.getProperty(BranchJobProperty.class) != null || job instanceof AbstractProject) {
if(job instanceof WorkflowJob || job instanceof AbstractProject) {
Copy link
Member

Choose a reason for hiding this comment

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

So why check types at all?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

GithubRequireOrganizationMembershipACL githubACL = (GithubRequireOrganizationMembershipACL) getRootACL();
return githubACL.cloneForProject(job);
} else {
Expand Down
6 changes: 5 additions & 1 deletion src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,11 @@ private String getRepositoryName() {
Describable scm = null;
if (this.item instanceof WorkflowJob) {
WorkflowJob project = (WorkflowJob) item;
scm = project.getProperty(BranchJobProperty.class).getBranch().getScm();
if (project.getProperty(BranchJobProperty.class) != null) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@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);
}

Copy link
Member

Choose a reason for hiding this comment

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

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.

scm = project.getProperty(BranchJobProperty.class).getBranch().getScm();
} else {
scm = project.getTypicalSCM();
}
} else if (this.item instanceof MultiBranchProject) {
MultiBranchProject project = (MultiBranchProject) item;
scm = (SCMSource) project.getSCMSources().get(0);
Expand Down
27 changes: 26 additions & 1 deletion src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ private Project mockProject(String url) {
PowerMockito.when(userRemoteConfig.getUrl()).thenReturn(url);
return project;
}
private WorkflowJob mockWorkflowJob(String url) {
private WorkflowJob mockWorkflowBranchJob(String url) {
WorkflowJob project = PowerMockito.mock(WorkflowJob.class);
GitSCM gitSCM = PowerMockito.mock(GitSCM.class);
Branch branch = PowerMockito.mock(Branch.class);
Expand All @@ -257,6 +257,17 @@ private WorkflowJob mockWorkflowJob(String url) {
PowerMockito.when(userRemoteConfig.getUrl()).thenReturn(url);
return project;
}
private WorkflowJob mockWorkflowJob(String url) {
WorkflowJob project = PowerMockito.mock(WorkflowJob.class);
GitSCM gitSCM = PowerMockito.mock(GitSCM.class);
Branch branch = PowerMockito.mock(Branch.class);
UserRemoteConfig userRemoteConfig = PowerMockito.mock(UserRemoteConfig.class);
List<UserRemoteConfig> userRemoteConfigs = Arrays.asList(userRemoteConfig);
PowerMockito.when(project.getTypicalSCM()).thenReturn(gitSCM);
PowerMockito.when(gitSCM.getUserRemoteConfigs()).thenReturn(userRemoteConfigs);
PowerMockito.when(userRemoteConfig.getUrl()).thenReturn(url);
return project;
}

private MultiBranchProject mockMultiBranchProject(String url) {
WorkflowMultiBranchProject multiBranchProject = PowerMockito.mock(WorkflowMultiBranchProject.class);
Expand All @@ -276,14 +287,18 @@ public void testCanReadAndBuildOneOfMyRepositories() throws IOException {
String repoUrl = "https://github.com/me/a-repo.git";
Project mockProject = mockProject(repoUrl);
MultiBranchProject mockMultiBranchProject = mockMultiBranchProject(repoUrl);
WorkflowJob mockWorkflowBranchJob = mockWorkflowBranchJob(repoUrl);
WorkflowJob mockWorkflowJob = mockWorkflowJob(repoUrl);
GithubRequireOrganizationMembershipACL workflowJobBranchAcl = aclForWorkflowJob(mockWorkflowBranchJob);
GithubRequireOrganizationMembershipACL workflowJobAcl = aclForWorkflowJob(mockWorkflowJob);
GithubRequireOrganizationMembershipACL multiBranchProjectAcl = aclForMultiBranchProject(mockMultiBranchProject);
GithubRequireOrganizationMembershipACL projectAcl = aclForProject(mockProject);
GithubAuthenticationToken authenticationToken = new GithubAuthenticationToken("accessToken", "https://api.github.com");

assertTrue(projectAcl.hasPermission(authenticationToken, Item.READ));
assertTrue(projectAcl.hasPermission(authenticationToken, Item.BUILD));
assertTrue(workflowJobBranchAcl.hasPermission(authenticationToken, Item.READ));
assertTrue(workflowJobBranchAcl.hasPermission(authenticationToken, Item.BUILD));
assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.READ));
assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.BUILD));
assertTrue(multiBranchProjectAcl.hasPermission(authenticationToken, Item.READ));
Expand All @@ -305,7 +320,9 @@ public void testCanReadAndBuildOrgRepositoryICollaborateOn() throws IOException
String repoUrl = "https://github.com/some-org/a-private-repo.git";
Project mockProject = mockProject(repoUrl);
MultiBranchProject mockMultiBranchProject = mockMultiBranchProject(repoUrl);
WorkflowJob mockWorkflowBranchJob = mockWorkflowBranchJob(repoUrl);
WorkflowJob mockWorkflowJob = mockWorkflowJob(repoUrl);
GithubRequireOrganizationMembershipACL workflowJobBranchAcl = aclForWorkflowJob(mockWorkflowBranchJob);
GithubRequireOrganizationMembershipACL workflowJobAcl = aclForWorkflowJob(mockWorkflowJob);
GithubRequireOrganizationMembershipACL multiBranchProjectAcl = aclForMultiBranchProject(mockMultiBranchProject);
GithubRequireOrganizationMembershipACL projectAcl = aclForProject(mockProject);
Expand Down Expand Up @@ -336,7 +353,9 @@ public void testCanReadAndBuildOtherOrgPrivateRepositoryICollaborateOn() throws
String repoUrl = "https://github.com/org-i-dont-belong-to/a-private-repo-i-collaborate-on.git";
Project mockProject = mockProject(repoUrl);
MultiBranchProject mockMultiBranchProject = mockMultiBranchProject(repoUrl);
WorkflowJob mockWorkflowBranchJob = mockWorkflowBranchJob(repoUrl);
WorkflowJob mockWorkflowJob = mockWorkflowJob(repoUrl);
GithubRequireOrganizationMembershipACL workflowJobBranchAcl = aclForWorkflowJob(mockWorkflowBranchJob);
GithubRequireOrganizationMembershipACL workflowJobAcl = aclForWorkflowJob(mockWorkflowJob);
GithubRequireOrganizationMembershipACL multiBranchProjectAcl = aclForMultiBranchProject(mockMultiBranchProject);
GithubRequireOrganizationMembershipACL projectAcl = aclForProject(mockProject);
Expand All @@ -347,6 +366,8 @@ public void testCanReadAndBuildOtherOrgPrivateRepositoryICollaborateOn() throws
assertTrue(projectAcl.hasPermission(authenticationToken, Item.BUILD));
assertTrue(multiBranchProjectAcl.hasPermission(authenticationToken, Item.READ));
assertTrue(multiBranchProjectAcl.hasPermission(authenticationToken, Item.BUILD));
assertTrue(workflowJobBranchAcl.hasPermission(authenticationToken, Item.READ));
assertTrue(workflowJobBranchAcl.hasPermission(authenticationToken, Item.BUILD));
assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.READ));
assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.BUILD));
}
Expand All @@ -359,7 +380,9 @@ public void testCanNotReadOrBuildRepositoryIDoNotCollaborateOn() throws IOExcept
String repoUrl = "https://github.com/some-org/another-private-repo.git";
Project mockProject = mockProject(repoUrl);
MultiBranchProject mockMultiBranchProject = mockMultiBranchProject(repoUrl);
WorkflowJob mockWorkflowBranchJob = mockWorkflowBranchJob(repoUrl);
WorkflowJob mockWorkflowJob = mockWorkflowJob(repoUrl);
GithubRequireOrganizationMembershipACL workflowJobBranchAcl = aclForWorkflowJob(mockWorkflowBranchJob);
GithubRequireOrganizationMembershipACL workflowJobAcl = aclForWorkflowJob(mockWorkflowJob);
GithubRequireOrganizationMembershipACL multiBranchProjectAcl = aclForMultiBranchProject(mockMultiBranchProject);
GithubRequireOrganizationMembershipACL projectAcl = aclForProject(mockProject);
Expand All @@ -370,6 +393,8 @@ public void testCanNotReadOrBuildRepositoryIDoNotCollaborateOn() throws IOExcept
assertFalse(projectAcl.hasPermission(authenticationToken, Item.BUILD));
assertFalse(multiBranchProjectAcl.hasPermission(authenticationToken, Item.READ));
assertFalse(multiBranchProjectAcl.hasPermission(authenticationToken, Item.BUILD));
assertFalse(workflowJobBranchAcl.hasPermission(authenticationToken, Item.READ));
assertFalse(workflowJobBranchAcl.hasPermission(authenticationToken, Item.BUILD));
assertFalse(workflowJobAcl.hasPermission(authenticationToken, Item.READ));
assertFalse(workflowJobAcl.hasPermission(authenticationToken, Item.BUILD));
}
Expand Down