31 Mar
2010
31 Mar
'10
12:59 a.m.
Sergiu Dumitriu wrote:
I Will remove it.
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-secu…
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.
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.
Thanks for pointing that out, changing...
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.
On 03/30/2010 06:50 AM, Caleb James DeLisle wrote:
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.
Sergiu Dumitriu wrote:
Yes,exceptions should be placed in the context.
On 03/29/2010 06:57 PM, cjdelisle (SVN) wrote:
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. 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. 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.
Thanks,
changing.
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 !