Problem/Motivation

I would like a way to disable the showcore parameter, so that users cannot use it to get to the native Drupal login page.

Steps to reproduce

  1. Configure site to use the "replace" login form method.
  2. Go to /user/login?showcore=1

Proposed resolution

Add a setting to disable the workaround.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork openid_connect-3441149

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

solideogloria created an issue. See original summary.

solideogloria’s picture

solideogloria commented
Issue summary: View changes

alt36 made their first commit to this issue’s fork.

alt36 opened merge request !108

solideogloria’s picture

solideogloria commented
Status: Active » Needs review
solideogloria’s picture

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

I tested the MR using the new option and it works. It's a good solution, too.

Navigating to /user/login?showcore=1 just shows a button to log in with the configured provider. The Drupal core user login form is not displayed.

liquidcms’s picture

liquidcms commented
Status: Reviewed & tested by the community » Needs work

I think the logic behind this is incorrect. As the title suggests, there should be an option to "disable the use of showcore". This isn't the same as forcing the login page with the OIC button. It should simply stop showcore in the url from doing anything.

It is difficult to develop against possible future enhancements; but I'd say it is safe to assume the Autologin feature #3011413: Autologin when one client enabled will eventually be merged; and this patch does not take that into account. The admin UI for this shouldn't be "force replace" or anything to do with those options; it should simply be a separate checkbox for "Disable showcore" (or even better, it should be "Enable showcore" and have this security hole disabled by default).

With that feature disabled and autologin enabled; then going to user/login?anything-including-showcore would simply attempt to access whichever auth client is being used.

Patch on the way.

solideogloria’s picture

solideogloria commented

I use the Autologin feature as well, and the solution from the MR is working.

solideogloria’s picture

solideogloria commented

However, I do agree that a separate checkbox is probably the better solution.

liquidcms’s picture

liquidcms commented

@solideogloria, from what you describe in #6, that is not auto login. Auto login would not go to that page, it would just connect to your Auth provider. What do you see with auto login enabled?

Also, the name of the option (force replace) does sound like what you've described, which is not auto login.

We have done up a patch but would conflict with the auto login patch, so we've merged both functions into 1 patch. Just need to do some testing and will post here.

solideogloria’s picture

solideogloria commented

composer.json

        "drupal/openid_connect": "^3.0@alpha",
        "drupal/openid_connect_windows_aad": "^2.0@beta",

composer.patches.json

    "drupal/openid_connect": {
      "#3011413: Autologin when one client enabled": "./patches/openid_connect-3011413-m98.patch",
      "#3375886: Warning: Undefined array key 'iss_allowed_domains'": "./patches/openid_connect-3375886-m86.patch",
      "#3441149: Add option to disallow using the 'showcore' query parameter": "./patches/openid_connect-3441149-m108.diff"
    },

pfrilling changed the visibility of the branch 3.x to hidden.

pfrilling changed the visibility of the branch 3.x to active.

pfrilling opened merge request !145

pfrilling’s picture

pfrilling
Location Minster, OH
commented
Status: Needs work » Needs review

I created a new MR with the changes from MR #108 and added functional testing. I think this looks good if someone wants to review and RTBC.

pfrilling closed merge request !108

pfrilling changed the visibility of the branch 3.x to hidden.

solideogloria’s picture

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

Looks good to me. I've been using the changes for quite a while.

  • pfrilling committed 54746f31 on 3.x 
    Issue #3441149 by pfrilling, alt36, solideogloria, liquidcms: Add option...
pfrilling’s picture

pfrilling
Location Minster, OH
commented
Status: Reviewed & tested by the community » Fixed

Thanks everyone!

Status: Fixed » Closed (fixed)

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