Details

    • Testing Instructions:
      Hide

      Testing core_course_get_contents()
      ==================================

      Get a PHP web service client: https://github.com/moodlehq/sample-ws-clients

      1- Call the core_course_get_contents($courseid) web service function on a big course (for example restore http://hub.moodle.org/?courseid=278)
      2- check that the content you receive is correct (try to check one activity per module type)
      3- finally call this function when the https setting in enabled (all the site in https only). The returned file download urls should be in https (for example for file resource).

      Testing the file download script:
      =================================
      1- Download one of the file url you got from your previous core_course_get_contents(). You'll need to add your ws token at the end (?token=893ny873b873b). Check that you can retrieve the right file.
      2- Set the 'Can download file' in service advance settings to OFF. Then try to download the file again. It should fail.
      3- Try some fail cases from the following list: ip restriction/unvalid date on token/service, user not listed in restricted list, missing required capability, site in maintenance mode, user suspended, user confirmed, user deleted, nologin auth, password expired, web service disabled. I added some info about that in the docs: http://docs.moodle.org/21/en/Web_services_FAQ#What_is_the_.27Access_control_exception.27_error

      Show
      Testing core_course_get_contents() ================================== Get a PHP web service client: https://github.com/moodlehq/sample-ws-clients 1- Call the core_course_get_contents($courseid) web service function on a big course (for example restore http://hub.moodle.org/?courseid=278 ) 2- check that the content you receive is correct (try to check one activity per module type) 3- finally call this function when the https setting in enabled (all the site in https only). The returned file download urls should be in https (for example for file resource). Testing the file download script: ================================= 1- Download one of the file url you got from your previous core_course_get_contents(). You'll need to add your ws token at the end (?token=893ny873b873b). Check that you can retrieve the right file. 2- Set the 'Can download file' in service advance settings to OFF. Then try to download the file again. It should fail. 3- Try some fail cases from the following list: ip restriction/unvalid date on token/service, user not listed in restricted list, missing required capability, site in maintenance mode, user suspended, user confirmed, user deleted, nologin auth, password expired, web service disabled. I added some info about that in the docs: http://docs.moodle.org/21/en/Web_services_FAQ#What_is_the_.27Access_control_exception.27_error
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-28646
    • Rank:
      19485

      Description

      This web service will be used to return course activities and their contents. It should be generic to handle files and contents.

      For resource module, it should return files url.

      Needs more details and specs

        Issue Links

          Activity

          Hide
          Dongsheng Cai added a comment -

          I was thinking about this web service today, there are a few problems we need to notice:

          1. Folder module, moodle web service isn't capable to represent tree data, so we may need to ask moodle create a zip file, we download the file and present the tree locally
          2. I am afraid we cannot create a generic web service to return all module contents, because moodle web service cannot return dynamic data structures, the return structure are defined in service.php and the actually return value must match web service definition.
          3. We probably still need more web services to load activity contents, like get_forum_contents, this get_course_contents web service will be used to return course structure and essential activity summaries.
          Show
          Dongsheng Cai added a comment - I was thinking about this web service today, there are a few problems we need to notice: Folder module, moodle web service isn't capable to represent tree data, so we may need to ask moodle create a zip file, we download the file and present the tree locally I am afraid we cannot create a generic web service to return all module contents, because moodle web service cannot return dynamic data structures, the return structure are defined in service.php and the actually return value must match web service definition. We probably still need more web services to load activity contents, like get_forum_contents, this get_course_contents web service will be used to return course structure and essential activity summaries.
          Show
          Jérôme Mouneyrac added a comment - - edited Usable WIP: https://github.com/mouneyrac/moodle/commit/e6e88145300a60bc13245eca173b6acd910ddd49
          Hide
          Dongsheng Cai added a comment -
          1. we should have modicon field for each module
          2. In topic course format, I have a section without modules, but it has name and description, it's not shown in web service response
          Show
          Dongsheng Cai added a comment - we should have modicon field for each module In topic course format, I have a section without modules, but it has name and description, it's not shown in web service response
          Hide
          Jérôme Mouneyrac added a comment -

          1. & 2. added.

          Show
          Jérôme Mouneyrac added a comment - 1. & 2. added.
          Hide
          Dongsheng Cai added a comment -

          A note:

          if section name doesn't exist, web service should use default section name or an empty string, section name field should be required.

          Show
          Dongsheng Cai added a comment - A note: if section name doesn't exist, web service should use default section name or an empty string, section name field should be required.
          Hide
          Dongsheng Cai added a comment -

          Just found one problem, for resource module, we should return all available files not just main, I'm working on it.

          Show
          Dongsheng Cai added a comment - Just found one problem, for resource module, we should return all available files not just main, I'm working on it.
          Hide
          Jérôme Mouneyrac added a comment -

          I found an issue:
          1- Download from the community finder the course called: ECOIS-training: Moodle voor docenten (2 dagen)
          2- Call the function
          3- An exception is raised by the web service returned values validation. The URL resource called '''Toelichting: groepen''' has for fileurl => $@BOOKVIEWBYID*1102@$&chapterid=123

          I'm not sure where the issue come from (backup process ? restore process? url resource?), but this kind of bug could happens again I guess.

          I think the function should clean fileurl, and if ever fileurl != cleaned fileurl, then we just don't send back the element, or marked if as failure?

          Show
          Jérôme Mouneyrac added a comment - I found an issue: 1- Download from the community finder the course called: ECOIS-training: Moodle voor docenten (2 dagen) 2- Call the function 3- An exception is raised by the web service returned values validation. The URL resource called '''Toelichting: groepen''' has for fileurl => $@BOOKVIEWBYID*1102@$&chapterid=123 I'm not sure where the issue come from (backup process ? restore process? url resource?), but this kind of bug could happens again I guess. I think the function should clean fileurl, and if ever fileurl != cleaned fileurl, then we just don't send back the element, or marked if as failure?
          Hide
          Dongsheng Cai added a comment -

          The above issues have been fixed.

          Show
          Dongsheng Cai added a comment - The above issues have been fixed.
          Hide
          Rossiani Wijaya added a comment -

          Hi DS,

          In topic course format, I have a section without modules, but it has name and description, it's not shown in web service response
          

          Your above comment has not been fixed in topic and weekly course format.

          I'm not sure how to reproduce the issue on a download from community finder (Jerome's last comment). could you provide some instruction steps for it?

          Show
          Rossiani Wijaya added a comment - Hi DS, In topic course format, I have a section without modules, but it has name and description, it's not shown in web service response Your above comment has not been fixed in topic and weekly course format. I'm not sure how to reproduce the issue on a download from community finder (Jerome's last comment). could you provide some instruction steps for it?
          Hide
          Dongsheng Cai added a comment -

          Rossiani

          The section name has been fixed, please re-fetch it from my github.

          > I'm not sure how to reproduce the issue on a download from community finder (Jerome's last comment). could you provide some instruction steps for it?

          It's been fixed, so you cannot reproduce it anymore.

          Show
          Dongsheng Cai added a comment - Rossiani The section name has been fixed, please re-fetch it from my github. > I'm not sure how to reproduce the issue on a download from community finder (Jerome's last comment). could you provide some instruction steps for it? It's been fixed, so you cannot reproduce it anymore.
          Hide
          Rossiani Wijaya added a comment -

          Thanks for fixing this DS.

          Patch looks great.

          Show
          Rossiani Wijaya added a comment - Thanks for fixing this DS. Patch looks great.
          Hide
          Martin Dougiamas added a comment -

          This one basically HAS to land for 2.2

          Show
          Martin Dougiamas added a comment - This one basically HAS to land for 2.2
          Hide
          Martin Dougiamas added a comment -

          Dongsheng, can you please review /webservice/pluginfile.php and make sure it's doing full checks? For example, it doesn't actually seem to be checking if the token supplied actually allows that user to download files.

          Show
          Martin Dougiamas added a comment - Dongsheng, can you please review /webservice/pluginfile.php and make sure it's doing full checks? For example, it doesn't actually seem to be checking if the token supplied actually allows that user to download files.
          Hide
          Dongsheng Cai added a comment -

          Martin

          Is your comment regarding this issue: MDL-28629?

          Show
          Dongsheng Cai added a comment - Martin Is your comment regarding this issue: MDL-28629?
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Dongsheng,
          I've had a look through this and heres my 2c:

          webservice/pluginfile.php :

          • i think its better to wrap this service, that depends on Tokens for access, within an extension to class webservice_base_server. Theres checks ( see load_function_info() ) that do proper access checks that ws/pluginfile.php is trying to reimplement (but not fully). This way we can define webservices to strict secure policies that are centralised in that one class.

          course/externallib.php :

          • summaryformat param PARAM_INT instead?
          • modname param : PARAM_PLUGIN instead?

          the rest looks good to me but this needs more work to be secure. Eloy and others may have other ideas of implementing security quicker.

          Show
          Aparup Banerjee added a comment - - edited Hi Dongsheng, I've had a look through this and heres my 2c: webservice/pluginfile.php : i think its better to wrap this service, that depends on Tokens for access, within an extension to class webservice_base_server. Theres checks ( see load_function_info() ) that do proper access checks that ws/pluginfile.php is trying to reimplement (but not fully). This way we can define webservices to strict secure policies that are centralised in that one class. course/externallib.php : summaryformat param PARAM_INT instead? modname param : PARAM_PLUGIN instead? the rest looks good to me but this needs more work to be secure. Eloy and others may have other ideas of implementing security quicker.
          Hide
          Dongsheng Cai added a comment -

          > summaryformat param PARAM_INT instead?
          External application doesn't know the meaning of the int number, so I think it makes more sense to use string here.

          > modname param : PARAM_PLUGIN instead?
          Agreed, will fix that

          Regarding the security check, I got comments from mixed sources (Martin's from Petr, Aparup's from Eloy and Jerome), please comment here directly.

          From what I understand, we are not sure about webservice/pluginfile.php of token to setup user session. The token is associated with moodle web service pack, say our mobile service, because of download script is not really a web service function, if we do need to check if the token allows user to download files, we will need a fake web service function, if it included in web service pack, then download requests allowed, if not reject download request, does this make sense?

          Show
          Dongsheng Cai added a comment - > summaryformat param PARAM_INT instead? External application doesn't know the meaning of the int number, so I think it makes more sense to use string here. > modname param : PARAM_PLUGIN instead? Agreed, will fix that Regarding the security check, I got comments from mixed sources (Martin's from Petr, Aparup's from Eloy and Jerome), please comment here directly. From what I understand, we are not sure about webservice/pluginfile.php of token to setup user session. The token is associated with moodle web service pack, say our mobile service, because of download script is not really a web service function, if we do need to check if the token allows user to download files, we will need a fake web service function, if it included in web service pack, then download requests allowed, if not reject download request, does this make sense?
          Hide
          Dongsheng Cai added a comment -

          I had talked with Eloy, I summarise the ideas here:

          1. We need to validate the token's contexts, make sure it only worked in certain contexts, for example, if users request a file in course A, but token is only allowed to download from course B, then pluginfile.php should reject the download request. I talked with Jerome, the problem is there is no context restricts on token and external service at all (the web service function pack), we need to implement this in web service core and somehow indicate the download capability in certain web service pack (maybe a fake download ws function)
          2. Each web service function should have limited access to moodle files, for example get_forum_attachments should only be able to access forum_attachments area, not course_files area, two things we need to do to archive this: 1. add new web service description to define file areas 2. ws/pluginfile.php needs to know which ws function is request the file. And we should have a ws function to validate URLs embedded in web service returns to authorise the uses.
          Show
          Dongsheng Cai added a comment - I had talked with Eloy, I summarise the ideas here: We need to validate the token's contexts, make sure it only worked in certain contexts, for example, if users request a file in course A, but token is only allowed to download from course B, then pluginfile.php should reject the download request. I talked with Jerome, the problem is there is no context restricts on token and external service at all (the web service function pack), we need to implement this in web service core and somehow indicate the download capability in certain web service pack (maybe a fake download ws function) Each web service function should have limited access to moodle files, for example get_forum_attachments should only be able to access forum_attachments area, not course_files area, two things we need to do to archive this: 1. add new web service description to define file areas 2. ws/pluginfile.php needs to know which ws function is request the file. And we should have a ws function to validate URLs embedded in web service returns to authorise the uses.
          Hide
          Dongsheng Cai added a comment -

          updated

          This patch requires MDL-30268 for new db field

          Show
          Dongsheng Cai added a comment - updated This patch requires MDL-30268 for new db field
          Hide
          Martin Dougiamas added a comment -

          Sorry integrators but this is quite urgent to deal with as:

          • the Mobile app depends on this, and we'd like to release it with 2.2.
          • Dongsheng is going on leave in one week.

          Note the dependency on Jerome's MDL-30268

          Show
          Martin Dougiamas added a comment - Sorry integrators but this is quite urgent to deal with as: the Mobile app depends on this, and we'd like to release it with 2.2. Dongsheng is going on leave in one week. Note the dependency on Jerome's MDL-30268
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (sam is going to take a look to this)

          Show
          Eloy Lafuente (stronk7) added a comment - (sam is going to take a look to this)
          Hide
          Martin Dougiamas added a comment -

          OK, go Sam!

          Show
          Martin Dougiamas added a comment - OK, go Sam!
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Interesting stuff…. not yet sorry a few technical problems and a certainly I think the approach in regards to the new callback needs to be discussed.

          About the approach:

          1. I really really dislike the new something_export_contents.
            Personally I think the approach is wrong, I think that get_course_contents should not return that information instead the client should be required to request content information about specific modules, it shouldn't be up to one magical function to return everything. Splitting it means that we would need to create externallib functions for the affected modules which is a double bonus because then information can be targeted specifically anyway when required. This also avoids the need for hacks like in mod/page/lib.php.
            Now I know that this is highly desired to get in but honestly I think it needs to be thought about otherwise I fear we will just be duplicating our code in the future and causing unfortold headaches. If this is still going to go in then I must ask that at least we rename the callback to something that relates to what the function is doing. Not something that could be mistake to return content.

          Notes from code:

          1. course/externallib.php
            1. Please document the options, likely in the phpdocs of the get_course_contents method and probably useful in the options.name param description in get_course_contents_parameters
            2. The exception thrown if the course format is not found would be totally extraneous to the average user. That exception should be have a generic localised description and a non-localised message to the effect of that string passed in the debug information.
            3. Presently before accessing a course in get_course_contents we check whether the user can update the course OR the course is visible OR they can view hidden courses. Surely this requires more checks. I would think that this section of code needs to jump through the same hoops that course/view.php does, most importantly that he user can view/access the course.
            4. $sectionvalues['name'] should be retrieved through course/lib.php::get_section_name for consistency.
            5. $sectionvalues['summary'] should this not be formatted properly before returning it. Certainly as an client I would expect to get the summary in its formatted state.
            6. $sectionvalues['summary format'] is there any need for this to be returned? I can't think why it would be useful, especially if summary is always formatted in which case things will always be formatted as HTML.
            7. $cm->uservisible checks the users ability to view a module and includes checks both visibility (+viewhiddenactivities) and group access so there is no need to check viewhiddeactivities.
            8. $cm->name should be formatted before setting to $module['modname'] for return again assuming we deliver things in their final format rather than raw.
            9. $module['description'] code is flawed, its using !empty($module['description']) which could never have been set. My -1 to send that back if it is meant to be there.
            10. $module['visible'] should always be set because any users who can not see visible courses should not have got that far.
            11. There's something about the availability code that I don't like although I haven't focused on it yet. I will review this further.
          2. webservice/pluginfile.php
            1. The SQL statement to get the service can be improved, there doesn't appear to be any need to join to external_tokens as you already have the token. Also noting that you should check that result of that query to make sure it does return a service or use MUST_EXIST.
            2. Before calling get_auth_plugin($user->auth) you need to default user->auth to manual if it is empty. I also need to further review the check for auth expiration as I'm not familiar with
            3. Why is download being forced here, shouldn't that still depend on the file being served? What if the someone writes a web client and outputs the result to the browser it will force them to download otherwise inline content right?
            4. There is also another situation whereby if someone requests a users profile picture through webservice/pluginfile.php they get redirected to the default profile pic in the pix directory. I think this ties in with the above and we need to deal with that force download thing.
          3. lib/filelib.php: Whats the desired outcome if the file can't be served? For instance if someone fails a require_login call.

          I've been pouring over this all afternoon now. I'll give you guys a chance to respond and then we'll continue from there.
          Please note there are a couple of things I need to investigate still, two noted above, and I need to wrap my head around the implications of file serving like this.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Interesting stuff…. not yet sorry a few technical problems and a certainly I think the approach in regards to the new callback needs to be discussed. About the approach: I really really dislike the new something_export_contents. Personally I think the approach is wrong, I think that get_course_contents should not return that information instead the client should be required to request content information about specific modules, it shouldn't be up to one magical function to return everything. Splitting it means that we would need to create externallib functions for the affected modules which is a double bonus because then information can be targeted specifically anyway when required. This also avoids the need for hacks like in mod/page/lib.php. Now I know that this is highly desired to get in but honestly I think it needs to be thought about otherwise I fear we will just be duplicating our code in the future and causing unfortold headaches. If this is still going to go in then I must ask that at least we rename the callback to something that relates to what the function is doing. Not something that could be mistake to return content. Notes from code: course/externallib.php Please document the options, likely in the phpdocs of the get_course_contents method and probably useful in the options.name param description in get_course_contents_parameters The exception thrown if the course format is not found would be totally extraneous to the average user. That exception should be have a generic localised description and a non-localised message to the effect of that string passed in the debug information. Presently before accessing a course in get_course_contents we check whether the user can update the course OR the course is visible OR they can view hidden courses. Surely this requires more checks. I would think that this section of code needs to jump through the same hoops that course/view.php does, most importantly that he user can view/access the course. $sectionvalues ['name'] should be retrieved through course/lib.php::get_section_name for consistency. $sectionvalues ['summary'] should this not be formatted properly before returning it. Certainly as an client I would expect to get the summary in its formatted state. $sectionvalues ['summary format'] is there any need for this to be returned? I can't think why it would be useful, especially if summary is always formatted in which case things will always be formatted as HTML. $cm->uservisible checks the users ability to view a module and includes checks both visibility (+viewhiddenactivities) and group access so there is no need to check viewhiddeactivities. $cm->name should be formatted before setting to $module ['modname'] for return again assuming we deliver things in their final format rather than raw. $module ['description'] code is flawed, its using !empty($module ['description'] ) which could never have been set. My -1 to send that back if it is meant to be there. $module ['visible'] should always be set because any users who can not see visible courses should not have got that far. There's something about the availability code that I don't like although I haven't focused on it yet. I will review this further. webservice/pluginfile.php The SQL statement to get the service can be improved, there doesn't appear to be any need to join to external_tokens as you already have the token. Also noting that you should check that result of that query to make sure it does return a service or use MUST_EXIST. Before calling get_auth_plugin($user->auth) you need to default user->auth to manual if it is empty. I also need to further review the check for auth expiration as I'm not familiar with Why is download being forced here, shouldn't that still depend on the file being served? What if the someone writes a web client and outputs the result to the browser it will force them to download otherwise inline content right? There is also another situation whereby if someone requests a users profile picture through webservice/pluginfile.php they get redirected to the default profile pic in the pix directory. I think this ties in with the above and we need to deal with that force download thing. lib/filelib.php: Whats the desired outcome if the file can't be served? For instance if someone fails a require_login call. I've been pouring over this all afternoon now. I'll give you guys a chance to respond and then we'll continue from there. Please note there are a couple of things I need to investigate still, two noted above, and I need to wrap my head around the implications of file serving like this. Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          Dongsheng needs to address most of this, but I can address a couple points straight away:

          Approach: DS can confirm this but I think this was because of mobile performance. We need to get the whole course outline as a structured tree in terms of files, to avoid hundreds of hits on the server from a phone. I agree though that having the hardcoded webservice URL in there isn't awesome.

          One solution is to get get_course_contents() to call effectively the same functions in externallib.php rather than lib.php, if they were defined there as fully standalone web service functions. It's the external interface to each module after all.

          What I really wanted was to re-use the portfolio system but it looks like DS didn't implement it that way. So the other solution (my preferred, I think) would be simply to extend the new function in lib.php a little to take out the "webservice" URL hardcoding (by adding some other parameter like $fileroot, and make it a generic low-level function that can be used by Web services AND portfolio later.

          2.3 That's for security and if it isn't already, should be documented as comments in the file. This script is for sending files efficiently to web service clients only. It should never serve files to be displayed directly (to avoid XSS). There is /pluginfile.php for direct web serving.

          Show
          Martin Dougiamas added a comment - Dongsheng needs to address most of this, but I can address a couple points straight away: Approach: DS can confirm this but I think this was because of mobile performance. We need to get the whole course outline as a structured tree in terms of files, to avoid hundreds of hits on the server from a phone. I agree though that having the hardcoded webservice URL in there isn't awesome. One solution is to get get_course_contents() to call effectively the same functions in externallib.php rather than lib.php, if they were defined there as fully standalone web service functions. It's the external interface to each module after all. What I really wanted was to re-use the portfolio system but it looks like DS didn't implement it that way. So the other solution (my preferred, I think) would be simply to extend the new function in lib.php a little to take out the "webservice" URL hardcoding (by adding some other parameter like $fileroot, and make it a generic low-level function that can be used by Web services AND portfolio later. 2.3 That's for security and if it isn't already, should be documented as comments in the file. This script is for sending files efficiently to web service clients only. It should never serve files to be displayed directly (to avoid XSS). There is /pluginfile.php for direct web serving.
          Hide
          Sam Hemelryk added a comment - - edited

          My apologies I've just been looking at the auth code and my 2.2 is not correct, there is no need to check $user->auth

          Show
          Sam Hemelryk added a comment - - edited My apologies I've just been looking at the auth code and my 2.2 is not correct, there is no need to check $user->auth
          Hide
          Dongsheng Cai added a comment -

          Thanks for reviewing Sam, I will fix the small problem you found this afternoon

          Show
          Dongsheng Cai added a comment - Thanks for reviewing Sam, I will fix the small problem you found this afternoon
          Hide
          Jérôme Mouneyrac added a comment -

          Nice work and nice reviews. It's impressive.

          Just a note about $options that is not used. We could use it to only ask for the 'forums' in one course. For the moment we just need to document it as 'options - not used yet - will be used to specify a module type in a later version'

          As we could use $options to tweak this function later, I'd vote for Martin's second solution to make the COMPONENT_export_contents(...) more "lib function"-like.

          About the name, it's still confusing for me too But we could not find a better one.

          Show
          Jérôme Mouneyrac added a comment - Nice work and nice reviews. It's impressive. Just a note about $options that is not used. We could use it to only ask for the 'forums' in one course. For the moment we just need to document it as 'options - not used yet - will be used to specify a module type in a later version' As we could use $options to tweak this function later, I'd vote for Martin's second solution to make the COMPONENT_export_contents(...) more "lib function"-like. About the name, it's still confusing for me too But we could not find a better one.
          Hide
          Martin Dougiamas added a comment -

          Dongsheng I just noticed that page_export_contents is actually sending the content as a string. Which is completely different to all the other modules and if a page has a lot of them could result in a massive download.

          This seems incorrect, the content should be served as a file URL exactly like everything else.

          What I think should be happening is that it returns a pluginfile.php link, and then that will call page_pluginfile() in page/lib.php. That function may have to be modified to recognise different arguments so it can generate and serve a HTML file from the content.

          Show
          Martin Dougiamas added a comment - Dongsheng I just noticed that page_export_contents is actually sending the content as a string. Which is completely different to all the other modules and if a page has a lot of them could result in a massive download. This seems incorrect, the content should be served as a file URL exactly like everything else. What I think should be happening is that it returns a pluginfile.php link, and then that will call page_pluginfile() in page/lib.php. That function may have to be modified to recognise different arguments so it can generate and serve a HTML file from the content.
          Hide
          Dongsheng Cai added a comment -

          Martin, the existing page_pluginfile is for serving page contents, images, audio etc, the file area named `content`, I create a new one `pagecontent`?

          Show
          Dongsheng Cai added a comment - Martin, the existing page_pluginfile is for serving page contents, images, audio etc, the file area named `content`, I create a new one `pagecontent`?
          Hide
          Dongsheng Cai added a comment - - edited

          Hi all, I asked Jerome to peer review, this is a list of things I changed:

          1.1 options is not in used yet, I added comment in phpdocs and web service description to indicate this.
          1.2 tell user cannot get course contents and added error to debug info
          1.3 *** added section visible check, is there more I missed?
          1.4 done
          1.5 done
          1.6 removed ,but please note, other course related web service has summaryformat field, we better fix them too
          1.7 removed
          1.8 formated
          1.9 fixed
          1.10 fixed

          2.1 the service is associated with token, I need to join the token table to find the related service
          2.3 2.4 Martin's comment explained these

          Other things:

          1. page module now use page_pluginfile to export page content (as a real file), there is a problem, when page contains embedded images, we need to convert %%PLUGINFILE%% to real url, but xxx_pluginfile don't know is from webservice/pluginfile.php or /pluginfile.php, to properly fix this we need somehow inform xxx_pluginfile its source, but this requires change file api... I discussed with Martin, we added a norewrite arg after in url to prevent rewrite urls in page content, it's not pretty solution for this issue
          2. Regarding the design of this web service, I guess too late to change it, the original doc created on August, and we didn't get many comments on it. Then Jerome started to work on it, and the mobile app is based on it, it's quite a lot re-work if we choose another way to do this.
          Show
          Dongsheng Cai added a comment - - edited Hi all, I asked Jerome to peer review, this is a list of things I changed: 1.1 options is not in used yet, I added comment in phpdocs and web service description to indicate this. 1.2 tell user cannot get course contents and added error to debug info 1.3 *** added section visible check, is there more I missed? 1.4 done 1.5 done 1.6 removed ,but please note, other course related web service has summaryformat field, we better fix them too 1.7 removed 1.8 formated 1.9 fixed 1.10 fixed 2.1 the service is associated with token, I need to join the token table to find the related service 2.3 2.4 Martin's comment explained these Other things: page module now use page_pluginfile to export page content (as a real file), there is a problem, when page contains embedded images, we need to convert %%PLUGINFILE%% to real url, but xxx_pluginfile don't know is from webservice/pluginfile.php or /pluginfile.php, to properly fix this we need somehow inform xxx_pluginfile its source, but this requires change file api... I discussed with Martin, we added a norewrite arg after in url to prevent rewrite urls in page content, it's not pretty solution for this issue Regarding the design of this web service, I guess too late to change it, the original doc created on August, and we didn't get many comments on it. Then Jerome started to work on it, and the mobile app is based on it, it's quite a lot re-work if we choose another way to do this.
          Hide
          Jérôme Mouneyrac added a comment -

          Went through all the points, it looks good. Sam can continue his massive review

          Show
          Jérôme Mouneyrac added a comment - Went through all the points, it looks good. Sam can continue his massive review
          Hide
          Martin Dougiamas added a comment -

          OK, I've been thinking about this more, Dongsheng.

          I think with the page_pluginfile() that we can safely assume that the main filearea will NEVER include an index.html file. It simply doesn't make sense in the module and filepicker won't allow it.

          So let's just use that filename to get the file content.

          So page_export_content() returns a list of "index.html", "fred.jpg" and "wilma.jpg". This will only be returned to things like portfolio and web service clients of course. In these cases, we don't need any full URL for embedded media at all. In fact, we KNOW that the embedded media is relative to this GENERATED file in a very particular way.

          When the file index.html is requested in page_pluginfile, we generate the file on the fly and rewrite the links to be proper relative links like <img src="wilma.jpg" />

          This means no fake filearea, no bad URLs. All we have is a fake file, but then that is exactly what mod/page is.

          I think this is very clean and simple. And mobile clients only need to download the page (and associated media) when they need it.

          Show
          Martin Dougiamas added a comment - OK, I've been thinking about this more, Dongsheng. I think with the page_pluginfile() that we can safely assume that the main filearea will NEVER include an index.html file. It simply doesn't make sense in the module and filepicker won't allow it. So let's just use that filename to get the file content. So page_export_content() returns a list of "index.html", "fred.jpg" and "wilma.jpg". This will only be returned to things like portfolio and web service clients of course. In these cases, we don't need any full URL for embedded media at all. In fact, we KNOW that the embedded media is relative to this GENERATED file in a very particular way. When the file index.html is requested in page_pluginfile, we generate the file on the fly and rewrite the links to be proper relative links like <img src="wilma.jpg" /> This means no fake filearea, no bad URLs. All we have is a fake file, but then that is exactly what mod/page is. I think this is very clean and simple. And mobile clients only need to download the page (and associated media) when they need it.
          Hide
          Dongsheng Cai added a comment -

          Martin

          yes, this should be ok, the matter is about how we present page html, so your suggestion is removing %%PLUGINFILE%% from page content, then client will use relative path to display resources, this requires minimum changes in code.

          What I gonna do:
          1. instead of rewriting URLs in page html, we simply remove all %%PLUGINFILE%%
          2. Just to be sure, and url for page html will be:
          http://moodlesite.local/webservice/pluginfile.php/12/mod_page/content/index.html

          Right? This should be easy, I can finish this tonight

          Show
          Dongsheng Cai added a comment - Martin yes, this should be ok, the matter is about how we present page html, so your suggestion is removing %%PLUGINFILE%% from page content, then client will use relative path to display resources, this requires minimum changes in code. What I gonna do: 1. instead of rewriting URLs in page html, we simply remove all %%PLUGINFILE%% 2. Just to be sure, and url for page html will be: http://moodlesite.local/webservice/pluginfile.php/12/mod_page/content/index.html Right? This should be easy, I can finish this tonight
          Hide
          Martin Dougiamas added a comment -

          Yep, great!

          Show
          Martin Dougiamas added a comment - Yep, great!
          Hide
          Martin Dougiamas added a comment -

          Looks good to me now, DS! Apu can you take this?

          Show
          Martin Dougiamas added a comment - Looks good to me now, DS! Apu can you take this?
          Hide
          Jérôme Mouneyrac added a comment -

          Just quick fix you can pick there: https://github.com/mouneyrac/moodle/tree/wip-MDL-30459

          Show
          Jérôme Mouneyrac added a comment - Just quick fix you can pick there: https://github.com/mouneyrac/moodle/tree/wip-MDL-30459
          Hide
          Dongsheng Cai added a comment -

          Thanks Jerome

          I cherry-picked your changes, added on top of my changes.

          Show
          Dongsheng Cai added a comment - Thanks Jerome I cherry-picked your changes, added on top of my changes.
          Hide
          Aparup Banerjee added a comment - - edited

          reviewing - ping if any more changes.

          Show
          Aparup Banerjee added a comment - - edited reviewing - ping if any more changes.
          Hide
          Aparup Banerjee added a comment -

          Here's a review/opinion/braindump, i hope it helps

          webservice/upload.php :
          line 91 - do we want to deactivate/delete the token if the password is expired for additional security measures. - for future consideration.

          mod/page/lib.php:
          line 383 - will inter-mod_page links break? is that even possible? <noidea/>

          webservice/pluginfile.php :
          line 45 (2.1) : Each token is linked to only one web service currently (no idea why but thats what security keys page shows too). I think what Sam means is

          {external_tokens} is many-1 association with {external_services}. We already have the entire {external_tokens}

          for a single token from the validation of token check so for performance :

           // Obtain token record
          if (!$token = $DB->get_record('external_tokens', array('token'=>$token))) {
              print_error('invalidtoken', 'webservice');
          }
          
           //retrieve web service record
          $servicesql = 'SELECT s.*
                           FROM {external_services} s
                          WHERE s.id = ?
                                AND t.token = ? AND t.userid = ? AND s.enabled = 1';
          $service = $DB->get_record_sql($servicesql, array($token->externalserviceid, $token->token, $token->userid), MUST_EXIST);
          

          hm, makes sense ^^ ?

          generally:
          edge case: *_export_contents() serves fileurl without https. what about a moodle site only accessible via SSL alone? workaround is to modify wwwroot to use https. DS mentioned a possible WS-https-only configuration flag to detect this case.

          minor issues,
          webservice/simpletest/testwebservice.php:
          line 265 , 268 - Lets have a MDL issue next to the TODOs here.
          line 270 - Jerome's varlog() in comment

          with that, I don't see any major issues blocking this going into integration and upstream currently (or on monday before Eloy rolls master). Since we're in a rush to have this in before 2.2, having some side issues would be great to later tackle undealt with issues.

          Show
          Aparup Banerjee added a comment - Here's a review/opinion/braindump, i hope it helps webservice/upload.php : line 91 - do we want to deactivate/delete the token if the password is expired for additional security measures. - for future consideration. mod/page/lib.php: line 383 - will inter-mod_page links break? is that even possible? <noidea/> webservice/pluginfile.php : line 45 (2.1) : Each token is linked to only one web service currently (no idea why but thats what security keys page shows too). I think what Sam means is {external_tokens} is many-1 association with {external_services}. We already have the entire {external_tokens} for a single token from the validation of token check so for performance : // Obtain token record if (!$token = $DB->get_record('external_tokens', array('token'=>$token))) { print_error('invalidtoken', 'webservice'); } //retrieve web service record $servicesql = 'SELECT s.* FROM {external_services} s WHERE s.id = ? AND t.token = ? AND t.userid = ? AND s.enabled = 1'; $service = $DB->get_record_sql($servicesql, array($token->externalserviceid, $token->token, $token->userid), MUST_EXIST); hm, makes sense ^^ ? generally: edge case: *_export_contents() serves fileurl without https. what about a moodle site only accessible via SSL alone? workaround is to modify wwwroot to use https. DS mentioned a possible WS-https-only configuration flag to detect this case. minor issues, webservice/simpletest/testwebservice.php: line 265 , 268 - Lets have a MDL issue next to the TODOs here. line 270 - Jerome's varlog() in comment with that, I don't see any major issues blocking this going into integration and upstream currently (or on monday before Eloy rolls master). Since we're in a rush to have this in before 2.2, having some side issues would be great to later tackle undealt with issues.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          About the points I can talk:

          webservice/upload.php
          Don't worry, in login/token.php and webservice layer do the same, so it's consisten. In my opinion, password and token are not related to each other, so it's good like that for me

          webservice/pluginfile.php
          I think this one is enough:
          $servicesql = 'SELECT s.* FROM

          {external_services}

          s WHERE s.id = ? AND s.enabled = 1';
          $service = $DB->get_record_sql($servicesql, array($token->externalserviceid), MUST_EXIST);
          However I just noticed (sorry, I should have picked it up before as I did the fixes in login/token.php), we missed few checks:

          • check if the service is restricted, and in this case check if the user is in the restricted list.
          • check if the service has ip restriction / valid until date (we only check the ones for the token)
          • check if the service has a required capability.

          *_export_contents()
          I don't remember if Moodle switch automatically in HTTPS when the setting is ON. if not, then it is quite a big issue.

          webservice/simpletest/testwebservice.php
          The second TODO it's more for a developer advice using this function for debugging Not really a TODO. You can replace //varlog byt //error_log(print_r(...,true));

          Show
          Jérôme Mouneyrac added a comment - - edited About the points I can talk: webservice/upload.php Don't worry, in login/token.php and webservice layer do the same, so it's consisten. In my opinion, password and token are not related to each other, so it's good like that for me webservice/pluginfile.php I think this one is enough: $servicesql = 'SELECT s.* FROM {external_services} s WHERE s.id = ? AND s.enabled = 1'; $service = $DB->get_record_sql($servicesql, array($token->externalserviceid), MUST_EXIST); However I just noticed (sorry, I should have picked it up before as I did the fixes in login/token.php), we missed few checks: check if the service is restricted, and in this case check if the user is in the restricted list. check if the service has ip restriction / valid until date (we only check the ones for the token) check if the service has a required capability. *_export_contents() I don't remember if Moodle switch automatically in HTTPS when the setting is ON. if not, then it is quite a big issue. webservice/simpletest/testwebservice.php The second TODO it's more for a developer advice using this function for debugging Not really a TODO. You can replace //varlog byt //error_log(print_r(...,true));
          Hide
          Martin Dougiamas added a comment -

          Jerome, can you fix all that and then can we land it today Apu?

          Show
          Martin Dougiamas added a comment - Jerome, can you fix all that and then can we land it today Apu?
          Hide
          Aparup Banerjee added a comment -

          Yup, this should likely go in today. Reopened for Jerome to work on - go go go

          Show
          Aparup Banerjee added a comment - Yup, this should likely go in today. Reopened for Jerome to work on - go go go
          Hide
          Jérôme Mouneyrac added a comment -

          I added a new commit for the missing authentication checks (I didn't test all of them). Note that I made the download script an AJAX script. I thought it would be good to be like the upload script. I think it's easier for the web service client to deal with a json error than the Moodle exception html code. If anyone has an opinion on how to return error for download scripts, please let me know. I'll be happy to improve this

          For history, previous fix was:
          git://github.com/dongsheng/moodle.git
          https://github.com/dongsheng/moodle/compare/master...dev_MDL-28646_course_contents_master

          Show
          Jérôme Mouneyrac added a comment - I added a new commit for the missing authentication checks (I didn't test all of them). Note that I made the download script an AJAX script. I thought it would be good to be like the upload script. I think it's easier for the web service client to deal with a json error than the Moodle exception html code. If anyone has an opinion on how to return error for download scripts, please let me know. I'll be happy to improve this For history, previous fix was: git://github.com/dongsheng/moodle.git https://github.com/dongsheng/moodle/compare/master...dev_MDL-28646_course_contents_master
          Hide
          Aparup Banerjee added a comment -

          looking good on final approach..

          thanks guys this has been integrated!

          spotted an issue with 'read' type of the function but requiring 'update' sort of cap, but this had already been previously noted and have issues (MDL-29962 , Thanks Jerome for the quick searches)

          Show
          Aparup Banerjee added a comment - looking good on final approach.. thanks guys this has been integrated! spotted an issue with 'read' type of the function but requiring 'update' sort of cap, but this had already been previously noted and have issues ( MDL-29962 , Thanks Jerome for the quick searches)
          Hide
          Aparup Banerjee added a comment -

          added a version bump

          Show
          Aparup Banerjee added a comment - added a version bump
          Hide
          Aparup Banerjee added a comment - - edited

          and passing too, works for me , ready for (new) QA testing.

          Thanks everyone!

          Show
          Aparup Banerjee added a comment - - edited and passing too, works for me , ready for (new) QA testing. Thanks everyone!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days.

          Thanks for the hard work! Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days. Thanks for the hard work! Closing, ciao
          Hide
          Tim Barker added a comment -

          Added QA test MDLQA-1548.

          Show
          Tim Barker added a comment - Added QA test MDLQA-1548 .

            People

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

              Dates

              • Created:
                Updated:
                Resolved: