-
Notifications
You must be signed in to change notification settings - Fork 103
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
LiveTest, & fix for CCE in SamlSecurityRealm.doFinishLogin #93
Conversation
pom.xml
Outdated
<java.level>8</java.level> | ||
<jcasc.version>1.35</jcasc.version> | ||
<jenkins-test-harness.version>1492.v843c23c9d568</jenkins-test-harness.version> <!-- TODO https://github.com/jenkinsci/jenkins-test-harness/pull/281 --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using RealJenkinsRule
here because
thread.setContextClassLoader(InitializationService.class.getClassLoader()); |
Line 96 in de609d4
<exclusions> |
JenkinsRule
would simulate plugin operation as accurately as ATH.
The acceptance test started failing as of #90, according to https://ci.jenkins.io/job/Core/job/acceptance-test-harness/job/master/309/testReport/plugins/SAMLPluginTest/java_8_split7___authenticationOK/:
Reproduced that here, and fixed in 2d2cc3b. |
…uecomment-618206131
} | ||
|
||
private static SamlSecurityRealm configureBasicSettings(IdpMetadataConfiguration idpMetadataConfiguration, SamlAdvancedConfiguration advancedConfiguration) throws IOException { | ||
// TODO use @DataBoundSetter wherever possible and load defaults from DescriptorImpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Would likely also be better for CasC: should be able to omit default values of attributes.)
this is a long-delayed task on my queue, I have an environment to make the manual test (https://github.com/kuisathaverat/jenkins-issues) I'll take a look on the weekend to see what is broken on the ATH |
I wanted to try comparing the
and from |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…es Jetty to silently refuse to serve 302s, maybe due to Stapler-Trace headers or something?
saveUser |= modifyUserEmail(user, (List<String>) saml2Profile.getAttribute(getEmailAttributeName())); | ||
Object _emails = saml2Profile.getAttribute(getEmailAttributeName()); | ||
@SuppressWarnings("unchecked") | ||
List<String> emails = _emails instanceof List ? (List<String>) _emails : _emails instanceof String ? Collections.singletonList((String) _emails) : Collections.emptyList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you see it returning a String? IIRC the SAML definition for the emails is a list, so it always returns a list, even though getAttribute can return a String.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it is a list of strings
<saml:Attribute Name="mail" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
<saml:AttributeValue xsi:type="xs:string">test@example.com</saml:AttributeValue>
</saml:Attribute>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know anything about SAML, I just know that saml2Profile.getAttribute(getEmailAttributeName())
returns a String
(an email address), both here and in the acceptance test that started failing as of 2.0.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Minimum jenkins version raised to a 2 days old version in a patch release? |
@viceice the minimum requirement Jenkins core version for the SAMl plugin 2.0.0 is 2.266 that it is a weekly, due there is an LTS version released it is fair to bump the minimum version of the plugin to the new LTS. This PR is included in SAML 2.0.1 For more details about the changes please read the release notes of the plugin and the Jenkins Core https://github.com/jenkinsci/saml-plugin/releases/tag/saml-2.0.0 https://www.jenkins.io/changelog-stable/#v2.277.1 The changes that forced a major release is the change from Spring framework 2.5.x to 5.x |
Ok, thanks for clarify. Sorry for the noise. |
Trying to provide meaningful test coverage inside this plugin, without relying on ATH. Otherwise tests pass even when the plugin is broken:
Also fixes a regression: #93 (comment)