Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect loop instead of error message when using external identity provider #16181

Open
mvarblow opened this issue May 28, 2024 · 4 comments
Open
Milestone

Comments

@mvarblow
Copy link
Contributor

mvarblow commented May 28, 2024

Describe the bug

If you enable an external identity provider such as Microsoft Entra ID and the configure the user settings to disable local sign-in and user registration, then users who have not been provisioned will not receive the "Site does not allow user registration" error as expected. Instead, they end up in an infinite redirect loop.

To Reproduce

Steps to reproduce the behavior:

  1. Start and Orchard Core site and enable the Microsoft Entra ID Authentication feature - see https://docs.orchardcore.net/en/latest/guides/microsoft-entra-id-integration/.
  2. Click on Security > Settings > User Login
  3. Check "Use external provider for login". Click Save.
  4. Click on Security > Settings> User Registration and ensure that "Configure users registration" is set to NoRegistration. Click Save.
  5. In a new incognito window, open the site and attempt to access the admin page. You'll be redirected to Microsoft Entra ID to sign in and then sent back to the Orchard Core site. The Orchard Core site will redirect you back to Entra ID. This time you won't be prompted to sign in since you're already signed in, you'll just be redirected back to the Orchard Core site. Orchard Core will redirect you to Entra ID. ....

Expected behavior

If you look in the Orchard Core log file you'll see a warning: "Site does not allow user registration." This warning will be repeated many times, depending on how long the user allowed the redirect loop to continue as they waited for the page to appear. I would expect that the user should receive an error message similar to what was logged instead of being continuously redirected. If I locate the code in AccountController which is responsible for generating that log entry, it appears that this is what the author of the code also intended. Though I think they didn't consider this scenario where the "use external provider for login" option was enabled.

image

If "use external provider for login" were disabled, then the redirect wouldn't happen. Instead, the user would see the sign-in page with the error message. To fix this, it seems that the AccountController needs to account for this possibility and replace the RedirectToLogin response with a view which can display the error message.

@Piedone
Copy link
Member

Piedone commented May 28, 2024

This looks indeed like something should be handled better. Although enabling external login, not having users pre-created, and not enabling registration together is also an incorrect configuration. Or did you do it deliberately?

@Piedone Piedone added the Users label May 28, 2024
@mvarblow
Copy link
Contributor Author

This looks indeed like something should be handled better. Although enabling external login, not having users pre-created, and not enabling registration together is also an incorrect configuration. Or did you do it deliberately?

In this case, we are expecting that users will be pre-created. An administrator needs to provision the user manually to ensure their account is set up correctly in the OrchardCore site. But there will occasionally be cases where a user tries to sign in before their OrchardCore account has been provisioned. In this case, we'd expect an error. What the user gets instead is a page that seems to the user to be loading "forever". Of course it's actually in a redirect loop.

@Piedone
Copy link
Member

Piedone commented May 29, 2024

I see, thanks.

@sebastienros sebastienros added this to the 2.x milestone May 30, 2024
Copy link
Contributor

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants