Hi all here!

In my project I use SSO service which doesn't provide NameId as expected in module. I have an error:

OneLogin\Saml2\ValidationError encountered while processing SAML authentication response: NameID not found in the assertion of the Response in OneLogin\Saml2\Response->getNameIdData() (line 618 of /var/www/html/vendor/onelogin/php-saml/src/Saml2/Response.php).

But when I add wantNameId => FALSE in security section of configuration (SamlService :: reformatConfig()) login works fine.

CommentFileSizeAuthor
#2 samlauth-want_name_id-3086827.patch2.73 KBantonnavi

Comments

Antonnavi created an issue. See original summary.

antonnavi’s picture

antonnavi
Primary language Ukrainian
Location Leipzig
commented
StatusFileSize
new samlauth-want_name_id-3086827.patch2.73 KB

Attached patch solves this issue.

antonnavi’s picture

antonnavi
Primary language Ukrainian
Location Leipzig
commented
Status: Active » Needs review
davidferlay’s picture

davidferlay commented
Status: Needs review » Reviewed & tested by the community

Tested OK !

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 8.x-3.0-alpha1 » 8.x-3.x-dev
Issue tags: +Needs tests 
+++ b/src/Form/SamlauthConfigureForm.php
@@ -403,6 +403,12 @@ class SamlauthConfigureForm extends ConfigFormBase {
+      '#title' => $this->t('NameId is required'),

Would be great to add better explanation and reference to docs in description or help

  • roderik committed 7612e8e on 8.x-3.x 
    Issue #3086827: fix for configuration screen on existing installs, and...
  • roderik committed 7eab565 on 8.x-3.x authored by Antonnavi
    Issue #3086827 by Antonnavi, davidferlay, andypost, roderik: Add UI to...
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Reviewed & tested by the community » Fixed

Thank you for this addition. Additions to the configuration screen are accepted almost by default. I won't require tests yet, as mentioned in #2882568: Plan for SAML Authentication 4.x.

Two things:

  • Agreed with @andypost that this needs a better description. I just added this myself, because I still get ideas about fixing descriptions every time I look at the configuration screen. (Like: I'll change the existing "Request messages to be signed" to "Require messages to be signed", along with this, for unification / because it's better.)
  • I believe I caught a 'bug' in the configuration screen: the default checkbox state must be 'Checked', otherwise existing installations which show the screen and save it, will have the value saved as FALSE - which will introduce a degradation in security. (......aaaaand I was thinking that SamlService::reformatConfig() doesn't need it, but it actually does. I'll push another commit.)

  • roderik committed 42539c3 on 8.x-3.x 
    Issue #3086827: fix for default configuration.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.