MaharaProject / moodle-assignsubmission_mahara

Old, unmaintained Mahara assignment submission plugin for Moodle
https://moodle.org/plugins/view/assignsubmission_mahara
4 stars 10 forks source link

Multiple submissions of the same page to different assignments breaks previous assignments' access #2

Open agwells opened 9 years ago

agwells commented 9 years ago

The current implementation for allowing assignments to be submitted without locking the page/collection, is pretty much a hack. It calls the Mahara method that submits & locks the page. It retrieves and stores the secret URL which is created by this call. Then it immediately calls the Mahara method that releases the page.

As a bug in the original Mahara code, the secret URL created for the lock is not deleted when the lock is released. So this quick lock-release creates and stores the secret URL, leaving the page unlocked afterwards.

The problem is, the behavior it relies on was a bug. The secret URL really should be deleted once the page is released. And it gets deleted once another lock is created on the page, which means that a page will, to the user, inexplicably stop being visible at some point, if it's submitted to another assignment.

What we really need I guess, is some kind of API for creating and storing MNet secret URLs, without locking the page. Or maybe drop the secret URL angle entirely, and use MNet itself to verify that the user is allowed to see the page?

kabalin commented 9 years ago

I think using MNET only for page access is the best option that would allow to void making secret URL mechanism more complicated.

agwells commented 9 years ago

Yeah, it seems like there must be some way to do that through MNet.

The way the system currently works, is we create a hidden secreturl in Mahara, and then we make the link in Moodle be to a URL that has an "mt=" param on the end. This "mt" acts the same as any other secreturl (which normally is a "t=" param) except that it only works for people who are logged in via MNet. So, it gives you access to the page, and it also sets a flag in a cookie to ensure you continue to have access to that page until you log out (so that you can, for instance, look at an artefact in the page then navigate back to it, and we don't have to rewrite every link everywhere to have that "mt" param).

If we were going to do this without the "mt" secreturl token... I think it would have to involve telling Mahara that it needs to call back to Moodle and make sure the user is allowed to view the page. Something like...

  1. Moodle creates a link to the page, with a param on the URL that says "accessing this via the assignment submission plugin"
  2. User clicks on the link, roams to Mahara.
  3. Mahara loads up the Page, sees the "Accessing this via assignment submission plugin" param
  4. Mahara does an MNet function call back to Moodle, passing the view ID & the user
  5. Moodle verifies that user should be allowed to view that page because it was submitted via MNet.
  6. Mahara receives that verification, and sets the access cookie as per usual

This would move all the Moodle access control stuff out of Mahara and back into Moodle, which would save us a lot of annoying syncing.

agwells commented 9 years ago

Would require:

  1. Patching Mahara
  2. Asking the Moodle admin to allow moodle to Publish an MNet service for the assignment submission plugin instead of just Subscribing to one.
upats commented 9 years ago

Sounds like a good plan to me. When we developed the plugin we needed a very quick fix for having the option to not lock the portfolio on submission... this "bug" was discovered so we used it :)

It would be great to have this feature fully implemented.

agwells commented 9 years ago

I realized there's a problem with the cookie angle I mentioned above.

So, under the current system, after you access a submitted page via its "view/view.php?mt=___" URL, the part we store in the "mviewaccess" cookie is the "mt=" token itself. So we'll have to change that system if we're no longer generating a secreturl-ish access token.

I think we should be fine just storing the list of allowed viewids in $SESSION instead. As far as I can tell, the reason we have that separate "mviewaccess" cookie is because it's copying the standard secreturl system, which stores each accessed secreturl token in a "viewaccess" cookie. But the reason we use a separate cookie for secreturls is to make sure the list survives if you go from logged out to logged in. That's not a concern for Moodle-submitted pages, because you have to be logged in to see them in the first place.

agwells commented 9 years ago

Hi upats,

Thanks for the feedback. I figured that was the situation. :)

It's not the best implementation, but on the other hand the ability to not lock pages is a much-requested feature, so kudos to you for making it happen.

Cheers, Aaron

samchaffee commented 9 years ago

Hi all. I'm reviewing this plugin for inclusion as an update to our codebase and I'm trying to grasp the full implications of this issue.

I'm very familiar with Moodle, but not terribly familiar with Mahara. My understanding from looking at code in both places is that the main consequence is that the secreturl, which Moodle stores for viewing the submission will be "deleted once another lock is created on the page, which means that a page will, to the user, inexplicably stop being visible at some point, if it's submitted to another assignment."

I'm really more concerned about security, which is mentioned in the assignsubmission plugin. But from reading the comments in this issue and reviewing the code it seems that it does not create a problem for pages submitted via Moodle.

If anyone has a moment to check my understanding of the issue, I'd greatly appreciate it.

kabalin commented 9 years ago

@sam-moodle If I recall correctly, there is no way to submit the same page to more than one assignment at the time.

agwells commented 9 years ago

Hi @kabalin,

You can't submit a locked Mahara page to another Moodle assignment. But the Portland U code makes it so that the Mahara page is not permanently locked anymore. If you use the feedback plugin, it gets unlocked after grading. And if you use the option (which this bug is about) to not lock the page at all, there's nothing to stop you from submitting it to another assignment at the same time.

Hi @sam-moodle,

Yeah, I don't see any major security ramifications in this issue.

As mentioned, we use a "secret URL" to let the teachers view the submitted Mahara pages. This is a URL with a token on the end of it, and anyone who visits that URL and is logged into Mahara via MNet, can view the page it points at regardless of the page's other access settings.

So access control isn't totally locked down. A teacher could copy that URL and send it to another MNet user, who would be able to view the page whether they're a teacher or not. Or someone could get the URL out of the browser history of the teacher's computer.

If we implemented the "check back with Moodle via MNet" system I described a few comments back, that would eliminate the problem.

Cheers, Aaron

samchaffee commented 9 years ago

Thanks for the responses, @kabalin and @agwells. I really appreciate the feedback.

upats commented 9 years ago

Hi all,

Is there any way for us to help out with this issue? We are running into a serious problem with one of our departments (mostly political stuff) because of this particular shortcoming which may seriously hurt the perception and continued usage of Mahara on our campus. The short of it is the thread has already worn pretty thin and this particular issue might be enough to break it.

If there's anything our university can do to help out, please let me know. We don't have a group of plugin developers, so we couldn't directly contribute to re-coding anything, but we may have funds available to support the development of the plugin.

Thanks, Tony

agwells commented 9 years ago

Hi Tony,

Come to think of it, I've been sitting on this pull request from Ruslan since June, which I think might take care of this issue: https://github.com/MaharaProject/moodle-assignsubmission_mahara/pull/12 . It's been on my "to do" list, but I've been busy with the whole front-end overhaul in Mahara 15.10 so I haven't had community-support time available for it.

So if you could test that out, that would be very helpful.

If you've got some funding available, you can email Kristina Hoeppner (kristina@catalyst.net.nz) to see about contracting us to work on this immediately. Otherwise, it'll probably have to wait until after the 15.10.0 release, which is looking like the end of October.

Cheers, Aaron

upats commented 9 years ago

Thanks for the suggestion, Aaron. I know you're busy which is why I was wondering if we could help in any way :)

I just tested out the #12 pullrequest and it does not fix the issue, unfortunately. I will speak with my boss over here to see if we can work with Kristina in some way.

-Tony

agwells commented 7 years ago

I just had a discussion with @anitsirk about this, because it came up again for one of our clients.

The current status of this is:

  1. The assignment submission plugin (this plugin) still uses a hidden secreturl to control access. And this causes permissions issues when you submit the same Mahara page/collection to multiple Moodle assignments. Specifically, each page/collection can only have one secreturl of this type, so a second submission wipes out the view access via the first submission.

  2. We've written & merged code on the Mahara side, to use a new MNet service (on the Moodle side) to check for view permissions, instead of using hidden secreturls. This can fix the problem.

  3. @hughdavenport Wrote up a patch (see issue #19) that updates this assignment submission plugin to publish the new MNet service.

  4. But there's a bug in Moodle that's stopping us, MDL-52172. The problem is that the MNet system doesn't allow subplugins (like "assignment/submission") to publish MNet services. Due to a deprecation in MNet, and people changing jobs, the patch for that bug never got merged.

So, currently there are three options to resolve this issue:

  1. We get MDL-52172 merged into Moodle finally, and we update this plugin with the patch from issue #19, so that this plugin declares the necessary Mnet functions for the new permissions system.

  2. We instead move the necessary MNet functions into a new "local" plugin, and go back to this being a multi-plugin system. @hughdavenport originally wrote it this way but I didn't want to go back to having multiple plugins. But since MDL-52172 has languished for so long, this is probably the quickest fix. It may require patching Mahara to change the name of the Mnet function to call.

  3. We move forward quickly with replacing MNet in this plugin with some other system (webservices or LTI). But we've been talking about that for near on 10 years now, so I wouldn't want to wait on that.

Individual affected systems can work around this by manually patching their systems with the patch from issue #19, and the patch from MDL-52172.

anitsirk commented 7 years ago

Ideally, LTI for assignment submission won't take many years. We can now do SSO via LTI (from Mahara 17.04 onwards) and are actively working on the proposal for LTI in the assignment process and estimating the required effort for it.

https://wiki.mahara.org/wiki/Developer_Area/Specifications_in_Development/MNet_replacement/LTI_implementation

agwells commented 7 years ago

Updating the title of this issue to be more descriptive of the actual bug it causes. (Title stolen from duplicate issue report #14 :) )