Vojtech Szocs has posted comments on this change. Change subject: webadmin: reports sso token ......................................................................
Patch Set 4: (9 comments) http://gerrit.ovirt.org/#/c/27542/4/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 30: */ Line 31: @SuppressWarnings({ "unchecked", "rawtypes" }) Line 32: public class GWTRPCCommunicationProvider implements CommunicationProvider { Line 33: Line 34: private static final String SSO_TOKEN_HEADER = "X-OVIRT-SSO-TOKEN"; //$NON-NLS-1$ Please see http://stackoverflow.com/questions/3561381/custom-http-headers-naming-conventions Since 2012 it's recommended not to use "X-" prefix for non-standard headers, preferring sensible names. Line 35: Line 36: /** Line 37: * GWT RPC service. Line 38: */ Line 58: @Inject Line 59: public GWTRPCCommunicationProvider(final GenericApiGWTServiceAsync asyncService, final EventBus eventBus) { Line 60: this.eventBus = eventBus; Line 61: this.service = asyncService; Line 62: ((ServiceDefTarget) this.service).setRpcRequestBuilder(new RpcRequestBuilder() { This code modifies the state of injected singleton, GenericApiGWTServiceAsync. User code should not do this, in my opinion. GenericApiGWTServiceAsync is bound in BaseSystemModule. I strongly advise to replace current binding: bind(GenericApiGWTServiceAsync.class).in(Singleton.class); with @Provides method, which takes care of proper instance initialization while accepting additional dependencies such as EventBus: @Provides @Singleton public GenericApiGWTServiceAsync getGenericApiGWTService(EventBus eventBus) { // no need to use GenericApiGWTServiceAsync.Util as this is GIN-managed singleton anyway GenericApiGWTServiceAsync service = GWT.create(GenericApiGWTServiceAsync.class); // cast to ServiceDefTarget and set RPC request builder as needed } This way, GWTRPCCommunicationProvider doesn't even need EventBus dependency. Essentially, EventBus is not a dependency of GWTRPCCommunicationProvider, it's a dependency of GenericApiGWTServiceAsync which uses custom RPC request builder. Line 63: @Override Line 64: protected void doSetCallback(RequestBuilder rb, final RequestCallback callback) { Line 65: super.doSetCallback(rb, new RequestCallback() { Line 66: Line 61: this.service = asyncService; Line 62: ((ServiceDefTarget) this.service).setRpcRequestBuilder(new RpcRequestBuilder() { Line 63: @Override Line 64: protected void doSetCallback(RequestBuilder rb, final RequestCallback callback) { Line 65: super.doSetCallback(rb, new RequestCallback() { In my opinion, just doing: rb.setCallback(new RequestCallback() { ... }); is a lot simpler than relying on "super" default implementation (which might potentially change anyway). In other words, I don't think that we need to reuse "super" behavior in this particular case. Line 66: Line 67: @Override Line 68: public void onResponseReceived(Request request, Response response) { Line 69: String tokenValue = response.getHeader(SSO_TOKEN_HEADER); http://gerrit.ovirt.org/#/c/27542/4/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/SSOTokenChange.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/SSOTokenChange.java: Line 2: Line 3: import com.gwtplatform.dispatch.annotation.GenEvent; Line 4: Line 5: @GenEvent Line 6: public class SSOTokenChange { Please add some brief Javadoc to explain the purpose of this event, for example: Event triggered when SSO token is acquired as part of successful authentication. Line 7: String ssoToken; Line 3: import com.gwtplatform.dispatch.annotation.GenEvent; Line 4: Line 5: @GenEvent Line 6: public class SSOTokenChange { Line 7: String ssoToken; Minor thing, I'd just use "token" as the class name makes it quite apparent that this relates to SSO token. http://gerrit.ovirt.org/#/c/27542/4/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/auth/SSOTokenData.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/auth/SSOTokenData.java: Line 1: package org.ovirt.engine.ui.common.auth; Line 2: Line 3: import com.google.gwt.core.client.JavaScriptObject; Line 4: Line 5: public final class SSOTokenData extends JavaScriptObject { Please add a brief Javadoc comment, for example: Overlay type for {@code ssoToken} global JS object. Line 6: Line 7: protected SSOTokenData() { Line 8: } Line 9: Line 14: public native String getValue() /*-{ Line 15: return this.value; Line 16: }-*/; Line 17: Line 18: public String getSsoToken() { I'd suggest to name this method just "getToken" as it's apparent that it relates to SSO token. Line 19: return getValue(); Line 20: } http://gerrit.ovirt.org/#/c/27542/4/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/BaseApplicationInit.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/BaseApplicationInit.java: Line 172: getLoginModel().autoLogin(loggedUser); Line 173: } Line 174: }); Line 175: Line 176: SSOTokenChangeEvent.fire(eventBus, SSOTokenData.instance().getSsoToken()); Shouldn't we first check if SSO token value is available? Line 177: // Indicate that the user should be logged in automatically Line 178: user.setAutoLogin(true); Line 179: } Line 180: http://gerrit.ovirt.org/#/c/27542/4/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/ReportInit.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/ReportInit.java: Line 180: new SSOTokenChangeHandler() { Line 181: Line 182: @Override Line 183: public void onSSOTokenChange(SSOTokenChangeEvent event) { Line 184: ssoToken = event.getSsoToken(); Please consider using "ReportInit.this.ssoToken" for better readability. Also, what should we do in case the SSO token is null? If such (edge) case occurs, I guess we should simply update some flag and make checkIfInitFinished method proceed with raising reportsInitEvent. Line 185: checkIfInitFinished(); Line 186: } Line 187: } Line 188: ); -- To view, visit http://gerrit.ovirt.org/27542 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib906a1cca87712fb4e31c68c5e8c70151976ce0f Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: Yaniv Dary <yd...@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
