Alexander Wels has posted comments on this change. Change subject: userportal,webadmin: additional protection for GWT RPC ......................................................................
Patch Set 3: (8 comments) http://gerrit.ovirt.org/#/c/27024/3/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/GWTRPCCommunicationProvider.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/GWTRPCCommunicationProvider.java: Line 69: * Get the GWT RPC service. Line 70: * @param callback The callback to use when determining the service. Line 71: */ Line 72: private void getService(final ServiceCallback callback) { Line 73: ((ServiceDefTarget) this.xsrfService).setServiceEntryPoint(getXsrfServiceEndPoint()); > This xsrfService initialization could be made part of xsrfService instance Done Line 74: if (xsrfRequestBuilder.getXsrfToken() != null) { Line 75: callback.serviceFound(service); Line 76: } else { Line 77: xsrfService.getNewXsrfToken(new AsyncCallback<XsrfToken>() { Line 88: }); Line 89: } Line 90: } Line 91: Line 92: private String getXsrfServiceEndPoint() { > We could avoid this method if we initialized xsrfService via @Provides meth Done Line 93: if (baseUrl == null) { Line 94: baseUrl = GWT.getModuleBaseURL(); Line 95: } Line 96: return baseUrl + "xsrf"; //$NON-NLS-1$ Line 92: private String getXsrfServiceEndPoint() { Line 93: if (baseUrl == null) { Line 94: baseUrl = GWT.getModuleBaseURL(); Line 95: } Line 96: return baseUrl + "xsrf"; //$NON-NLS-1$ > Could we make "xsrf" a String constant within XsrfTokenServiceAsync interfa We could if it wasn't a GWT provided interface. But it is so we can't. But I moved it somehwere that makes sense. Line 97: } Line 98: Line 99: /** Line 100: * Constructor. Line 105: public GWTRPCCommunicationProvider(final GenericApiGWTServiceAsync asyncService, Line 106: final XsrfTokenServiceAsync xsrfTokenService) { Line 107: this.service = asyncService; Line 108: this.xsrfRequestBuilder = new XsrfRpcRequestBuilder(); Line 109: ((ServiceDefTarget) this.service).setRpcRequestBuilder(xsrfRequestBuilder); > Again, "service" (GenericApiGWTServiceAsync) could be initialized via @Prov Done Line 110: this.xsrfService = xsrfTokenService; Line 111: } Line 112: Line 113: /** Line 601: public void setBaseURL(String baseURL) { Line 602: this.baseUrl = baseURL; Line 603: } Line 604: Line 605: public XsrfRpcRequestBuilder getXsrfRequestBuilder() { > Do we need this method? (If yes, does it need to be public? As I understand No we don't after the associated where made. Line 606: return this.xsrfRequestBuilder; Line 607: } http://gerrit.ovirt.org/#/c/27024/3/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/XsrfRpcRequestBuilder.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/XsrfRpcRequestBuilder.java: Line 4: import com.google.gwt.user.client.rpc.RpcRequestBuilder; Line 5: import com.google.gwt.user.client.rpc.XsrfToken; Line 6: Line 7: public class XsrfRpcRequestBuilder extends RpcRequestBuilder { Line 8: public static final String XSRF_TOKEN_HEADER = "X-CSRF-Token"; //$NON-NLS-1$ > I suggest to avoid "X-" prefix for header names, honoring IETF recommendati Done Line 9: Line 10: private XsrfToken xsrfToken; Line 11: Line 12: @Override http://gerrit.ovirt.org/#/c/27024/3/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/XsrfProtectedRpcServlet.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/XsrfProtectedRpcServlet.java: Line 18: // Can't use the one from XsrfTokenServiceServlet as it is not public. Line 19: static final String COOKIE_NAME_NOT_SET_ERROR_MSG = Line 20: "Session cookie name not set! Use context-param to specify session cookie name"; //$NON-NLS-1$ Line 21: Line 22: String sessionCookieName = null; > OK, I see that the XSRF protection mechanism is based on comparing md5(JSES Correct, I followed the GWT version of this on how to generate/compare the values. We can make it whatever we want it to be. Line 23: Line 24: /** Line 25: * Default constructor. Line 26: */ http://gerrit.ovirt.org/#/c/27024/3/frontend/webadmin/modules/frontend/src/main/resources/META-INF/web-fragment.xml File frontend/webadmin/modules/frontend/src/main/resources/META-INF/web-fragment.xml: Line 91: <servlet-class>org.ovirt.engine.core.branding.BrandingCascadingResourceServlet</servlet-class> Line 92: </servlet> Line 93: Line 94: <servlet> Line 95: <servlet-name>xsrf</servlet-name> > Minor thing, can't we use "XsrfTokenServiceServlet" to stay consistent with Done Line 96: <servlet-class>com.google.gwt.user.server.rpc.XsrfTokenServiceServlet</servlet-class> Line 97: </servlet> Line 98: <!-- PageNotFound Servlet --> Line 99: <servlet> -- To view, visit http://gerrit.ovirt.org/27024 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4a5ad1f33eb985aa79e1376aecb48ae365d334d Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
