Follow-up: Consider if the page is accessible by current user before using simplified display

[indigoxela]

Description of the bug

Issue #6570 added simplified login path displays for three paths, Issue #6799 extended it to allow also the default theme to be used.

What we didn't consider:

If user registration is disabled on a site, this path will lead to a 403, but still with the simplified display (the admin theme) ... and that looks awkward.

Steps To Reproduce

Requires latest dev.

1. Go to admin/config/people/login and verify, the simplified display's turned on, using the admin theme
2. Go to admin/config/people/settings and verify that anonymous may not register accounts
3. In a different browser or a browser window without session go to user/register

Actual behavior

It's a 403, of course, as expected, but wow, that page looks odd.

The same weird display probably happens, when flood prevention strikes. Edit: it doesn't,

Expected behavior

If the site doesn't have user registration enabled, the simplified page display makes no sense for that path. Same if the page isn't accessible for other reasons. Deliver regular 403 pages instead.

Additional information

• Backdrop CMS version: 1.30.x-dev

[indigoxela]

Here's my idea: backdrop/backdrop#4984

@herbdool While this primarily prevents the odd display when user/register's not accessible, this might also fix the problem with TFA you described in our Zulip chat.

Edit: there's yet another time a problem with caching... The menu cache sticks. I have to save the "Account settings" form twice, so the delivery callback gets wiped again.

[indigoxela]

Some more digging and now the static cache no longer gets into the way. Phew.

Ready for testing and review.

[argiepiano]

Thanks, @indigoxela. This works great! A simple fix. WFM. I'll let @herbdool test with TFA and mark as RTBC.

[herbdool]

I love it when I wake up and not only is there an issue, but there's a fix! I'll test with TFA.

[herbdool]

I don't know if I need to fix something in TFA or in core, but now the rest of the TFA process looks worse.

I can't understand why system/tfa/*/* is getting access is FALSE, when it's not on an access denied page. And TFA has an access callback tfa_entry_access which is returning TRUE so it should be okay. I also stepped through the code and it is still able to find system/tfa/%/% even though backdrop_valid_path() gets passed system/tfa/*/*.

[indigoxela]

backdrop_valid_path() gets passed system/tfa//.

Oh, wait... for backdrop_valid_path() we'd actually need system/tfa/%user/% the menu placeholder not the ... "placeholder" 😉 (is that the actual path?).
And we'd need to set the function param for $dynamic_allowed to true (defaults to false), but we can't use that function, anyway.

Seems like we're not really there, yet. 😜

[herbdool]

I can't get it to work even if I try hardcode the path into user_get_user_login_paths() (for testing only):

if ($path == 'system/tfa/*/*') {
        $path = 'system/tfa/%user/%';
      }
      if ($enabled && backdrop_valid_path($path, TRUE)) {
      // if ($enabled) {
        $patterns['login'][] = $path;
      }

I also tried having tfa_entry_access() always return TRUE, and also hardcode 'access' => TRUE, in tfa_menu. To no avail.

[indigoxela]

I woke up with a different approach in mind. 😉

We should do the path check in user_custom_theme() instead. The layout part is OK, anyway, it's just the admin theme getting applied on our 403. PR updated accordingly.

The register path now only gets added in user_user_login_paths() if it's accessible.
This fixes things with "user/register" early.
Additionally there's a check if the path's accessible in user_custom_theme().
This should fix things for flood and tfa - to my understanding. But of course, requires testing it.

The PR still needs work, I guess, but testing the new approach would help to evaluate.

[herbdool]

I'll give it a test in a few hours.

[argiepiano]

Thanks @indigoxela. I've test on a clean install with no other modules, the new approach works fine. I left a comment about a portion of the PR that doesn't seem to be needed for the fix to still work.

[indigoxela]

For people willing to play with it locally - here's a tiny custom module I used to explore some scenarios (like a path has been added, but it's either 404 (no such path), or 403 (not accessible).

foobar.module

<?php

function foobar_user_login_paths() {
  return array(
    'user/foobar' => TRUE,
    'user/foobar/*' => TRUE,
    'doesnotexist' => TRUE,
  );
}

function foobar_menu() {
  $items['user/foobar'] = array(
    'title' => 'Foobar page',
    'page callback' => 'foobar_abc_view',
    'access arguments' => array('administer users'),
  );
  $items['user/foobar/%'] = array(
    'title' => 'Foobar page',
    'page callback' => 'foobar_abc_view',
    'page arguments' => array(2),
    'access callback' => 'foobar_access',
    'access arguments' => array(2),
  );
  return $items;
}

function foobar_abc_view($arg = NULL) {
  if ($arg) {
    return "foobar $arg";
  }
  return 'foobar';
}

function foobar_access($arg) {
  if ($arg == 13) {
    return FALSE;
  }
  return TRUE;
}

[herbdool]

Still not working with TFA. backdrop_valid_path() always returns TRUE with a path like /system/tfa/30/sPBDZ32iZ28gohc31BP1eOWxRPPwlpmN9mcdd8_fh8c. I think it's because tfa_entry_access() only denies access if the TFA session is expired. The other times it hits access denied is during form validation, so not early enough for backdrop_valid_path() I'm guessing.

So that part is an outlier. It's possible that other modules could have the same issue. Though in the case of TFA I could consider just having form errors instead of access denied.

[herbdool]

I'm hesitant to set it to "needs work" for the TFA issue, though there is something else.

I think this part is not needed:

  if (config_get('system.core', 'user_register') != USER_REGISTER_ADMINISTRATORS_ONLY) {
    $paths['user/register'] = TRUE;
  }

when this will already catch user/register in my testing:

    if (!backdrop_valid_path(current_path(), TRUE)) {
      return NULL;
    }

[argiepiano]

I think this part is not needed:

I had pointed that out, please see @indigoxela's answer in the PR.

@indigoxela - will those additional lines carry any performance benefit?

[herbdool]

Okay, I see that now. Let's leave it in then.

[indigoxela]

Still not working with TFA. backdrop_valid_path() always returns TRUE

Hm... then the path is (permission-wise) accessible and core probably has no criterion to evaluate - is that assumption correct?

@herbdool I can see that tfa has an open issue re that topic, but with no description, nor any screenshots. It's hard to understand what we could do to make things easier for contrib. Does the bugfix here even have any impact on tfa pages? That's not really clear to me.

If nothing else works... why not adding some CSS via backdrop_add_css() to the module's pages in question, to style them?

Keep in mind: Site admins still have the choice to either disable simplified display, or to use the default theme (not the admin theme).

Question: With the default theme things are fine - is that correct? If so, you could add a settings recommendation to the module's README.

[indigoxela]

So, the first thing two reviewers found in parallel is, that there's some sort of "double checking". 😄

will those additional lines carry any performance benefit?

@argiepiano that's actually a good question. I'm assuming, the benefit's minimal. However, function backdrop_valid_path() seems a little "expensive", as it does a database query - indirectly via menu_get_item(), and that also handles a hook. For nothing in this case - we know, it'll be a 403 (by config), we don't also have to ask the database... 🤷

If that code's too puzzling - I can remove it. But I'd prefer to let it in. No strong feelings, though.

[herbdool]

@indigoxela this PR doesn't make it worse for TFA, so that's okay. In it's current form the TFA screen looks like what you posted in the issue description. It's the front layout but the Seven theme.

You are correct that in regards to the path, core has no criterion to evaluate. It is a result, I'm guessing, of the particular way that backdrop_access_denied() keeps the path but loading the access denied page.

[argiepiano]

That sounds fine to me, @indigoxela. It does make sense, leave it in!

[argiepiano]

It is a result, I'm guessing, of the particular way that backdrop_access_denied() keeps the path but loading the access denied page.

That was my feeling too. We could dig a bit more into the process of getting that denied page so that it uses the same layout selected for the path of the page that was originally denied. Right now, the returned 403 page uses the default layout, simply ignoring the suppression of the original path.

[herbdool]

@argiepiano or figure out how to have access denied pages always show the active theme? Or does that introduce other problems?

[indigoxela]

Right now, the returned 403 page uses the default layout

Whoah, and that's good! I hope, you're not suggesting to add some frontend-backend-theme-layout mixup by intention? @argiepiano this issue here is mainly about fixing the outcome of such a mix. 😜

...figure out how to have access denied pages always show the active theme?

They do. At least in my testing scenarios. What am I missing? @herbdool I thought, tfa does not 403 these pages... 😕

[argiepiano]

@argiepiano or figure out how to have access denied pages always show the active theme? Or does that introduce other problems?

Yes, that's probably a better option, since anonymous typically doesn't have access to Seven anyway. From the point of view of routes, the 403 page is actually the "correct" response to the original path (e.g. user/register), so that complicates things quite a bit. You'd have to do a theme switch somewhere within backdrop_deliver_html_page().

[argiepiano]

I hope, you're not suggesting to add some frontend-backend-theme-layout mixup by intention?

Nope

[indigoxela]

Maybe it's better to discuss the topics strictly related to TFA in the TFA issue queue. Otherwise this core issue gets a little confusing. 😉

[argiepiano]

The fix right now WFM for the scope described in this issue.

[indigoxela]

Did anyone already test with flood protection? That might make sense.
I only tested the scenarios from my foobar testing module (from previous comment. All the things I had in mind, that might make sense...)

[argiepiano]

I did test flood protection in core and it works fine.

[argiepiano]

You don't get a 403 for flood protection. You get a form error

[herbdool]

@indigoxela by "active theme" I mean the front end theme.

Just so we're on the same page, this is what I see:

when there's a backdrop_access_denied() in a form validation:

function tfa_form_validate($form, &$form_state) {
  // No validation when issuing fallback.
  if (isset($form_state['values']['fallback']) && $form_state['values']['op'] === $form_state['values']['fallback']) {
    return;
  }
  $account = $form['account']['#value'];
  $tfa = tfa_get_process($account);

  if (!$tfa->validateForm($form, $form_state)) {
    foreach ($tfa->getErrorMessages() as $element => $message) {
      form_set_error($element, $message);
    }
    $identifier = config_get('user.flood', 'flood_uid_only') ? $account->uid : $account->uid . '-' . ip_address();
    flood_register_event('tfa_user', config_get('tfa.settings', 'tfa_user_window'), $identifier);

    if (_tfa_hit_flood($tfa)) {
      module_invoke_all('tfa_flood_hit', $tfa->getContext());
      return backdrop_access_denied();
    }
  }
}

So, yes, this is particular to TFA, though it is possible that any module would get this if it inserted a backdrop_access_denied().

At any rate, let's get this PR in.

[indigoxela]

So, yes, this is particular to TFA, though it is possible that any module would get this if it inserted a backdrop_access_denied().

Right. I can reproduce such a scenario with some simple steps. I could imagine, that this makes it hard for some modules to use the simplified display.

Anyway, many thanks for testing and review, to both of you! 🙏

[herbdool]

For TFA, I'm going to just do a backdrop_goto() backdrop-contrib/tfa#16