NameID in AuthnRequest

[max-m-s]

Hi guys, I spotted Net::SAML2::Protocol::AuthnRequest has a name_id parameter, but it doesn't result in what I would expect.

If populated it would add a NameID tag with NameQualifier="value", rather than providing the value for NameID itself.

I had to make my own version of AuthnRequest and change the following line:

    #return $x->Subject($saml, $x->NameID($saml, {NameQualifier => $self->nameid}));
    return $x->Subject($saml, $x->NameID($saml, {}, $self->nameid));

Would it be possible to add a parameter that sets the NameID value (maybe the Format attribute too)? Though I guess it can no longer be called "name_id" since it seems NameQualifier could be used to hint to the IdP of the domain the user might reside in.

The above change was proven to work with the "saml_subject" attribute on PingOne.

[timlegge]

@waterkip

I am thinking something like:

diff --git a/lib/Net/SAML2/Protocol/AuthnRequest.pm b/lib/Net/SAML2/Protocol/AuthnRequest.pm
index f2a3fb0..1b64419 100644
--- a/lib/Net/SAML2/Protocol/AuthnRequest.pm
+++ b/lib/Net/SAML2/Protocol/AuthnRequest.pm
@@ -93,6 +93,12 @@ has 'nameid' => (
     predicate => 'has_nameid'
 );
 
+has 'nameid_format' => (
+    isa       => 'Str',
+    is        => 'rw',
+    predicate => 'has_nameid_format'
+);
+
 has 'nameidpolicy_format' => (
     isa       => 'Str',
     is        => 'rw',
@@ -278,7 +284,7 @@ sub _set_name_id {
     my $self = shift;
     return unless $self->has_nameid;
     my $x = shift;
-    return $x->Subject($saml, $x->NameID($saml, {NameQualifier => $self->nameid}));
+    return $x->Subject($saml, $x->NameID($saml, {NameQualifier => $self->issuer_namequalifier, Format => $self->nameid_format}, $self->nameid));
 }
 
 sub _set_name_policy_format {
diff --git a/t/09-authn-request.t b/t/09-authn-request.t
index e7301e2..117d390 100644
--- a/t/09-authn-request.t
+++ b/t/09-authn-request.t
@@ -41,7 +41,14 @@ $override->override('Net::SAML2::Protocol::AuthnRequest::_build_id' =>
 
     test_xml_attribute_ok($xp,
         '/samlp:AuthnRequest/saml:Subject/saml:NameID/@NameQualifier',
-        'mynameid');
+        'bar');
+
+    test_xml_attribute_ok($xp,
+        '/samlp:AuthnRequest/saml:Subject/saml:NameID/@Format',
+        'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent');
+
+    $node = get_single_node_ok($xp, '/samlp:AuthnRequest/saml:Subject/saml:NameID');
+    is($node->textContent, 'mynameid', '... and has the correct value');
 
     %attributes = (
         Format      => 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent',

[max-m-s]

Oh I see now, it should have been "issuer_namequalifier". Is it logical to send just a NameQualifier without NameID? If it is then maybe the return statement needs to allow for either, like:
return unless $self->has_nameid || $self->has_issuer_namequalifier;

[waterkip]

Oh I see now, it should have been "issuer_namequalifier". Is it logical to send just a NameQualifier without NameID? If it is then maybe the return statement needs to allow for either, like: return unless $self->has_nameid || $self->has_issuer_namequalifier;

That would make sense. We could also tweak the builder to croak once you set the NameQualifier and not the NameID. However, that might not be possible as the attributes are 'rw'...

@timlegge be aware that our BUILDARGS are doing some magic with nameid_format.

It was introduced with 56051db

    4e3ff81d4f2b963a855a5c24c96c87e2d4577c5b - this commit wipes the
    NameIDPolicy in the XML from Net::SAML2::Protocol::AuthnRequest
    (nameid_format attribute is now simply ignored). This is a breaking
    change for consumers of .17

    7f459c303c250c532d5b47f8e137bdd88c4fc2a7 - this commit reintroduces it,
    but under a different name: nameidpolicy_format. Now older code will
    need to deal with this change. I therefore introduced an `around
    BUILDARGS` which changes `nameid_format` to `nameidpolicy_format`.
    Although I think we should pick *one* and stick to it. Other code uses
    `nameid_format`: Net::SAML2::Protocol::LogoutRequest, Net::SAML2::SP and
    Net::SAML2.

That might need further inspection to see how we want to deal with it.

[waterkip]

I've not forgotten this issue. I've made a first step with #200 to add a deprecation warning to the builder so we have wiggle room to fix the issue correctly.