Commit 3a8e3ed1 authored by Kazuhiko Shiozaki's avatar Kazuhiko Shiozaki

discoverable: make discoverMetadata() user independent.

parent 608a34f6
...@@ -31,6 +31,7 @@ from AccessControl import ClassSecurityInfo, getSecurityManager ...@@ -31,6 +31,7 @@ from AccessControl import ClassSecurityInfo, getSecurityManager
from Products.ERP5Type.Globals import InitializeClass from Products.ERP5Type.Globals import InitializeClass
from ZODB.POSException import ConflictError from ZODB.POSException import ConflictError
from Products.ERP5Type import Permissions from Products.ERP5Type import Permissions
from Products.ERP5Type.UnrestrictedMethod import UnrestrictedMethod
from Products.ERP5Type.Utils import convertToUpperCase from Products.ERP5Type.Utils import convertToUpperCase
from Products.ERP5.mixin.cached_convertable import CachedConvertableMixin from Products.ERP5.mixin.cached_convertable import CachedConvertableMixin
import os import os
...@@ -120,6 +121,7 @@ class DiscoverableMixin(CachedConvertableMixin): ...@@ -120,6 +121,7 @@ class DiscoverableMixin(CachedConvertableMixin):
### Metadata disovery and ingestion methods ### Metadata disovery and ingestion methods
security.declareProtected(Permissions.ModifyPortalContent, security.declareProtected(Permissions.ModifyPortalContent,
'discoverMetadata') 'discoverMetadata')
@UnrestrictedMethod
def discoverMetadata(self, filename=None, user_login=None, def discoverMetadata(self, filename=None, user_login=None,
input_parameter_dict=None): input_parameter_dict=None):
""" """
......
  • A bit strange that we keep this in the API then:

        user_login - this is a login string of a person; can be None if the user is
                     currently logged in, then we'll get him from session
  • To be clear, this change is not wrong, in another project I worked around by using proxy roles in Document_getPropertyDictFromContent. The current state of DMS is not really good in my opinion

  • I'm still not sure this change is correct, yesterday we had an incident on the forum:

    • there is a document with reference R in shared state (ie. not modifiable by anybody). It has a filename that follows the DMS regular expressions, so the reference can be extracted from the filename.
    • user downloads this document, makes changes and posts on forum, attaching the modified document to the forum post
    • the metadata discovery of this attached document uses mergeRevision and overwrites in place the existing document. I believe this is not the expected behavior, because here we have an assertion that user has the permission to edit the existing document, which would have prevented this issue (instead of overwriting a "read only" existing document, we would only have an error).

    @kazuhiko I am thinking of adjusting this patch so that the final mergeRevision is done without @UnrestrictedMethod, do you see a problem with this idea ? I'll start writing a test to reproduce such an issue of "user overwrites a document that they are not allowed to modify", but please let me know soon if you think I am misunderstanding something.

    /cc @xiaowu.zhang

  • @jerome If such merge is not expected (in certain use cases), what should be the ideal behaviour ? A new document having the same reference is created and shared, and the old one becomes archived ?

    Anyway I feel that forum should use Embedded File / Embedded Image under Discussion Post document...

  • @jerome If such merge is not expected (in certain use cases), what should be the ideal behaviour ? A new document having the same reference is created and shared, and the old one becomes archived ?

    I don't know exactly, but I feel at this level it should just raise Unauthorized. At a higher level, there should be a constraint or something user friendly telling the user "Another document already exists with the same coordinates but you are not allowed to update it".

    Anyway I feel that forum should use Embedded File / Embedded Image under Discussion Post document...

    Yes, but using for example Spreadsheet portal type for spreadsheet is good because it gives an online preview, with Embedded File we can just download. I think the general problem with attachment in forum is that just attaching a document should not trigger all the DMS magic. I feel this can be achieved by auto allocating references based on the forum's Discussion Thread reference and a counter of existing documents (or the date, or anything that can be unique).

    does this make sense ? I am thinking of implementing the first part for now, raising seems better than silently erasing another document the user is not allowed to edit.

  • and thanks for feedback I forgot to say 😄

  • I feel this can be achieved by auto allocating references based on the forum's Discussion Thread reference and a counter of existing documents (or the date, or anything that can be unique).

    Yes, something like that. For example "(Discussion Thread reference)-(Discussion Post ID)-(# of attachments)".

  • mentioned in merge request !1846 (merged)

    Toggle commit list
  • I submitted !1846 (merged) which should fix the problem I experienced. It does not change that discoverMetadata uses mergeRevision as unrestricted and I still feel this is not good, but when writing the test I realized there was another place in ContributionTool implementing more or less the same logic of checking if user can update a document in place and updating that document, so I just made the fix here.

  • After adding many tests for !1846 (merged) I had to modify discoverMetadata to be "partially unrestricted": I split in two and do only the first part as unrestricted, so that th "security sensitive parts" remain restricted

Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment