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?).
>>
>> 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.
>>
--
Sergiu Dumitriu
http://purl.org/net/sergiu/
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
