Check Workspace permission before showing source code viewer

[akash-manna-sky]

Add permissions for source code viewer

Fixes #2940

Testing done

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests that demonstrate the feature works or the issue is fixed

[KalleOlaviNiemitalo]

"Add permissions for source code viewer" sounds as if this defined a new permission type. If this instead uses Job/Workspace permission, then please say so in the title, so that it will be clear from the release notes.

[akash-manna-sky]

Please review the changes. @uhafner

[uhafner]

I think we should not reuse the Workspace permissions. See https://issues.jenkins.io/browse/JENKINS-48354 for details.
When we check for permissions, we should create a new permission in the prism plugin (it is responsible for showing source code). But I think the new permission should be an opt-out, so we can see the source code by default and only when an admin decides then the source code is hidden.

[akash-manna-sky]

I think we should not reuse the Workspace permissions. See https://issues.jenkins.io/browse/JENKINS-48354 for details. When we check for permissions, we should create a new permission in the prism plugin (it is responsible for showing source code). But I think the new permission should be an opt-out, so we can see the source code by default and only when an admin decides then the source code is hidden.

Then, what should I do now? Should I open a new PR in https://github.com/jenkinsci/prism-api-plugin based on your suggestion or anything else?

[uhafner]

If it is possible to create permissions as opt-out, then it makes sense to create the permission in the prism plugin (and move the view to this plugin). Can you check, if that is possible?

[akash-manna-sky]

If it is possible to create permissions as opt-out, then it makes sense to create the permission in the prism plugin (and move the view to this plugin). Can you check, if that is possible?

Ok, I will investigate and inform you soon.

[akash-manna-sky]

I implemented the functionality by calling create from prism-api-plugin, but there are two test failure DetailFactoryTest.shouldReturnSourceDetailWhenCalledWithSourceLinkAndIssueNotInConsoleLog and DetailFactoryTest.shouldShowExceptionMessageIfAffectedFileIsNotReadable because these tests expect a running Jenkins instance, but none exists in the test context. Should I move these two test in separate ITest file? Like:

package io.jenkins.plugins.analysis.core.model;

import org.junit.jupiter.api.Test;

import edu.hm.hafner.analysis.IssueBuilder;
import edu.hm.hafner.analysis.Report;

import java.io.IOException;
import java.io.StringReader;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;

import hudson.model.Run;

import io.jenkins.plugins.analysis.core.testutil.IntegrationTestWithJenkinsPerSuite;
import io.jenkins.plugins.analysis.core.util.BuildFolderFacade;
import io.jenkins.plugins.prism.SourceCodeViewModel;
import io.jenkins.plugins.util.JenkinsFacade;

import static io.jenkins.plugins.analysis.core.testutil.Assertions.*;
import static org.mockito.ArgumentMatchers.*;
import static org.mockito.Mockito.*;

/**
 * Integration tests for {@link DetailFactory} source file reading functionality.
 * These tests require a Jenkins instance because SourceCodeViewModel.create() checks permissions.
 *
 * @author Akash Manna
 */
class DetailFactorySourceFileITest extends IntegrationTestWithJenkinsPerSuite {
    private static final Charset ENCODING = StandardCharsets.UTF_8;
    private static final String AFFECTED_FILE_CONTENT = "public class Test { }";
    private static final Report NEW_ISSUES = new Report();
    private static final Report OUTSTANDING_ISSUES = new Report();
    private static final Report FIXED_ISSUES = new Report();

    /**
     * Checks that the error message is shown if an affected file could not be read.
     */
    @Test
    void shouldShowExceptionMessageIfAffectedFileIsNotReadable() throws IOException {
        JenkinsFacade jenkins = mock(JenkinsFacade.class);
        BuildFolderFacade buildFolder = mock(BuildFolderFacade.class);
        when(buildFolder.readFile(any(), anyString(), any())).thenThrow(new IOException("file error"));

        var details = createDetails(jenkins, buildFolder, "a-file");

        assertThat(details).isInstanceOfSatisfying(SourceCodeViewModel.class,
                s -> assertThat(s.getSourceCode()).contains("IOException: file error"));
    }

    /**
     * Checks that a link to a source returns a SourceDetail-View.
     */
    @Test
    void shouldReturnSourceDetailWhenCalledWithSourceLinkAndIssueNotInConsoleLog() throws IOException {
        JenkinsFacade jenkins = mock(JenkinsFacade.class);
        BuildFolderFacade buildFolder = mock(BuildFolderFacade.class);
        when(buildFolder.readFile(any(), anyString(), any())).thenReturn(new StringReader(AFFECTED_FILE_CONTENT));

        var details = createDetails(jenkins, buildFolder, "a-file");

        assertThat(details).isInstanceOfSatisfying(SourceCodeViewModel.class,
                s -> assertThat(s.getSourceCode()).contains(AFFECTED_FILE_CONTENT));
    }

    private Object createDetails(final JenkinsFacade jenkins, final BuildFolderFacade buildFolder,
            final String fileName) {
        try (var issueBuilder = new IssueBuilder()) {
            var detailFactory = new DetailFactory(jenkins, buildFolder);

            issueBuilder.setFileName(fileName);
            var issue = issueBuilder.build();

            var report = new Report();
            report.add(issue);

            var project = createFreeStyleProject();
            buildSuccessfully(project);
            Run<?, ?> run = project.getLastBuild();

            return detailFactory.createTrendDetails("source." + issue.getId().toString(),
                    run, createAnalysisResult(), report, NEW_ISSUES, OUTSTANDING_ISSUES, FIXED_ISSUES, ENCODING,
                    createParent());
        }
    }

    private AnalysisResult createAnalysisResult() {
        AnalysisResult result = mock(AnalysisResult.class);
        when(result.getErrorMessages()).thenReturn(org.eclipse.collections.impl.factory.Lists.immutable.empty());
        when(result.getInfoMessages()).thenReturn(org.eclipse.collections.impl.factory.Lists.immutable.empty());
        return result;
    }

    private IssuesDetail createParent() {
        IssuesDetail parent = mock(IssuesDetail.class);
        StaticAnalysisLabelProvider labelProvider = mock(StaticAnalysisLabelProvider.class);
        when(labelProvider.getName()).thenReturn("Test");
        when(labelProvider.getSmallIconUrl()).thenReturn("/icon");
        when(labelProvider.getLargeIconUrl()).thenReturn("/icon");
        when(parent.getLabelProvider()).thenReturn(labelProvider);
        return parent;
    }
}

Should I add new test for new functionality? (as it was already tested in prism-api-plugin)

@uhafner

[uhafner]

Thanks, this looks good now. Just the facade can be replaced with the real one...

[uhafner]

Suggested change

	private Object createDetails(final JenkinsFacade jenkins, final BuildFolderFacade buildFolder,
	private Object createDetails(final BuildFolderFacade buildFolder,

[uhafner]

I think you can use new JenkinsFacade() since Jenkins is now running.

[uhafner]

Suggested change

	when(result.getErrorMessages()).thenReturn(org.eclipse.collections.impl.factory.Lists.immutable.empty());
	when(result.getInfoMessages()).thenReturn(org.eclipse.collections.impl.factory.Lists.immutable.empty());
	when(result.getErrorMessages()).thenReturn(Lists.immutable.empty());
	when(result.getInfoMessages()).thenReturn(Lists.immutable.empty());

[akash-manna-sky]

Thanks!