Sergiu Dumitriu wrote:
> On 03/30/2010 06:50 AM, Caleb James DeLisle wrote:
>>
>> Sergiu Dumitriu wrote:
>>> On 03/29/2010 06:57 PM, cjdelisle (SVN) wrote:
>>>> Author: cjdelisle
>>>> Date: 2010-03-29 18:57:58 +0200 (Mon, 29 Mar 2010)
>>>> New Revision: 28004
>>>>
>>>> Modified:
>>>>
>>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/Document.java
>>>>
>>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/XWiki.java
>>>> Log:
>>>> XWIKI-5041: Allow script authors to load and save documents in their own
>>>> security context, not the user's
>>> Quick review:
>>>
>>> 1. Although this is not always the case in the current API, methods
>>> should not throw exceptions, since there is no way to catch them in
>>> Velocity, and it's not nice at all to display exceptions and stacktraces
>>> to the user. It would be better to catch all and return a boolean if the
>>> save/delete fails.
>> What should become of the exception?
>> I have seen some very old code which put the exception in the context but I
>> don't see that happening
>> anywhere in the Document or XWiki API.
>
> Yes,exceptions should be placed in the context.
>
>> Is it right to eat the exception and return false?
>
> Well, it's wrong to let the exception pass through. But since this
> creates an API inconsistency, it should be discussed on a broader scope
> (Proposal?).
I noticed some code stores the exception in XWikiContext key "error" however
this is unreachable
to code without PR so it might as well eat the exception.
I see some code throws an exception and some eats it sometimes logging the fact.
Maybe the easiest solution is to put try-catch support into velocity, that way
we don't need to change
old API or create additional inconsistency.
On the other hand, it seems that we (mostly you and I) are pushing velocity way
beyond all reasonable
limits and we might consider making Groovy more secure. Unfortunately Rhino is
licensed GPL/MPL and not
LGPL.
>
>>> 2. I don't quite like the comments; with an empty mind, I wouldn't
>>> understand what "the author of the code calling this function takes
>>> responsibility" actually means. Maybe something like this would be better:
>>>
>>> Save the document under the privileges of the script's author. More
>>> specifically, before saving, access rights are checked for the {...@link
>>> #getContentAuthor content author} of the context document on the target
>>> document, which also is set as the new author of the document.
>>>
>>> The comment for XWiki.getDocumentAsAuthor is quite good.
>>>
>>> Read below for more comments.
>>>
>>>> Modified:
>>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/Document.java
>>>> ===================================================================
>>>> ---
>>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/Document.java
>>>> 2010-03-29 16:57:48 UTC (rev 28003)
>>>> +++
>>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/Document.java
>>>> 2010-03-29 16:57:58 UTC (rev 28004)
>>>> @@ -1823,6 +1823,64 @@
>>>> }
>>>> }
>>>>
>>>> + /**
>>>> + * Save the document, the author (contentAuthor) of the code calling
>>>> this function takes responsibility.
>>>> + * Saves with minorEdit set to false and no edit comment.
>>>> + *
>>>> + * @throws XWikiException if script author is not allowed to save the
>>>> document or if save operation fails.
>>>> + * @see #saveAsAuthor(String, boolean)
>>> 2.3M2 already.
>> Thanks, changing.
>>
>>>> + * @since 2.3M1
>>>> + */
>>> public boolean saveAsAuthor()
>>>
>>>> + public void saveAsAuthor() throws XWikiException
>>>> + {
>>>> + saveAsAuthor("", false);
>>>> + }
>>>> +
>>>> + /**
>>>> + * Save the document, the author (contentAuthor) of the code calling
>>>> this function takes responsibility.
>>>> + * Saves with minorEdit set to false.
>>>> + *
>>>> + * @param comment The comment to display in document history (what
>>>> did you change in the document)
>>>> + * @throws XWikiException if script author is not allowed to save the
>>>> document or if save operation fails.
>>>> + * @see #saveAsAuthor(String, boolean)
>>>> + * @since 2.3M1
>>>> + */
>>>> + public void saveAsAuthor(String comment) throws XWikiException
>>>> + {
>>>> + saveAsAuthor(comment, false);
>>>> + }
>>>> +
>>>> + /**
>>>> + * Save the document, the author (contentAuthor) of the code calling
>>>> this function takes responsibility.
>>>> + * This document will be saved if the author of the document
>>>> containing the code which calls this function has
>>>> + * edit access to it. The contentAuthor of this document will be set
>>>> to the author of the calling script, not the
>>>> + * viewer.
>>> This line doesn't make sense:
>> I Will remove it.
>>
>>>> + * It is unwise to allow this function to be called by the viewer of
>>>> the script.
>>>> + *
>>>> + * @param comment The comment to display in document history (what
>>>> did you change in the document)
>>>> + * @param minorEdit Set true to advance the document version number
>>>> by 0.1 or false to advance version to the next
>>>> + * integer + 0.1 eg: 25.1
>>>> + * @throws XWikiException if script author is not allowed to save the
>>>> document or if save operation fails.
>>>> + * @since 2.3M1
>>>> + */
>>>> + public void saveAsAuthor(String comment, boolean minorEdit) throws
>>>> XWikiException
>>>> + {
>>> I wonder if this is the best way to get the content author. What happens
>>> with all the different include methods? Specifically, DocumentA includes
>>> DocumentB (sheet or topic), the call is in DocumentB; what is the
>>> context document?
>> This was discussed:
>> "momentarily switched to the content author of the document in the context"
>> http://n2.nabble.com/Proposal-Add-APIs-to-get-and-save-document-in-the-security-context-of-the-author-of-the-script-which-td4791045.html
>>
>> I think that we should adhere to the standard procedure that
>> #includeInContext and {{include context="default"}}
>> makes the document stay the same, this is how programmingRights work and I
>> don't think it is a security issue
>> any more than velocity is a security problem because a script coder can say
>> #evaluate($request.get('code'))
>> and allow the viewer to run code.
>>
>> #includeTopic - Included document
>> {{include context="new" Included document
>>
>> #includeInContext - Including document
>> #includeForm - Including document
>> {{include context="default" Including document
>>
>> I think discussion of changing this behavior should be part of the 2.0 API
>> design.
>
> Sure, I wasn't proposing a change, just wondering aloud.
>
>>>> + String author = getXWikiContext().getDoc().getContentAuthor();
>>>> + if (hasAccessLevel("edit", author)) {
>>>> + String viewer = getXWikiContext().getUser();
>>>> + try {
>>>> + getXWikiContext().setUser(author);
>>>> + saveDocument(comment, minorEdit);
>>>> + } finally {
>>>> + getXWikiContext().setUser(viewer);
>>>> + }
>>>> + } else {
>>>> + java.lang.Object[] args = { author,
>>>> getXWikiContext().getDoc(), this.doc.getFullName() };
>>>> + throw new XWikiException(XWikiException.MODULE_XWIKI_ACCESS,
>>>> XWikiException.ERROR_XWIKI_ACCESS_DENIED,
>>>> + "Access denied; user {0}, acting through script in
>>>> document {1} cannot save document {2}", null, args);
>>>> + }
>>>> + }
>>>> +
>>>> protected void saveDocument(String comment, boolean minorEdit)
>>>> throws XWikiException
>>>> {
>>>> XWikiDocument doc = getDoc();
>>>> @@ -1952,6 +2010,31 @@
>>>> }
>>>> }
>>>>
>>>> + /**
>>>> + * Delete the document, the author (contentAuthor) of the code
>>>> calling this function takes responsibility.
>>>> + * It is unwise to allow this function to be called by the viewer of
>>>> the script.
>>>> + *
>>>> + * @throws XWikiException if script author is not allowed to delete
>>>> the document or if save operation fails.
>>>> + * @since 2.3M1
>>>> + */
>>>> + public void deleteAsAuthor() throws XWikiException
>>>> + {
>>>> + String author = getXWikiContext().getDoc().getContentAuthor();
>>>> + if (hasAccessLevel("delete", author)) {
>>>> + String viewer = getXWikiContext().getUser();
>>>> + try {
>>>> + getXWikiContext().setUser(author);
>>>> + deleteDocument();
>>>> + } finally {
>>>> + getXWikiContext().setUser(viewer);
>>>> + }
>>>> + } else {
>>>> + java.lang.Object[] args = { author,
>>>> getXWikiContext().getDoc(), this.doc.getFullName() };
>>>> + throw new XWikiException(XWikiException.MODULE_XWIKI_ACCESS,
>>>> XWikiException.ERROR_XWIKI_ACCESS_DENIED,
>>>> + "Access denied; user {0}, acting through script in
>>>> document {1} cannot delete document {2}", null, args);
>>>> + }
>>>> + }
>>>> +
>>>> public void deleteWithProgrammingRights() throws XWikiException
>>>> {
>>>> if (hasProgrammingRights()) {
>>>>
>>>> Modified:
>>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/XWiki.java
>>>> ===================================================================
>>>> ---
>>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/XWiki.java
>>>> 2010-03-29 16:57:48 UTC (rev 28003)
>>>> +++
>>>> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api/XWiki.java
>>>> 2010-03-29 16:57:58 UTC (rev 28004)
>>>> @@ -172,6 +172,53 @@
>>>> }
>>>>
>>>> /**
>>>> + * Loads an Document from the database. Rights are checked on the
>>>> author (contentAuthor) of the document
>>>> + * containing the currently executing script before sending back the
>>>> loaded document.
>>>> + *
>>>> + * @param fullName the full name of the XWiki document to be loaded
>>>> + * @return a Document object or null if it is not accessible
>>>> + * @throws XWikiException
>>>> + * @since 2.3M1
>>>> + */
>>>> + public Document getDocumentAsAuthor(String fullName) throws
>>>> XWikiException
>>>> + {
>>>> + DocumentReference reference;
>>>> +
>>> This branching could be replaced by
>>> ....resolve(StringUtils.defaultString(fullName);
>> I copied the getDocument method and I think they ought to both be the same,
>> I think resolve(StringUtils.defaultString(fullName) would create confusion
>> as to
>> what is actually happening and StringUtils is not currently included in
>> api.XWiki
>> which already has to many imports.
>>
>>>> + // We ignore the passed full name if it's null to match behavior
>>>> of getDocument
>>>> + if (fullName != null) {
>>>> + // Note: We use the CurrentMixed Resolver since we want to
>>>> use the default page name if the page isn't
>>>> + // specified in the passed string, rather than use the
>>>> current document's page name.
>>>> + reference =
>>>> this.currentMixedDocumentReferenceResolver.resolve(fullName);
>>>> + } else {
>>>> + reference = this.defaultDocumentReferenceResolver.resolve("");
>>>> + }
>>> Perhaps you mean:
>>> return getDocumentAsAuthor(reference);
>> Thanks for pointing that out, changing...
>>
>>>> + return getDocument(reference);
>>>> + }
>>>> +
>>>> + /**
>>>> + * Loads an Document from the database. Rights are checked on the
>>>> author (contentAuthor) of the document
>>>> + * containing the currently executing script before sending back the
>>>> loaded document.
>>>> + *
>>>> + * @param reference the reference of the XWiki document to be loaded
>>>> + * @return a Document object or null if it is not accessible
>>>> + * @throws XWikiException
>>>> + * @since 2.3M1
>>>> + */
>>>> + public Document getDocumentAsAuthor(DocumentReference reference)
>>>> throws XWikiException
>>>> + {
>>>> + String author = getXWikiContext().getDoc().getContentAuthor();
>>>> + XWikiDocument doc = this.xwiki.getDocument(reference,
>>>> getXWikiContext());
>>>> + if (this.xwiki.getRightService().hasAccessLevel("view", author,
>>>> doc.getFullName(),
>>> == false should be replaced by a !
>> Will change.
>>
>>>> + getXWikiContext()) == false) {
>>>> + return null;
>>>> + }
>>>> +
>>>> + Document newdoc = doc.newDocument(getXWikiContext());
>>>> + return newdoc;
>>>> + }
>>>> +
>>>> + /**
>>>> * @param fullname the {...@link XWikiDocument#getFullName() name}
>>>> of the document to search for.
>>>> * @param lang an optional {...@link XWikiDocument#getLanguage()
>>>> language} to filter results.
>>>> * @return A list with all the deleted versions of a document in
>>>> the recycle bin.
>
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
