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.

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-49044] Apply visibility filters to SecurityRealm and AuthorizationStrategy #3246

Merged
merged 2 commits into from Feb 16, 2018

Conversation

@amuniz
Copy link
Member

amuniz commented Jan 19, 2018

See JENKINS-49044.

Proposed changelog entries

  • JENKINS-49044: DescriptorVisibilityFilter extensions are now applied to SecurityRealm and AuthorizationStrategy.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Jan 19, 2018

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Jan 19, 2018

The way I've always explained it to myself is that we only apply descriptor filters for things that cannot represent existing configuration, such as the New Item screen, or the dropdowns for build steps/publishers.

In this case, a misbehaving DVF can actually remove the currently selected option, submission of the form would be pretty bad. I'm not sure this is what we want.

Other opinions?

@amuniz

This comment has been minimized.

Copy link
Member Author

amuniz commented Jan 19, 2018

In this case, a misbehaving DVF can actually remove the currently selected option

That's already possible as /lib/form/hetero-list is filtering descriptors.

submission of the form would be pretty bad

Yes, it will erase the selected value, however it would be a regression in the code adding the DVF.

However, preemptively limiting a core feature because of potential bugs introduced by plugins seems extremely defensive to me.

@jtnord

jtnord approved these changes Jan 29, 2018

@amuniz

This comment has been minimized.

Copy link
Member Author

amuniz commented Feb 12, 2018

@jenkinsci/code-reviewers care to review this please?

@Vlatombe
Copy link
Member

Vlatombe left a comment

lgtm

@jglick

jglick approved these changes Feb 12, 2018

Copy link
Member

jglick left a comment

Test ought to be improved, but content is fine.

@Override
public SecurityComponents createSecurityComponents() { return null; }

@TestExtension

This comment has been minimized.

Copy link
@jglick

jglick Feb 12, 2018

Member

please use

@TestExtension("securityRealmAndAuthStrategyHidden")
}
}

@TestExtension

This comment has been minimized.

Copy link
@jglick

jglick Feb 12, 2018

Member

ditto

@Override
public Collection<String> getGroups() { return null; }

@TestExtension

This comment has been minimized.

Copy link
@jglick

jglick Feb 12, 2018

Member

ditto

}
}

@TestExtension

This comment has been minimized.

Copy link
@jglick

jglick Feb 12, 2018

Member

ditto

j.jenkins.setAuthorizationStrategy(AuthorizationStrategy.UNSECURED);
HtmlPage page = j.createWebClient().goTo("configureSecurity");
String response = page.getWebResponse().getContentAsString();
assertThat(response, not(containsString("TestSecurityRealm")));

This comment has been minimized.

Copy link
@jglick

jglick Feb 12, 2018

Member

More convincing for the DescriptorVisibilityFilter impls (which BTW could be consolidated to just one) to be dynamic, so your test can first visit /configureSecurity, prove that they are displayed as a control, then flip the switch, visit it again, and prove that they are hidden.

Otherwise your test is weak since it would continue to pass if you, say, dropped all the @TestExtension annotations.

@oleg-nenashev oleg-nenashev merged commit 0e51e36 into jenkinsci:master Feb 16, 2018

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.