Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DS-304, DS-1922: METS generator should check for READ permissions #2451

Merged
merged 3 commits into from Sep 9, 2021

Conversation

AlexanderS
Copy link
Contributor

This is a replacement for #2364. (I can port it to dspace-5_x if it is merged. Then it would replace #2363, too.)

This is a fix for DS-304 and DS-1922.

The DSpaceMETSGenerator should check, that the current user has READ permission on the object. This pull request adds a new method to the AbstractAdapter to check for this permissions (#2364 only handles the ItemAdapter via an instanceof check and a cast).

This pull request also handles a similar case for the DSpaceOREGenerator. The ore.xml does not contain all metadata, but at least the title and the authors are visible even if the current user does not have the permission to view the item.

If a user does not have the READ permission, an empty response with a 403 response code is returned. (We could throw an AuthorizeException, but clients expect plain XML on both endpoints and could be confused with a HTML response.)

Ensure that the metadata of objects are not accessible via the mets file, if
the current user doesn't have sufficient permissions.
This code was copied from the DSpaceMETSGenerator and is not used anymore.
The DSpaceOREGenerator needs to check whether the item is readable by the current
user before generating a XML document.
@tdonohue tdonohue added bug high priority interface: XMLUI (obsolete) XMLUI in DSpace versions 1.x through 6.x. Removed in 7.x quick win Pull request is small in size & should be easy to review and/or merge labels Jun 26, 2019
@tdonohue tdonohue added this to the 6.4 milestone Jun 26, 2019
Copy link
Contributor

@nwoodward nwoodward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Tested and confirmed it works.

@alanorth
Copy link
Contributor

alanorth commented Jul 23, 2021

I confirm this is a minor security issue in DSpace 6.3. Before applying the patch I withdrew an item and then had the following experience when trying to access it:

  • localhost:8080/handle/10568/76259 → "This item has been withdrawn and is no longer available."
  • localhost:8080/handle/10568/76259?XML → "This item is restricted"
  • localhost:8080/metadata/handle/10568/76259/mets.xml → works, all metadata is visible!

Unfortunately when I apply the patch in DSpace 6.3 it breaks my XMLUI completely. The error in dspace.log is:

2021-07-23 21:45:41,660 ERROR org.dspace.app.xmlui.cocoon.DSpaceCocoonServletFilter @ Serious Error Occurred Processing Request!
org.springframework.web.util.NestedServletException: Handler processing failed; nested exception is java.lang.NoSuchMethodError: org.dspace.app.xmlui.objectmanager.AbstractAdapter.isAuthorized()Z
	at org.springframework.web.servlet.DispatcherServlet.triggerAfterCompletionWithError(DispatcherServlet.java:1275)
	at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:951)
	at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:867)
	at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:951)
	at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:842)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:621)
	at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:827)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:728)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:303)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)
	at org.dspace.app.xmlui.cocoon.SetCharacterEncodingFilter.doFilter(SetCharacterEncodingFilter.java:113)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:241)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)
	at org.dspace.app.xmlui.cocoon.DSpaceCocoonServletFilter.doFilter(DSpaceCocoonServletFilter.java:160)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:241)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)
	at org.dspace.app.xmlui.cocoon.servlet.multipart.DSpaceMultipartFilter.doFilter(DSpaceMultipartFilter.java:119)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:241)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)
	at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:241)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)
	at org.dspace.utils.servlet.DSpaceWebappServletFilter.doFilter(DSpaceWebappServletFilter.java:78)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:241)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:208)
	at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:219)
	at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:110)
	at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:492)
	at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:165)
	at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:104)
	at org.apache.catalina.valves.CrawlerSessionManagerValve.invoke(CrawlerSessionManagerValve.java:235)
	at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:116)
	at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:452)
	at org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1201)
	at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:654)
	at org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run(JIoEndpoint.java:319)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.NoSuchMethodError: org.dspace.app.xmlui.objectmanager.AbstractAdapter.isAuthorized()Z
	at org.dspace.app.xmlui.cocoon.DSpaceMETSGenerator.generate(DSpaceMETSGenerator.java:111)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.apache.cocoon.core.container.spring.avalon.PoolableProxyHandler.invoke(PoolableProxyHandler.java:71)
	at com.sun.proxy.$Proxy185.generate(Unknown Source)
	at org.apache.cocoon.components.pipeline.AbstractProcessingPipeline.processXMLPipeline(AbstractProcessingPipeline.java:544)
	at org.apache.cocoon.components.pipeline.impl.AbstractCachingProcessingPipeline.processXMLPipeline(AbstractCachingProcessingPipeline.java:273)
	at org.apache.cocoon.components.pipeline.AbstractProcessingPipeline.process(AbstractProcessingPipeline.java:439)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.apache.cocoon.core.container.spring.avalon.PoolableProxyHandler.invoke(PoolableProxyHandler.java:71)
	at com.sun.proxy.$Proxy175.process(Unknown Source)
	at org.apache.cocoon.components.treeprocessor.sitemap.SerializeNode.invoke(SerializeNode.java:147)
	at org.apache.cocoon.components.treeprocessor.AbstractParentProcessingNode.invokeNodes(AbstractParentProcessingNode.java:55)
	at org.apache.cocoon.components.treeprocessor.sitemap.MatchNode.invoke(MatchNode.java:87)
	at org.apache.cocoon.components.treeprocessor.AbstractParentProcessingNode.invokeNodes(AbstractParentProcessingNode.java:55)
	at org.apache.cocoon.components.treeprocessor.sitemap.MatchNode.invoke(MatchNode.java:87)
	at org.apache.cocoon.components.treeprocessor.AbstractParentProcessingNode.invokeNodes(AbstractParentProcessingNode.java:78)
	at org.apache.cocoon.components.treeprocessor.sitemap.PipelineNode.invoke(PipelineNode.java:143)
	at org.apache.cocoon.components.treeprocessor.AbstractParentProcessingNode.invokeNodes(AbstractParentProcessingNode.java:78)
	at org.apache.cocoon.components.treeprocessor.sitemap.PipelinesNode.invoke(PipelinesNode.java:81)
	at org.apache.cocoon.components.treeprocessor.ConcreteTreeProcessor.process(ConcreteTreeProcessor.java:239)
	at org.apache.cocoon.components.treeprocessor.ConcreteTreeProcessor.process(ConcreteTreeProcessor.java:171)
	at org.apache.cocoon.components.treeprocessor.TreeProcessor.process(TreeProcessor.java:247)
	at org.apache.cocoon.servlet.RequestProcessor.process(RequestProcessor.java:351)
	at org.apache.cocoon.servlet.RequestProcessor.service(RequestProcessor.java:169)
	at org.apache.cocoon.sitemap.SitemapServlet.service(SitemapServlet.java:84)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:728)
	at org.apache.cocoon.servletservice.ServletServiceContext$PathDispatcher.forward(ServletServiceContext.java:468)
	at org.apache.cocoon.servletservice.ServletServiceContext$PathDispatcher.forward(ServletServiceContext.java:443)
	at org.apache.cocoon.servletservice.spring.ServletFactoryBean$ServiceInterceptor.invoke(ServletFactoryBean.java:264)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
	at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:204)
	at com.sun.proxy.$Proxy172.service(Unknown Source)
	at org.dspace.springmvc.CocoonView.render(CocoonView.java:113)
	at org.springframework.web.servlet.DispatcherServlet.render(DispatcherServlet.java:1216)
	at org.springframework.web.servlet.DispatcherServlet.processDispatchResult(DispatcherServlet.java:1001)
	at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:945)
	... 38 more

The patch was applied cleanly and I re-ran maven / ant successfully. Any ideas?

@tdonohue
Copy link
Member

tdonohue commented Sep 3, 2021

@AlexanderS or @nwoodward : It looks like @alanorth had issues running this in a recent test. Any ideas? Maybe this needs updates?

If this ticket cannot be moved forward, we'd likely need to revert to #2364 which uses a slightly different approach to check permissions.

@tdonohue tdonohue closed this Sep 3, 2021
@tdonohue tdonohue reopened this Sep 3, 2021
@nwoodward
Copy link
Contributor

I deployed from the PR branch again, and I cannot replicate the NoSuchMethodError mentioned by @alanorth. I checked Cocoon and DSpace logs. It would be good to find a third tester to see if someone else has the same problem.

Copy link
Contributor

@J4bbi J4bbi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. Get a 403 after withdrawing an item.

@nwoodward
Copy link
Contributor

We've recently noticed a side effect of this PR that may or may not have been intended. We have one DSpace instance where the admins wanted to restrict access to a few communities to logged in users. They removed the Anonymous READ authorization on the community and added a new READ policy for a group with certain users. Then they made the short description a notice about how you must be logged in to view the community.

But after applying this PR if you remove Anonymous READ policies on communities or collections then they appear as empty lines on the homepage and in the Communities & Collections list page. It might be fine if they were just removed from the lists altogether if they aren't public, but it would be better if they still appeared in lists and then users saw the "This community/collection is restricted" page if they clicked on them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug high priority interface: XMLUI (obsolete) XMLUI in DSpace versions 1.x through 6.x. Removed in 7.x quick win Pull request is small in size & should be easy to review and/or merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants