Re: [xwiki-devs] [xwiki-notifications] r28004 - platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/api
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. 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.
+ * @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:
+ * 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?
+ 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);
+ // 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);
+ 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 !
+ 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/
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-secur... 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.
Hi Caleb & Sergiu, /**
+ * 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; + + // 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(""); + } +
+ return getDocument(reference);
This doesn't make sense, silly copy paste probably, shouldn't it be return getDocumentAsAuthor(reference); ?
+ }
On Tue, Mar 30, 2010 at 04:28, Sergiu Dumitriu <[email protected]> wrote:
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?
+ String author = getXWikiContext().getDoc().getContentAuthor();
Please have a look at XWIKI-5027, it is the right place for discussing that issue. Caleb changes does not make anything worse, but this is not perfect for sure. Denis -- Denis Gervalle SOFTEC sa - CEO eGuilde sarl - CTO
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-secur...
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/
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-secur...
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.
participants (3)
-
Caleb James DeLisle -
Denis Gervalle -
Sergiu Dumitriu