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

To support multiple authnContext #183

Closed
wants to merge 17 commits into from
Closed

To support multiple authnContext #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e7b86b0
to support multiple authnContext
Jan 2, 2017
03b07cb
to support multiple authnContext: modified array looping code as per …
Jan 2, 2017
8ce3706
to support multiple authnContext: modified based on the unit test case
Jan 3, 2017
ce93019
to support multiple authnContext: test results are modified based on …
Jan 3, 2017
63ff3b7
to support multiple authnContext: test results are modified based on …
Jan 3, 2017
67dd725
to support multiple authnContext: test results are modified based on …
Jan 3, 2017
dc0a7bc
to support multiple authnContext: test results are modified based on …
Jan 3, 2017
182f774
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
bb6af8d
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
7378125
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
3ec6227
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
39f13b4
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
fb82ad8
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
c035922
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
13110f3
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
c25e7f2
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
06a2257
to support multiple authnContext: test case syntax modified for statb…
Jan 3, 2017
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
15 changes: 10 additions & 5 deletions lib/passport-saml/saml.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,14 +180,19 @@ SAML.prototype.generateAuthorizeRequest = function (req, isPassive, callback) {
};
}

if (!self.options.disableRequestedAuthnContext) {
if (!self.options.disableRequestedAuthnContext) {
var authnContextInArray = [];
self.options.authnContext.split(',').forEach(function(value, index) {
authnContextInArray.push({
'@xmlns:saml': 'urn:oasis:names:tc:SAML:2.0:assertion',
'#text': self.options.authnContext.split(',')[index]
Copy link
Contributor

Choose a reason for hiding this comment

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

What about just using value?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cadesalaberry Could you say more about what you see that could be improved here?

Copy link

Choose a reason for hiding this comment

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

I believe he is wondering why not use value instead of self.options.authnContext.split(',')[index]

+      self.options.authnContext.split(',').forEach(function(value, index) {
+        authnContextInArray.push({
+            '@xmlns:saml': 'urn:oasis:names:tc:SAML:2.0:assertion',
+            '#text': value

Copy link

@illion20 illion20 Apr 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I vote for making it an array even if its one value, but I have no status here:

+      self.options.authnContext.forEach( context => {
+        authnContextInArray.push({
+            '@xmlns:saml': 'urn:oasis:names:tc:SAML:2.0:assertion',
+            '#text': context

Copy link
Collaborator

Choose a reason for hiding this comment

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

My comment earlier favored detecting if the incoming value was an array or a single value and switching accordingly. That prevents breakage.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cjbarth Couldn't either be valid according to the SAML spec? Is the breakage you referring backwards compatibility in the sense that we would continue to handle single values as before?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes @markstos , that is the breakage I'm referring to. We should break backwards compatibility of passing in a single value if we don't have to, and in this case I don't think we have to.

Copy link

Choose a reason for hiding this comment

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

I'm with @cjbarth on using an array instead of comma-separated.

For #252 we could extend an array implementation with detecting string vs objects (e.g.: [{ref: 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport', comparisonType: 'minimum'}]). That would be a pain with a comma-separated value.

});
});

request['samlp:AuthnRequest']['samlp:RequestedAuthnContext'] = {
'@xmlns:samlp': 'urn:oasis:names:tc:SAML:2.0:protocol',
'@Comparison': 'exact',
'saml:AuthnContextClassRef': {
'@xmlns:saml': 'urn:oasis:names:tc:SAML:2.0:assertion',
'#text': self.options.authnContext
}
'saml:AuthnContextClassRef': authnContextInArray
};
}

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"xml2js": "0.4.x",
"xml-crypto": "0.8.x",
"xmldom": "0.1.x",
"xmlbuilder": "2.5.x",
"xmlbuilder": "4.1.x",
"xml-encryption": "~0.7"
},
"devDependencies": {
Expand Down
2 changes: 1 addition & 1 deletion test/samlTests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ describe('SAML.js', function() {
// NOTE: This test only tests existence of the assertion, not the correctness
it('calls callback with saml request object', function(done) {
saml.getAuthorizeUrl(req, function(err, target) {
url.parse(target, true).query.should.have.property('SAMLRequest');
should(url.parse(target, true).query).have.property('SAMLRequest');
done();
});
});
Expand Down
12 changes: 9 additions & 3 deletions test/static/expected metadata.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,15 @@ nwtlCg==
</ds:X509Certificate>
</ds:X509Data>
</ds:KeyInfo>
<EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes256-cbc"/>
<EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes128-cbc"/>
<EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#tripledes-cbc"/>
<#list>
<EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes256-cbc"/>
</#list>
<#list>
<EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#aes128-cbc"/>
</#list>
<#list>
<EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#tripledes-cbc"/>
</#list>
</KeyDescriptor>
<NameIDFormat>urn:oasis:names:tc:SAML:2.0:nameid-format:transient</NameIDFormat>
<AssertionConsumerService index="1" isDefault="true" Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="http://example.serviceprovider.com/saml/callback"/>
Expand Down
3 changes: 2 additions & 1 deletion test/tests.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.