Moodle
  1. Moodle
  2. MDL-29940

login page has embedded html and php code - needs rewrite for code separation

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.2
    • Fix Version/s: None
    • Component/s: Authentication, Themes
    • Labels:
    • Rank:
      19484

      Description

      while working on MDL-27793, we realized that login page needs to be updated drastically. There is gross code+html in login/index_form.html which is also simply included from login/index.php

      Also we need to ensure proper way for the login area to co-exist happily with all themes and languages and small browser sizes.

        Issue Links

          Activity

          Aparup Banerjee created issue -
          Aparup Banerjee made changes -
          Field Original Value New Value
          Link This issue has been marked as being related by MDL-27793 [ MDL-27793 ]
          Show
          Aparup Banerjee added a comment - please also read Sam's take on this here http://tracker.moodle.org/browse/MDL-27793?focusedCommentId=125816&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-125816
          Hide
          Kyle Temkin added a comment - - edited

          I've started work on this, and have completely re-written the login form code using the renderer paradigm. This should solve both of the problems above, as 1) there's no more gross hard-coded HTML; and 2) any theme can override the login form's renderer.

          This code is "complete" but only minimally tested:

          https://github.com/bumoodle/moodle/compare/WIP_MDL_29940

          I will write tests and test more thoroughly soon- but any help testing is appreciated. I'd very much like to hear any suggestions / comments from the core devs. I'm willing to restructure the code to be more paradigmatic, if recommended.

          (Also worth adding: in the code above, I worked to keep all of the form structures the same- and avoided the use of a potentially-more-paradigmatic Moodleform- in order to keep compatibility with existing themes. It might be better to re-write this using a moodleform; in which case, it would be a good idea to keep the code I've submitted as an core_auth_legacy renderer. Old themes would be then be able to use the core_auth_legacy renderer as a stand-in until their CSS is updated.)

          Show
          Kyle Temkin added a comment - - edited I've started work on this, and have completely re-written the login form code using the renderer paradigm. This should solve both of the problems above, as 1) there's no more gross hard-coded HTML; and 2) any theme can override the login form's renderer. This code is "complete" but only minimally tested: https://github.com/bumoodle/moodle/compare/WIP_MDL_29940 I will write tests and test more thoroughly soon- but any help testing is appreciated. I'd very much like to hear any suggestions / comments from the core devs. I'm willing to restructure the code to be more paradigmatic, if recommended. (Also worth adding: in the code above, I worked to keep all of the form structures the same- and avoided the use of a potentially-more-paradigmatic Moodleform- in order to keep compatibility with existing themes. It might be better to re-write this using a moodleform; in which case, it would be a good idea to keep the code I've submitted as an core_auth_legacy renderer. Old themes would be then be able to use the core_auth_legacy renderer as a stand-in until their CSS is updated.)
          Aparup Banerjee made changes -
          Labels triaged
          Aparup Banerjee made changes -
          Labels triaged patch triaged
          Aparup Banerjee made changes -
          Assignee moodle.com [ moodle.com ] Aparup Banerjee [ nebgor ]
          Hide
          Aparup Banerjee added a comment - - edited

          pushing this up for peer-review for Kyle.

          (looks good from a glance although i'm wondering about the location of core_auth* files..)

          ps: i've tried to assign this MDL to Kyle but i couldn't find him there. I've pinged Michael De Raadt about this.

          Show
          Aparup Banerjee added a comment - - edited pushing this up for peer-review for Kyle. (looks good from a glance although i'm wondering about the location of core_auth* files..) ps: i've tried to assign this MDL to Kyle but i couldn't find him there. I've pinged Michael De Raadt about this.
          Aparup Banerjee made changes -
          Status Open [ 1 ] Waiting for peer review [ 10012 ]
          Pull Master Diff URL https://github.com/bumoodle/moodle/compare/WIP_MDL_29940
          Pull Master Branch WIP_MDL_29940
          Pull from Repository git://github.com/bumoodle/moodle.git
          Hide
          Michael de Raadt added a comment -

          Hi, Kyle.

          I've promoted you to the jira-developers group. This allows you to be assigned to issues, edit issues, and push to peer review. (http://docs.moodle.org/dev/Tracker#Tracker_groups_and_permissions).

          I've assigned this issue to you to reflect your contribution. Thanks for your continued work on this and other issues. The community really appreciates your efforts.

          Show
          Michael de Raadt added a comment - Hi, Kyle. I've promoted you to the jira-developers group. This allows you to be assigned to issues, edit issues, and push to peer review. ( http://docs.moodle.org/dev/Tracker#Tracker_groups_and_permissions ). I've assigned this issue to you to reflect your contribution. Thanks for your continued work on this and other issues. The community really appreciates your efforts.
          Michael de Raadt made changes -
          Assignee Aparup Banerjee [ nebgor ] Kyle Temkin [ ktemkin ]
          Hide
          Charles Fulton added a comment -

          Just thinking aloud but the Shibboleth login page should probably be rewritten to match if this is taken up.

          Show
          Charles Fulton added a comment - Just thinking aloud but the Shibboleth login page should probably be rewritten to match if this is taken up.
          Jérôme Mouneyrac made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Peer reviewer jerome
          Jérôme Mouneyrac made changes -
          Link This issue blocks MDL-34426 [ MDL-34426 ]
          Jérôme Mouneyrac made changes -
          Link This issue is blocked by MDLTEST-11 [ MDLTEST-11 ]
          Hide
          Petr Škoda added a comment -

          Hi, I agree we should somehow reimplement the login page, but I am afraid this is not the expected use of renderers. Renders are supposed to deal with visual rendering of data, it must not contain login code logic, sorry. If we decide to introduce some kind of identity providers we should rework auth plugins from the ground up at the same time - that is a huge task and we do not have enough time to do it before 2.4.

          Maybe we could let one auth plugin to handle everything on the login page - auth_manual by default and add a new selection option in admin settings. This would allow developers to implement auth plugin that implements any type of identity provider. The benefit would be full backwards compatibility and flexibility for any custom solutions before we decide to redesign our auth subsystem.

          In any case we are we close to the 2.4dev freeze, I am afraid this will have to wait until 2.5...

          Show
          Petr Škoda added a comment - Hi, I agree we should somehow reimplement the login page, but I am afraid this is not the expected use of renderers. Renders are supposed to deal with visual rendering of data, it must not contain login code logic, sorry. If we decide to introduce some kind of identity providers we should rework auth plugins from the ground up at the same time - that is a huge task and we do not have enough time to do it before 2.4. Maybe we could let one auth plugin to handle everything on the login page - auth_manual by default and add a new selection option in admin settings. This would allow developers to implement auth plugin that implements any type of identity provider. The benefit would be full backwards compatibility and flexibility for any custom solutions before we decide to redesign our auth subsystem. In any case we are we close to the 2.4dev freeze, I am afraid this will have to wait until 2.5...
          Hide
          Jérôme Mouneyrac added a comment -

          No problem I won't peer-review. In the Oauth2 plugins I also created the core_auth renderer, I thought it would have been good to integrate Kyle's contribution before the Oauth2 plugins.

          About the identity providers thing: for the Oauth2 plugins I'm using the current identity provider function: loginpage_idp_list(). I'm confused because I read that you suggest first that we need a big refactor for 2.5 and then you write about adding some "patchy" login code in the manual plugin for 2.4 - which from what I understand would be changed in 2.5.

          Show
          Jérôme Mouneyrac added a comment - No problem I won't peer-review. In the Oauth2 plugins I also created the core_auth renderer, I thought it would have been good to integrate Kyle's contribution before the Oauth2 plugins. About the identity providers thing: for the Oauth2 plugins I'm using the current identity provider function: loginpage_idp_list(). I'm confused because I read that you suggest first that we need a big refactor for 2.5 and then you write about adding some "patchy" login code in the manual plugin for 2.4 - which from what I understand would be changed in 2.5.
          Jérôme Mouneyrac made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Jérôme Mouneyrac made changes -
          Peer reviewer jerome moodle.com
          Jérôme Mouneyrac made changes -
          Link This issue is blocked by MDLTEST-11 [ MDLTEST-11 ]
          Hide
          Aparup Banerjee added a comment - - edited

          ah, we could do this with mform (index_form.php for the logic) and a rendering for the login page itself.

          found an old branch i had lying around (this is completely unfinished and unworkable atm) : https://github.com/nebgor/moodle/commits/WIP-MDL-27793_login_page_rewrite
          see commit https://github.com/nebgor/moodle/commit/2cd3fb358765bf93f50c6f5390ed869e99b934a2

          Show
          Aparup Banerjee added a comment - - edited ah, we could do this with mform (index_form.php for the logic) and a rendering for the login page itself. found an old branch i had lying around (this is completely unfinished and unworkable atm) : https://github.com/nebgor/moodle/commits/WIP-MDL-27793_login_page_rewrite see commit https://github.com/nebgor/moodle/commit/2cd3fb358765bf93f50c6f5390ed869e99b934a2
          Hide
          Petr Škoda added a comment - - edited

          I meant:

          • big rewrite = changes in all auth plugins, changes in 'user' database table, changes everywhere - this defines user as one entity with multiple ways to log in and there could be multiple instances of one auth plugin - this would need a lot of time
          • smaller change in login page only - that is to completely delegate login page to one selected auth plugin - a lot easier, because it is mostly refactoring with backwards compatibility, could be done in 2.5 I think, 4 weeks do not seem enough to me
          Show
          Petr Škoda added a comment - - edited I meant: big rewrite = changes in all auth plugins, changes in 'user' database table, changes everywhere - this defines user as one entity with multiple ways to log in and there could be multiple instances of one auth plugin - this would need a lot of time smaller change in login page only - that is to completely delegate login page to one selected auth plugin - a lot easier, because it is mostly refactoring with backwards compatibility, could be done in 2.5 I think, 4 weeks do not seem enough to me
          Hide
          Kyle Temkin added a comment - - edited

          smaller change in login page only - that is to completely delegate login page to one selected auth plugin - a lot easier, because it is mostly refactoring with backwards compatibility, could be done in 2.5 I think, 4 weeks do not seem enough to me

          That sounds like a good starting point. Perhaps the first step of that process should be to move the existing login-page logic into auth_plugin_base; possibly as a function called login_form, paralleling the existing config_form.

          It could be a good idea to have that function return a moodleform, rather than rending the page by itself. The default base class implementation could be something like this (pseudo)code:

          public function login_form() {
              
              $login_form_file = $CFG->dirroot.'/auth/'.$this->authtype.'/login_form.php';
              $login_form_name = 'auth_'.$this->authtype.'_login_form';
          
              // If the plugin form has a login form defined, use it...
              if(file_exists($login_form)) {
                  include_once($login_form);
                  return new $login_form_name();
              } else {
                  //default username-and-password login form
                  return new auth_plugin_login_form();
              }
          }
          

          This would allow us to maintain backwards-compatibility with older login plugins that don't define their own login forms- and would also allow us to support authentication methods with non-typical schemes; like many modern two-factor authentication methods.

          We could further extend this model by adding a "validate_login" function, which would eventually replace user_login. The compatible default could be something like:

          public function validate_login($formdata) {
          
              // Default implementation; should be overridden by most modern login plugins.
              debugging('You should update your login plugin to use the newer "validate login" function instead of "user_login".', DEBUG_DEVELOPER);
          
              // Delegate to the legacy auth-login plugin.
              return $this->user_login($formdata->username, $formdata->password);
          }
          
          public function user_login($username, $password) {
             print_error('mustbeoveride', 'debug', '', 'validate_login()');
          }
          
          Show
          Kyle Temkin added a comment - - edited smaller change in login page only - that is to completely delegate login page to one selected auth plugin - a lot easier, because it is mostly refactoring with backwards compatibility, could be done in 2.5 I think, 4 weeks do not seem enough to me That sounds like a good starting point. Perhaps the first step of that process should be to move the existing login-page logic into auth_plugin_base ; possibly as a function called login_form , paralleling the existing config_form . It could be a good idea to have that function return a moodleform, rather than rending the page by itself. The default base class implementation could be something like this (pseudo)code: public function login_form() { $login_form_file = $CFG->dirroot.'/auth/'.$ this ->authtype.'/login_form.php'; $login_form_name = 'auth_'.$ this ->authtype.'_login_form'; // If the plugin form has a login form defined, use it... if (file_exists($login_form)) { include_once($login_form); return new $login_form_name(); } else { // default username-and-password login form return new auth_plugin_login_form(); } } This would allow us to maintain backwards-compatibility with older login plugins that don't define their own login forms- and would also allow us to support authentication methods with non-typical schemes; like many modern two-factor authentication methods. We could further extend this model by adding a "validate_login" function, which would eventually replace user_login . The compatible default could be something like: public function validate_login($formdata) { // Default implementation; should be overridden by most modern login plugins. debugging('You should update your login plugin to use the newer "validate login" function instead of "user_login" .', DEBUG_DEVELOPER); // Delegate to the legacy auth-login plugin. return $ this ->user_login($formdata->username, $formdata->password); } public function user_login($username, $password) { print_error('mustbeoveride', 'debug', '', 'validate_login()'); }
          Jérôme Mouneyrac made changes -
          Link This issue blocks MDL-34426 [ MDL-34426 ]
          Hide
          Stacey Walker added a comment - - edited

          Hi,

          I've actually just been working on a possible patch for the same problem so thought I'd add to this stream.

          I have kept it a bit simpler though by merely shifting the HTML markup from the included file to the core renderer. The structure and use of the coded HTML forms is effectively the same, aside for a small breakup of the overall code into small helper functions where it helps with understanding the structure.

          My thinking is that this issue seemed originally intended to solve a background "visual" change and no huge chunks of functionality should actually be affected as part of it. So the larger changes such as moving to use Moodleforms, fixing values where constant definitions would make more sense (which are used in a bigger scope than just this area anyway) and expanding on the amount of authentication plugin control should all be separated out; resolved and tested independently.

          The commit I've created for the renderering is here https://github.com/lamiette/moodle/commits/MDL-29940 I realise it won't make it into 2.5 with the current code freeze but it's here for review anyway.

          I actually started on this because I needed to have greater flexibility with the way the potential IDPs were displayed (removing the requirement for the icon value) and figured the HTML itself could be improved first. I've put in the minor change I needed of the potential_idps(..) display not breaking if no icon is supplied and this seems reasonable for all authentication plugins. I've also left in a couple of ToDo comments around the idea of moving this to the authentication plugin itself as I think it'd make more sense if the auth could choose how and what to display as it wishes and not be so restricted by core.

          I'll create some more issues to separate out these and will link to the relevant ones here for future tracking.

          Thanks,
          Stacey

          Show
          Stacey Walker added a comment - - edited Hi, I've actually just been working on a possible patch for the same problem so thought I'd add to this stream. I have kept it a bit simpler though by merely shifting the HTML markup from the included file to the core renderer. The structure and use of the coded HTML forms is effectively the same, aside for a small breakup of the overall code into small helper functions where it helps with understanding the structure. My thinking is that this issue seemed originally intended to solve a background "visual" change and no huge chunks of functionality should actually be affected as part of it. So the larger changes such as moving to use Moodleforms, fixing values where constant definitions would make more sense (which are used in a bigger scope than just this area anyway) and expanding on the amount of authentication plugin control should all be separated out; resolved and tested independently. The commit I've created for the renderering is here https://github.com/lamiette/moodle/commits/MDL-29940 I realise it won't make it into 2.5 with the current code freeze but it's here for review anyway. I actually started on this because I needed to have greater flexibility with the way the potential IDPs were displayed (removing the requirement for the icon value) and figured the HTML itself could be improved first. I've put in the minor change I needed of the potential_idps(..) display not breaking if no icon is supplied and this seems reasonable for all authentication plugins. I've also left in a couple of ToDo comments around the idea of moving this to the authentication plugin itself as I think it'd make more sense if the auth could choose how and what to display as it wishes and not be so restricted by core. I'll create some more issues to separate out these and will link to the relevant ones here for future tracking. Thanks, Stacey
          Hide
          Stacey Walker added a comment -

          Move login page HTML markup rendered forms to Moodleforms

          Show
          Stacey Walker added a comment - Move login page HTML markup rendered forms to Moodleforms
          Stacey Walker made changes -
          Link This issue has a non-specific relationship to MDL-38976 [ MDL-38976 ]
          Hide
          Stacey Walker added a comment -

          Define $CFG->rememberusername (NB: may be blocked by or block this issue)

          Show
          Stacey Walker added a comment - Define $CFG->rememberusername (NB: may be blocked by or block this issue)
          Stacey Walker made changes -
          Link This issue has a non-specific relationship to MDL-38977 [ MDL-38977 ]
          Kyle Temkin made changes -
          Status Development in progress [ 3 ] Open [ 1 ]
          Hide
          Daniel Skinner added a comment -

          +1. Would be good to be able to control the HTML output for the login screen at the theme level. This would be very useful for modernising the HTML (e.g. when using the bootstrap theme).

          Show
          Daniel Skinner added a comment - +1. Would be good to be able to control the HTML output for the login screen at the theme level. This would be very useful for modernising the HTML (e.g. when using the bootstrap theme).
          Michael de Raadt made changes -
          Component/s Authentication [ 10067 ]
          Component/s Themes [ 10088 ]
          Component/s General [ 10049 ]

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated: