Comments

• 

  Tomasz Muras

  Wed, 26 Nov 2014, 6:08 AM

  Hi Chris,
  
  I'm having problems with Drupal-side or Moodle-Drupal integration, maybe you can help me with debugging.
  
  Status:
  Success : cookies: SSO Cookie discovered properly
  Fail : system/connect: User session data reachable but you aren't logged in!
  Success : user/login: Logged in to drupal!
  Fail : muser/Index: The muser resource is not available in the drupal service endpoint.
  
  The WS call that fails:
  array(26) { ["url"]=> string(48) "http://localhost/drupal/moodlesso/system/connect" ["content_type"]=> NULL ["http_code"]=> int(401) ["header_size"]=> int(0) ["request_size"]=> int(251) ["filetime"]=> int(-1) ["ssl_verify_result"]=> int(0) ["redirect_count"]=> int(0) ["total_time"]=> float(0.033002) ["namelookup_time"]=> float(1.1E-5) ["connect_time"]=> float(3.4E-5) ["pretransfer_time"]=> float(6.9E-5) ["size_upload"]=> float(0) ["size_download"]=> float(0) ["speed_download"]=> float(0) ["speed_upload"]=> float(0) ["download_content_length"]=> float(-1) ["upload_content_length"]=> float(0) ["starttransfer_time"]=> float(0.032964) ["redirect_time"]=> float(0) ["redirect_url"]=> string(0) "" ["primary_ip"]=> string(9) "127.0.0.1" ["certinfo"]=> array(0) { } ["primary_port"]=> int(80) ["local_ip"]=> string(9) "127.0.0.1" ["local_port"]=> int(57675) }
  
  As you can see I'm getting 401 (unauthorized) but when trying to access http://localhost/drupal/moodlesso/system/connect manually I always (even as my remote user) get 404 (not found), so that 401 may be a red herring.
  Other calls like http://localhost/drupal/moodlesso/user seem OK and I can see in services->resources settings that system -> connect is enabled.
  
  Would you have any hints/ideas on the debugging next?
  
  Tomek
• 

  chris porter

  Wed, 26 Nov 2014, 6:54 AM

  This is because you are using the 1.x version of the drupalservices plugin, and are using the moodle_sso module. You should switch to the 2.x-2.x branch of the plugin. things will work much better for you.
  
  Also, this is really not the place for support requests, Please submit them to the drupalservices plugin issue queue:
  https://github.com/cannod/moodle-drupalservices/issues
  
• 

  David Mudrák

  Wed, 26 Nov 2014, 7:15 AM

  Thanks for sharing this plugin Chris. Just to clarify, Tomek is a member of our plugins approval team and he is currently peer-reviewing this plugin.
• 

  Tomasz Muras

  Tue, 2 Dec 2014, 4:25 AM

  Hi Chris,
  
  Thanks for sharing your plugin, I will continue the review of https://moodle.org/plugins/download.php/7303/auth_drupalservices_moodle28_2014111400.zip not earlier than in 1 week so I'm listing my initial findings now.
  
  1. Copyright information shold be put in all files (just like you have in auth.php).
  2. There is a lot of notices during and after the installation of your plugin, if you turn on full debug in Moodle you should see them.
  3. http://localhost/drupal/moodlesso/moodlesso returns for me: (notice cookie_domain is empty) Do I use correct Drupal plugin - I have Moodle SSO 7.x-1.x-dev.
  4. I think you could mention that clean URLs are required for this to work (since you add /moodlesso to the URL)
  5. Troubleshooting is a bit difficult - if anything is wrong (e.g. cookie) then all you get is message "The status of the Moodle/Drupal SSO connection is listed below" with no extra text below.
  6. Do you know that you've bumped required PHP version to PHP 5.5 with this:
  empty($drupalsession = $this->get_drupal_session())) ?
  Moodle itself will run on lower version.
  
  Tomek
• 

  Tomasz Muras

  Tue, 2 Dec 2014, 4:22 PM

  Question from Chris:
  I have a question for you about the efficiency of using settings.php vs the config_form method w/in the auth_plugin class. I much prefer the admin_settings_xxx() functions for providing settings fields, but it appears that these functions only work under the context of the settings.php file. the data passed in to the config_form() method don't seem to be able to accept the results of the admin_settings_xxx functions.
  It would be great if I could drop the settings.php file entirely, since using it causes a double link to show up in the navigation area when visiting administration -> plugins -> auth.
• 

  chris porter

  Wed, 3 Dec 2014, 3:59 AM

  This is 1/2 resolved. It turns out the settings.php code just wasn't using the $settings variable that is set up by the calling function. getting rid of that and using that variable space solved the double navigation problem for me. I'm still a bit curious about the settings.php vs config_form() aspect though.
• 

  David Mudrák

  Wed, 3 Dec 2014, 5:06 PM

  Thanks again Tomek for providing a valuable feedback, and Chris for responding to it. For now, I would suggest to focus on essential things that should be fixed before we make this available/public. It basically means that we should have the plugin ready to safely serve to sites who want to start use it. Things like PHP notices displaying all over the screen do not leave a good impression and do not help the reputation of this plugin (and Moodle plugins generally). So a good documentation and smooth installation is a must IMHO.
  
  Internal implementation of settings management can surely be improved later as it is possible to upgrade from one mechanism to another. Also, once this is public, Chris will get a lot of other comments from the community to work on. And can discuss further improvements in moodle.org forums.
• 

  chris porter

  Wed, 3 Dec 2014, 11:10 PM

  Tomek's feedback was great, he's a good dude. I rolled in fixes for all of the relevant changes yesterday, and those changes are now part of both master, and the 2.x-2.x-dev release of the module. I think the documentation is pretty solid, there are a number of testers in the github issue queue for this fork, and the installation process is by design much easier now.
  
  David is right that the goal of a code review isn't to get a product to a perfect state, its to be sure it's written in the correct way, uses the right API's and won't expose users to security risks etc. Once publicly available to the community, those users will help identify the actual issues surrounding this code, and it'll be my (and any co-maintainers) job to resolve that quickly.
  
• 

  David Mudrák

  Thu, 4 Dec 2014, 4:25 PM

  Thanks Chris. May I ask for uploading the new version ZIP to this plugin directory then? Note that you can use our Github repository in the filepicker to fetch the current branch and/or tagged version from Github directly. I'll be happy to approve this plugin once I get a confirmation from Tomek that the runway is free.
• 

  chris porter

  Sat, 6 Dec 2014, 12:55 AM

  I created a new release (alpha 2) for you guys to review. The module is shaping up nicely, and the testers that currently use this module have done a really good job of shaking out bugs so far.
• 

  Tomasz Muras

  Thu, 11 Dec 2014, 5:51 AM

  I've run it by our checklist - it's good to go for approval.
  
  FYI Chris:
  in sync_users(), this can't work:
  foreach ($userlist as $uid) {...
  because $userlist is not defined before that.