Moodle

Creating new web service tokens create error

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 2.1.2
  • Fix Version/s: 2.1.6, 2.2.3
  • Component/s: Web Services
  • Labels:
  • Testing Instructions:
    Hide

    Add a new web service in the admin, and add all functions to this service. Create a token for a user non admin. Check that the error message is not displayed on the token admin page.

    Show
    Add a new web service in the admin, and add all functions to this service. Create a token for a user non admin. Check that the error message is not displayed on the token admin page.
  • Affected Branches:
    MOODLE_21_STABLE
  • Fixed Branches:
    MOODLE_21_STABLE, MOODLE_22_STABLE
  • Pull from Repository:
  • Pull Master Branch:
  • Pull Master Diff URL:

Description

Noticed the following error while testing MDL-30045, to create new token for a student:

Did you remember to make the first column something unique in your call to get_records? Duplicate value '766' found in column 'id'.

    line 818 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
    line 364 of /webservice/lib.php: call to mysqli_native_moodle_database->get_records_sql()
    line 391 of /webservice/lib.php: call to webservice->get_user_capabilities()
    line 7550 of /lib/adminlib.php: call to webservice->get_missing_capabilities_by_users()
    line 1343 of /lib/adminlib.php: call to admin_setting_managewebservicetokens->output_html()
    line 126 of /admin/settings.php: call to admin_settingpage->output_html()

Issue Links

discovered while testing

Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-30045 Backport 2.2 changes for login/token.php in 2.1

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
has a non-specific relationship to

Task - A task that needs to be completed. Tasks usually refer to work that must be done outside the product. MDL-31253 META - web service improvements for 2.3

  • Critical - Crashes server, loss of data, severe memory leak
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
has been marked as being related by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-32458 Verify all the capabilities specified in services.php definitions are checked in code

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Improvement - An enhancement to an existing Moodle feature. MDL-29962 Informing administrator about "required" capabilities to run a web service function

  • Minor - Minor loss of function where workaround is possible
  • Open - The issue is open and ready for the assignee to start work on it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jérôme Mouneyrac added a comment -

ping for peer review

Show
Jérôme Mouneyrac added a comment - ping for peer review
Hide
Permalink
Rossiani Wijaya added a comment -

Hi Jerome,

This looks good.

However, I'm just wondering if it is necessary to have rc.capability for group by. Removing it from the sql, produces the same output. Maybe this is used for multiDB compability.

Show
Rossiani Wijaya added a comment - Hi Jerome, This looks good. However, I'm just wondering if it is necessary to have rc.capability for group by. Removing it from the sql, produces the same output. Maybe this is used for multiDB compability.
Hide
Permalink
Jérôme Mouneyrac added a comment -

As Rosie, I don't know if rc.id is enough (cross-db compatible). I don't think it is a bad thing to group on both fields as it's what I try to obtain anyway. So I let it like that Submitting for integration.

Show
Jérôme Mouneyrac added a comment - As Rosie, I don't know if rc.id is enough (cross-db compatible). I don't think it is a bad thing to group on both fields as it's what I try to obtain anyway. So I let it like that Submitting for integration.
Hide
Permalink
Jérôme Mouneyrac added a comment - - edited

I let you cherry-pick this one line commit to previous branches (I don't think it make you any more work that you have to do for integration, and it saves me some)

Show
Jérôme Mouneyrac added a comment - - edited I let you cherry-pick this one line commit to previous branches (I don't think it make you any more work that you have to do for integration, and it saves me some)
Hide
Permalink
Dan Poltawski added a comment -

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Show
Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
Hide
Permalink
Dan Poltawski added a comment -

Hi Jerome,

I do not think that this group by SQL is correct - instead it should use DISTINCT.

But more than this, the SQL does not seem to make sense, it is retrieving any capability in any context. So if someone has some capability in a block or a PROHIBIT capabilty then this will be returned. The function document even mentions "(with context)" but the context isn't considered at all.

So I think that the functionality of this function also needs to be considered

Show
Dan Poltawski added a comment - Hi Jerome, I do not think that this group by SQL is correct - instead it should use DISTINCT. But more than this, the SQL does not seem to make sense, it is retrieving any capability in any context. So if someone has some capability in a block or a PROHIBIT capabilty then this will be returned. The function document even mentions "(with context)" but the context isn't considered at all. So I think that the functionality of this function also needs to be considered
Hide
Permalink
Jérôme Mouneyrac added a comment -

Hi Dan, I do understand that the SQL does not make sens. The goal is to deliver a help for admin (to have an idea at what capabilities the admin could be missing - before they were left without any clue at all and it was a nightmare for them without looking at the code). We didn't want to add any more complexity to the services.php mentioning cap/context couple, so we just added a plain capabilities text. I agree we miss some cap sometimes (when they don't have access on the context used by the function but do have the cap on other context). There is definitively big improvement to do in this domain, I'll write another issue for that.

Note there is also an existing issue that will refactor the capability part of services.php.

I tried first with DISTINCT but I gave up on it because I think it was not cross-db if I remember, I'll have a look again.

Show
Jérôme Mouneyrac added a comment - Hi Dan, I do understand that the SQL does not make sens. The goal is to deliver a help for admin (to have an idea at what capabilities the admin could be missing - before they were left without any clue at all and it was a nightmare for them without looking at the code). We didn't want to add any more complexity to the services.php mentioning cap/context couple, so we just added a plain capabilities text. I agree we miss some cap sometimes (when they don't have access on the context used by the function but do have the cap on other context). There is definitively big improvement to do in this domain, I'll write another issue for that. Note there is also an existing issue that will refactor the capability part of services.php. I tried first with DISTINCT but I gave up on it because I think it was not cross-db if I remember, I'll have a look again.
Hide
Permalink
Jérôme Mouneyrac added a comment -

Hi Dan, I linked two issues where you should find more explaination about the 'capabilities' field in service.php. you are welcome to participate to the topic

Show
Jérôme Mouneyrac added a comment - Hi Dan, I linked two issues where you should find more explaination about the 'capabilities' field in service.php. you are welcome to participate to the topic
Hide
Permalink
Jérôme Mouneyrac added a comment -

Hi I did the requested changes, rebased and retested. All good. Thanks for your review Dan. Resubmitting.

Show
Jérôme Mouneyrac added a comment - Hi I did the requested changes, rebased and retested. All good. Thanks for your review Dan. Resubmitting.
Hide
Permalink
Dan Poltawski added a comment -

Thanks Jerome, this has been integrated. Noting that this id a documentation function only

Show
Dan Poltawski added a comment - Thanks Jerome, this has been integrated. Noting that this id a documentation function only
Hide
Permalink
Jérôme Mouneyrac added a comment -

Ah I forgot to change the phpdoc of the function!

Show
Jérôme Mouneyrac added a comment - Ah I forgot to change the phpdoc of the function!
Hide
Permalink
Andrew Davis added a comment -

Stopping testing as I have to go for a few hours. Jerome, when you say to add a new web service do you mean as is described at http://docs.moodle.org/22/en/Using_web_services#Creating_a_service

Show
Andrew Davis added a comment - Stopping testing as I have to go for a few hours. Jerome, when you say to add a new web service do you mean as is described at http://docs.moodle.org/22/en/Using_web_services#Creating_a_service
Hide
Permalink
Jérôme Mouneyrac added a comment -

Yes that's it

Show
Jérôme Mouneyrac added a comment - Yes that's it
Hide
Permalink
Jérôme Mouneyrac added a comment -

Dan I added a commit: https://github.com/mouneyrac/moodle/commit/8a2705e64032dd84b4a871ca47e19c1564d0fe5f
Do you have time to add it?

Show
Jérôme Mouneyrac added a comment - Dan I added a commit: https://github.com/mouneyrac/moodle/commit/8a2705e64032dd84b4a871ca47e19c1564d0fe5f Do you have time to add it?
Hide
Permalink
Jérôme Mouneyrac added a comment - - edited

if no I'll create another issue for that (Andrew you can keep going testing this issue )

Show
Jérôme Mouneyrac added a comment - - edited if no I'll create another issue for that (Andrew you can keep going testing this issue )
Hide
Permalink
Dan Poltawski added a comment -

Hi Jerome, i've added that (to master only)

Show
Dan Poltawski added a comment - Hi Jerome, i've added that (to master only)
Hide
Permalink
Jérôme Mouneyrac added a comment -

thank you Dan

Show
Jérôme Mouneyrac added a comment - thank you Dan
Hide
Permalink
Andrew Davis added a comment -

Works as described. Passing.

Show
Andrew Davis added a comment - Works as described. Passing.
Hide
Permalink
Dan Poltawski added a comment -

Bonza mate!

Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

Hooroo

Show
Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: