The "Allow SAML users to login directly with Drupal" is not working as expected. When the checkbox is not flagged, SAML users can still login via Drupal; when it's flagged, they can't.

In fact the check on the authmap table happens only when the corresponding variable is TRUE:

function samlauth_check_saml_user($form, $form_state) {
  if (variable_get('samlauth_drupal_saml_login', FALSE)) {
    if (form_get_errors()) {

While on branch 8.x-3.x the same variable has the opposite meaning:

function samlauth_check_saml_user($form, FormStateInterface $form_state) {
  if (!\Drupal::config('samlauth.authentication')->get('drupal_saml_login')) {
    if ($form_state->hasAnyErrors()) {

Possible solutions:

  1. Fix the current logic and keep existing values as is, breaking the backward compatibility
  2. Fix the current logic and run an update on any existing variable
  3. Update the variable description or use a different variable
CommentFileSizeAuthor
#3 samlauth-fix_check_drupal_login-3113451-3.patch1.01 KBnironan
#2 samlauth-fix_check_drupal_login-3113451-2.patch658 bytesnironan

Comments

nironan created an issue. See original summary.

nironan’s picture

nironan commented
Status: Active » Needs review
StatusFileSize
new samlauth-fix_check_drupal_login-3113451-2.patch658 bytes

First attempt, following option #1

nironan’s picture

nironan commented
StatusFileSize
new samlauth-fix_check_drupal_login-3113451-3.patch1.01 KB

Found another bug on the same check.
At line 126, there's a call to function samlauth_find_unique_id_by_user_id($uid), which runs a db_select using the uid account. The result is checked at 127 via is_null(), which is always TRUE since the result of fetchField() is either the samlauth username or FALSE (hence: never NULL). This results in local non-SAML accounts being denied the login.

Attached is patch which checks that sets the form to error only when the fetchField() result is not false .

  • roderik committed 5edf0da on 7.x-1.x authored by nironan 
    Issue #3113451 by nironan: "Allow SAML users to login directly with...
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Fixed

Thank you; committed. (I also changed the checkbox description to be equal to what it was in D8, even though it's now even longer. Mainly I think the "Drupal login is hidden" bit was confusing, since that technically has no effect.)

I agree we should just change the behavior (option 1). We can't know with 100% certainty, but it's likely that what people want their site to do, is what the checkbox setting actually says.

Status: Fixed » Closed (fixed)

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