Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

jenkins.getCurrentUser() can use an incorrect context #632

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

fcojfernandez
Copy link
Member

#631 checks the logged in user by the getJenkins().getCurrentUser(). Some internal testing using SSO, probed that method will fail in such situation. The reason is that the injector is retrieving always the same jenkins instance instead of the actual instance where the Login happened.

@jtnord @amuniz

@EstherAF
Copy link

Really, this is fixing it???? 😆

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

IIUC trying to avoid the fallback in

if (context != null) {
return context.getJenkins();
}
// TODO try to find the real Jenkins root according to the owner of this object, via breadcrumb
// Alternately, Job could have a method to get Jenkins by appending ../../ to its own URL, if not in a folder (need a separate method to find folder owner, but that needs its own page object too)
return injector.getInstance(Jenkins.class);
? Perhaps
public PageObject(Injector injector, URL url) {
should be deprecated; the first random example I looked at
public Build(Job job, int buildNumber) {
super(job.injector, job.url("%d/", buildNumber));
seems to show the same mistake, so there may be many more.

@fcojfernandez
Copy link
Member Author

@jglick yes, that's the issue, while there's only one jenkins instance everything is fine. The problem comes when there are more than one instances implied and you navigate from one to another. Then the fallback retrieves always the same instance: the first one registered.
A quick search using the IDE is showing 20 usages of that getJenkins() method.

@jglick
Copy link
Member

jglick commented Jan 25, 2021

A quick search using the IDE is showing 20 usages of that getJenkins() method.

Which should be checked for correctness; and, as above, uses of the Injector-based constructor of PageObject are likely wrong.

If you do not plan on fixing other occurrences in this PR, could you at least file a Jira issue to track this?

@fcojfernandez
Copy link
Member Author

I'm more than happy to help with this, but I don't think I can do it in the short term, so I've created https://issues.jenkins.io/browse/JENKINS-64708

@jtnord jtnord added the bug label Jan 25, 2021
@jtnord
Copy link
Member

jtnord commented Jan 25, 2021

I am more than happy to merge and release this when the tests finish providing there are no unexpected regressions (which I do not anticipate)

@jtnord jtnord changed the title Make getJenkins retrieve the correct value jenkins.getCurrentUser() can use an incorrect context Jan 25, 2021
@fcojfernandez
Copy link
Member Author

I've analysed the failing tests. Those are the ones failing in the PR that are not failing in master:

TEST
publish_xunit_results – plugins.XUnitPluginTest
parallelTests – plugins.WorkflowPluginTest
generateSimpleXmlTest – plugins.plot.PlotPluginXmlTest
run_on_online_slave_and_master_with_node_restriction – plugins.NodeLabelParameterPluginTest
run_on_several_online_and_offline_slaves – plugins.NodeLabelParameterPluginTest
build_on_a_particular_slave – plugins.NodeLabelParameterPluginTest
run_on_label – plugins.NodeLabelParameterPluginTest
run_on_a_particular_offline_slave_with_ignore – plugins.NodeLabelParameterPluginTest
run_on_several_slaves – plugins.NodeLabelParameterPluginTest
pending_build_with_no_valid_node – plugins.NodeLabelParameterPluginTest
build_with_preselected_node – plugins.NodeLabelParameterPluginTest
run_on_a_particular_offline_slave – plugins.NodeLabelParameterPluginTest
createSshKeys – core.CredentialsTest
global_build_history – core.BuildHistoryTest
generate_simple_plot – plugins.plot.PlotPluginTest
no_exception_visit_plot_page – plugins.plot.PlotPluginTest
postbuild_rendering_should_work – plugins.plot.PlotPluginTest
publish_resources_and_remove_prefix – plugins.FtpPublishPluginTest
publish_multiple_sets – plugins.FtpPublishPluginTest
publish_resources_with_excludes – plugins.FtpPublishPluginTest
publish_with_empty_directory – plugins.FtpPublishPluginTest
publish_jenkins_variables – plugins.FtpPublishPluginTest
publish_resources – plugins.FtpPublishPluginTest
publish_with_no_default_exclude – plugins.FtpPublishPluginTest
publish_slave_resourses – plugins.FtpPublishPluginTest
publish_with_own_pattern – plugins.FtpPublishPluginTest
publish_with_flatten_files – plugins.FtpPublishPluginTest
publish_with_date_format – plugins.FtpPublishPluginTest
publish_with_clean_remote – plugins.FtpPublishPluginTest
publish_with_default_exclude – plugins.FtpPublishPluginTest
publish_without_empty_directory – plugins.FtpPublishPluginTest
publish_with_default_pattern – plugins.FtpPublishPluginTest

All of them seems to be failing because they don't find an element in UI by css or xpath, but they don't seem to be failing because of the login.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants