oracle / opengrok

OpenGrok is a fast and usable source code search and cross reference engine, written in Java
http://oracle.github.io/opengrok/
Other
4.33k stars 746 forks source link

Suggester needs to use PageConfig.getRequestedProjects() to get list of projects #2640

Closed ghost closed 5 years ago

ghost commented 5 years ago

I am using version 1.1.2 on tomcat Steps to re-produce

Observation If we select all those projects by clicking each one then there is no issue. Also if we refresh page after selecting the projects (by clicking select all or group name), suggester starts working.

vladak commented 5 years ago

full stack trace ?

ghost commented 5 years ago

24-Jan-2019 14:44:40.134 WARNING [http-nio-8080-exec-9] org.opengrok.web.api.error.GenericExceptionMapper.toResponse Exception while processing request java.lang.NullPointerException at org.opengrok.indexer.authorization.AuthorizationStack.isAllowed(AuthorizationStack.java:196) at org.opengrok.indexer.authorization.AuthorizationFramework.performCheck(AuthorizationFramework.java:838) at org.opengrok.indexer.authorization.AuthorizationFramework.checkAll(AuthorizationFramework.java:796) at org.opengrok.indexer.authorization.AuthorizationFramework.isAllowed(AuthorizationFramework.java:205) at org.opengrok.web.api.v1.suggester.provider.filter.AuthorizationFilter.filter(AuthorizationFilter.java:66) at org.glassfish.jersey.server.ContainerFilteringStage.apply(ContainerFilteringStage.java:132) at org.glassfish.jersey.server.ContainerFilteringStage.apply(ContainerFilteringStage.java:68) at org.glassfish.jersey.process.internal.Stages.process(Stages.java:197) at org.glassfish.jersey.server.ServerRuntime$1.run(ServerRuntime.java:269) at org.glassfish.jersey.internal.Errors$1.call(Errors.java:272) at org.glassfish.jersey.internal.Errors$1.call(Errors.java:268) at org.glassfish.jersey.internal.Errors.process(Errors.java:316) at org.glassfish.jersey.internal.Errors.process(Errors.java:298) at org.glassfish.jersey.internal.Errors.process(Errors.java:268) at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:289) at org.glassfish.jersey.server.ServerRuntime.process(ServerRuntime.java:256) at org.glassfish.jersey.server.ApplicationHandler.handle(ApplicationHandler.java:703) at org.glassfish.jersey.servlet.WebComponent.serviceImpl(WebComponent.java:416) at org.glassfish.jersey.servlet.WebComponent.service(WebComponent.java:370) at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:389) at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:342) at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:229) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:231) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:53) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.opengrok.web.StatisticsFilter.doFilter(StatisticsFilter.java:59) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.opengrok.web.AuthorizationFilter.doFilter(AuthorizationFilter.java:61) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:199) at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96) at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:490) at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:139) at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:92) at org.apache.catalina.valves.AbstractAccessLogValve.invoke(AbstractAccessLogValve.java:668) at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74) at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:343) at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:408) at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:66) at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:770) at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1415) at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1135) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) at java.base/java.lang.Thread.run(Thread.java:844)

ghost commented 5 years ago

I suspect its not a backend issue. Backend works fine if we select the individual projects or refresh the page after bulk selection like I mentioned in the first comment. Something goes wrong in the request sent from the frontend when doing bulk selection using the "Select All" button or clicking the group name in the project drop down.

vladak commented 5 years ago

This is a dup of #2587. Do you run with authorization framework enabled ? How does your authorization stack look like ?

vladak commented 5 years ago

AuthorizationFramework.java#isAllowed() calls checkAll():

204      public boolean isAllowed(HttpServletRequest request, Project project) {
205          return checkAll(
206                  request,
207                  "plugin_framework_project_cache",
208                  project,
209                  new AuthorizationEntity.PluginDecisionPredicate() {
210                      @Override
211                      public boolean decision(IAuthorizationPlugin plugin) {
212                          return plugin.isAllowed(request, project);
213                      }

which then calls performCheck():

758      private boolean checkAll(HttpServletRequest request, String cache, Nameable entity,
759              AuthorizationEntity.PluginDecisionPredicate pluginPredicate,
760              AuthorizationEntity.PluginSkippingPredicate skippingPredicate) {
...

796              overallDecision = performCheck(entity, pluginPredicate, skippingPredicate);

which is a wrapper:

834      private boolean performCheck(Nameable entity,
835              AuthorizationEntity.PluginDecisionPredicate pluginPredicate,
836              AuthorizationEntity.PluginSkippingPredicate skippingPredicate) {
837  
838              return stack.isAllowed(entity, pluginPredicate, skippingPredicate);
839      }

and leads to AuthorizationStack#isAllowed() that hits the NPE on line 196:

191      public boolean isAllowed(Nameable entity,
192              PluginDecisionPredicate pluginPredicate,
193              PluginSkippingPredicate skippingPredicate) {
194          boolean overallDecision = true;
195          LOGGER.log(Level.FINER, "Authorization for \"{0}\" in \"{1}\" [{2}]",
196                  new Object[]{entity.getName(), this.getName(), this.getFlag()});

The entity is thus the project that somehow became null.

vladak commented 5 years ago

AuthorizationFramework.java#isAllowed() should short circuit null arguments before passing them on. Or maybe do it in checkAll() since this is common for both isAllowed() methods - the group might became null as well.

vladak commented 5 years ago

Of course, this will only protect the authorization framework against null entities. The real root cause is yet to be determined.

ghost commented 5 years ago

No, I don't have authorization framework enabled. However I debugged the code by attaching the IDE debugger. Found that this problem will happen if you have project groups defined.

So if you have project groups defined, when you click "Select All" or click on a project group , UI sends the project group names along with the project name in the request. The code at opengrok-web/src/main/java/org/opengrok/web/api/v1/suggester/provider/filter/AuthorizationFilter.java line num 62 receives the project and group names as strings. Then when it tries to find the Project object for group name in line num 65, it gets null. Thats how entity becomes null and subsequent call throws NPE.

So as I originally suspected its an UI issues as the UI is sending project group names in the request.

vladak commented 5 years ago

Okay, it seems that the call to Project p = Project.getByName(project); in http://demo.opengrok.org/xref/opengrok/opengrok-web/src/main/java/org/opengrok/web/api/v1/suggester/provider/filter/AuthorizationFilter.java?r=a55b429e returns null.

However I fail to see how group name can get there. Maybe related to the changes in #2518 ?

vladak commented 5 years ago

Could you send a sample of the request representation (mainly all its parameters) as it arrives to the AuthorizationFilter ?

tulinkry commented 5 years ago

 Okay, it seems that the call to Project p = Project.getByName(project); in http://demo.opengrok.org/xref/opengrok/opengrok-web/src/main/java/org/opengrok/web/api/v1/suggester/provider/filter/AuthorizationFilter.java?r=a55b429e returns null.

yes. This should benefit from PageConfig.getProjects or how the method is called. It resolves the group parameter to projects.

vladak commented 5 years ago

you mean something like this ?

@@ -59,13 +60,11 @@ public class AuthorizationFilter implements ContainerRequestFilter {
         }
         AuthorizationFramework auth = env.getAuthorizationFramework();
         if (auth != null) {
-            String[] projects = request.getParameterValues(PROJECTS_PARAM);
-            if (projects != null) {
-                for (String project : projects) {
-                    Project p = Project.getByName(project);
-                    if (!auth.isAllowed(request, p)) {
-                        context.abortWith(Response.status(Response.Status.FORBIDDEN).build());
-                    }
+            PageConfig pageconfig = PageConfig.get(request);
+            for (String project : pageconfig.getRequestedProjects()) {
+                Project p = Project.getByName(project);
+                if (!auth.isAllowed(request, p)) {
+                    context.abortWith(Response.status(Response.Status.FORBIDDEN).build());
                 }
             }
         }
vladak commented 5 years ago

Still I don't see how group names could get into the projects param.

ghost commented 5 years ago

Could you send a sample of the request representation (mainly all its parameters) as it arrives to the AuthorizationFilter ?

I mentioned in my previous comment that the request that AuthorizationFilter receives contains projects as well as project group names.

vladak commented 5 years ago

Yes, however the group names should be under different parameter and suggester's AuthorizationFilter obtained the list via request.getParameterValues(PROJECTS_PARAM).

tulinkry commented 5 years ago

you mean something like this ?

yes, it also ensures that the project is not null

Still I don't see how group names could get into the projects param.

me neither

vladak commented 5 years ago

This will need more intervention in the suggester code as the model for SuggesterQueryData relies on the projects parameter:

public final class SuggesterQueryData {

    @QueryParam(AuthorizationFilter.PROJECTS_PARAM)
    private List<String> projects;

which is used in SuggesterQueryDataParser via getProjects().