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. Is it right to eat the exception and
return false?
>
> 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.
>
>> + 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
