Fix unwanted redirect to IdP from locallogin.php in case alternateloginurl is set, resolves #775

[krostas1983]

As detailed in the corresponding issue, this fix solves various minor problems regarding locallogin.php:

• Login form should still be displayed if local login is not available due to alternateloginurl being set.
• Redirect should be to /index.php instead of /login/index.php to prevent certain configurations (again, alternateloginurl) from redirecting to an external login prompt with a valid session.

The feature provided by locallogin.php is very helpful for those cases already where the Moodle WAYF service is not used and all users are expected to come with a valid SSO session.

===========================================

On behalf of the Boost Union Team: 🎉 Thank you for contributing! 🎉

Please note: There must be a GitHub issue for every pull request (PR)

We kindly ask you to create a github issue now if you haven't already done so.

Please make sure to follow these steps to ease the review process for the peer review team:

[x] link your issue in the PR title, using the keyword 'resolves #ISSUE-NUMBER', e.g. 'Feature: Provide the ultimate user experience, resolves #42'
[x] provide any further information that is relevant for peer review and not yet mentioned in the linked issue as a comment in the PR
[x] make sure that the 'Allow edits by maintainers' checkbox is checked when creating the PR. Otherwise, the peer reviewer would not be able to push any review changes to the PR and the communication overhead increases
[x] submit your PR in draft status to run the automated checks and review the results
[ ] in case any checks fail solve the mentioned errors by pushing the corrected code to your PR-branch
[x] if all checks pass (or if you are unable to resolve the failing steps without any help of the review team), mark the PR as 'ready for review'

Thank you again for your contribution, we will start reviewing your PR as soon as we are able to.

In the meantime, please check our wiki page for creating pull requests and our wiki page for reviewing pull requests for further infomation about our contribution and review process.

[krostas1983]

Commit is compatible with branches MOODLE_401_STABLE to MOODLE_405_STABLE.

[abias]

Hi @krostas1983 ,

first of all, many thanks for working on this PR to improve Boost Union!

I have seen that you have rebased this PR after you have submitted it to integrate the latest changes from the main branch. While rebasing PRs is generally highly appreciated, it should be done correctly.

If you want to do the rebase your PR manually, we have published some instructions for proper rebasing here:
https://github.com/moodle-an-hochschulen/moodle-plugin-maintaining/wiki/Forking-a-plugin,-creating-a-pull-request-and-keeping-your-plugin-fork-repo-up-to-date-(correctly)#keeping-pull-requests-up-to-date
If you want to do it through the Github GUI, you should use the "rebase" strategy and not the "merge commit" strategy (see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/keeping-your-pull-request-in-sync-with-the-base-branch).
We, as peer reviewers, prefer the manual way as it gives more control over what's happening.

I have now rebased your PR properly and have force-pushed it into your branch.

On top of that, I have added a "Review changes" commit.
Up to now, it just adds entries in the CHANGES.md and the README.md and adds a Behat test to cover the (already existing) redirect from locallogin.php if you have a valid session – this test which was missing up to now in Boost Union.

Now, coming to the changes which you want to contribute with your PR, I have to admit that I do not really understand why you think this is a bug.

In #775, you write:

Go to 'Site administration' -> 'Plugins' -> 'Authentication' -> 'Manage authentication'
Verify that authentication method 'Shibboleth' is active.
Verify that 'alternateloginurl' is set to '/auth/shibboleth/index.php'.
Go to 'Site administration' -> 'Plugins' -> 'Authentication' -> 'Shibboleth'
Verify that Shibboleth is configured properly and especially 'alt_login' (Moodle WAYF service) is set to 'No'.
Go to 'Site administration' -> 'Appearance' -> 'Boost Union' -> 'Look' -> 'Login Page'
Verify that 'loginlocalloginenable' is set to 'No'.
In a new browser window (no session), go to '/theme/boost_union/locallogin.php'
Login with a manual account.
Verify that you have a valid session (i.e. are logged into Moodle).
Go to '/theme/boost_union/locallogin.php'
You should be redirected to the IdP of your Shibboleth configuration.

I can not really agree with the last item. From my point of you, if you are logged in in Moodle with a manual account and have a valid Moodle session, there is no need (and for me it would be wrong) to redirect you to your IdP if you visit the side entrace login page again.
Can you please clarify your expectation?

In addition to that, the line which you change in https://github.com/moodle-an-hochschulen/moodle-theme_boost_union/pull/776/files#diff-f1cadfd66b433c5156930588da135dcbc5e7a0479e55b4256ef395522265fff4R51 targets the case when 'loginlocalloginenable' is set to 'Yes'. But in your steps to reproduce you are talking about 'loginlocalloginenable' being set to 'No'.

Last but not least, I do not fully understand why we should redirect to /index.php instead of /login/index.php.
/login/index.php already does the handling of alternateloginurl (see https://github.com/moodle/moodle/blob/main/login/index.php#L327) and it is, from my point of view, the natural redirect target from the side entrace login page unless we want to re-implement all its logic in the side entrace login page as well.

But maybe I am just getting your PR completely wrong, so I am looking for your insights .

Cheers,
Alex

[krostas1983]

First off, thank you very much for fixing the branch by properly rebasing. I rushed to rebase late last evening and had no option to properly rebase manually after the fact.

From my point of you, if you are logged in in Moodle with a manual account and have a valid Moodle session, there is no need (and for me it would be wrong) to redirect you to your IdP if you visit the side entrace login page again.

You are absolutely correct. That misunderstanding may be attributed to the nature of my issue vs. your expectations. I provided the instructions to reproduce the bug (being redirected to your IdP even though you have an active session). Maybe your expectation was testing instructions for when it is working?
Either case, we seem to agree that a valid session should not redirect a user to the IdP, which currently does happen - thus the bug report.

In addition to that, the line which you change in https://github.com/moodle-an-hochschulen/moodle-theme_boost_union/pull/776/files#diff-f1cadfd66b433c5156930588da135dcbc5e7a0479e55b4256ef395522265fff4R51 targets the case when 'loginlocalloginenable' is set to 'Yes'. But in your steps to reproduce you are talking about 'loginlocalloginenable' being set to 'No'.

It does target the case that loginlocalloginenable is set to "Yes" (specifically not set to "No") and in that case deactivates the fallback page and instead displays a warning message (to not use the fallback but the regular login page instead). In order to properly use the local login fallback under /theme/boost_union/locallogin.php and to reproduce the bug, that setting has to be set to "No". (Otherwise no redirect ever happens as the check for redirecting happens after that part and script execution would be stopped via die;.)

The change expands the conditions under which this warning is shown to include alternateloginurl to be empty as well. Otherwise taking advantage of this feature (fallback for local login) would require admins to disable Boost Union's loginlocalloginenable feature (for a login page that is never used as it redirects to alternateloginurl instead). The case

• alternateloginurl is not empty
• loginlocalloginenable is not set to "No"

would not result in the bug from the issue, but would still leave admins with manual accounts unable to access the Moodle site.

[abias]

Thank you @krostas1983 for your clarifications. I think I get your points now. And I think we have two changes combined in one PR.

Let's keep things separate:

1. If the user has a valid (manual / local) session, do not redirect to /login/index.php as this page would redirect to the IdP.

The idea behind the current implementation of this redirect was that the side entrace login page should not have to care at all about any redirects. If a user has a session, there is nothing to do on the side entrace login page and it should redirect the user to the standard login page to handle everything which follows.
In the case of an existing Moodle session, this would be showing a message like this:

If we would change this now to redirecting to /index.php, we would break this basic idea and would dictate a redirect to the site homepage. This might be somehow bad if the site is configured to prefer the Dashboard, but might also harm other configurations which I am not aware of yet currently.

Instead, I would propose to change the redirect target in a way that, if alternateloginurl is set, then the side entrace login page should redirect to /login/index.php?loginredirect=0. That way, the standard login page would not contrinue to redirect to the IDP but it would still show the message which I pasted above.
This parameter for /login/index.php was introduced in https://tracker.moodle.org/browse/MDL-67053 for Moodle 4.4 and above.

2. Allow the side entrace login page to be used as fallback for Moodle setups with external IdPs.

Currently, the side entrance login page is automatically enabled if loginlocalloginenable is disabled.
I would fully support to cover your case, but simply adding the additional check as you have done in your patch would make the desired "External IdP support" something like an easter-egg feature.

You should at least amend the description of the loginlocalloginenable setting to cover this additional case.

But to do things properly, my gut feeling is that we should add a new setting to enable or disable the side entrance login page independently from the loginlocalloginenable setting. That new setting should be off by default, but it would have to be introduced with an upgrade script which sets it to enabled if loginlocalloginenable is disabled in the Moodle instance.

Would you agree with me or did I miss anything?
Do you think that you can change your patch into this direction or should I make a proposal?

Cheers,
Alex

[krostas1983]

Thanks for your feedback, effort in the review process and suggestions, @abias
I agree that the two issues should be discussed separately.

1. If the user has a valid (manual / local) session, do not redirect to /login/index.php as this page would redirect to the IdP.

As I understand Moodle's redirect policy regarding the start page, a fixed redirect to the site homepage would need to be implemented with /index.php?redirect=0, while a simple redirect do /index.php would facilitate further redirects where warranted (i.e. to Dashboard, Course Overview, etc., if so configured under defaulthomepage).

I think it would be beneficial to get the message notification about an existing session as per your screenshot, though. This should reflect the standard login workflow as accurately as possible. So yes, I do agree in the end that a redirect to /login/index.php as a default case and to /login/index.php?loginredirect=0 in case alternateloginurl is set should be our desired outcome of this fix.

2. Allow the side entrace login page to be used as fallback for Moodle setups with external IdPs.

From my point of view, there are some beneficial and detrimental points to your suggested course of action:
+ Direct control for site admins whether the fallback manual login is active or not.
- Possibility for site admins to lock themselves out of their Moodle through Boost Union config.

I fully support the proposal to amend the description to the admin setting(s) in question.
However, I suggest to make the setting somewhat flexible to allow for some human error on the admin side.

• Active (locallogin.php always allows manual logins)
• Dynamic (locallogin.php allows manual logins only when manual login is disabled otherwise)
• Inactive (locallogin.php never allows manual logins)

Even though the design philosophy of Boost Union is to change nothing about the way Moodle works by default, I suggest to make "Dynamic" the default option of these. For the sole reason of minimizing the chances of admins locking themselves out.

Another option to make this feature less "easter-eggy" could be an optional notification on /login/index.php in case no valid login options are left to display. (Possibly resolves #777 )

---

I will do the following steps moving forward:

1. Create a separate Issue for the second part
2. Move functionality from the first part into a separate feature branch
3. Move functionality from the second part into a separate feature branch
4. Adjust source branch for this PR & create new PR for second part

2 & 3 will be where the code changes happen. I will take this on with the help of my coworkers and come back on your offer for a proposal in case we're getting stuck.