Alexander Wels has posted comments on this change. Change subject: Introduction of filters to unify AAA flows for UI and REST-API ......................................................................
Patch Set 38: (11 comments) http://gerrit.ovirt.org/#/c/28022/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java: Line 482: Line 483: private static String addSessionToContext(VdcQueryParametersBase parameters) { Line 484: String sessionId = parameters.getSessionId(); Line 485: if (StringUtils.isEmpty(sessionId)) { Line 486: sessionId = ThreadLocalParamsContainer.getSessionId(); what happens in the case when parameters.getSessionId() is null or empty AND ThreadLocalParamsContainer.getSessionId() is null or empty? Or are those two conditions mutually exclusive? Line 487: } Line 488: ThreadLocalParamsContainer.setSessionId(sessionId); Line 489: return sessionId; Line 490: } http://gerrit.ovirt.org/#/c/28022/38/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java: Line 105: // update his isAdmin flag accordingly Line 106: updateUserData(); Line 107: Line 108: ApplicationMode appMode = ApplicationMode.from(Config.<Integer> getValue(ConfigValues.ApplicationMode)); Line 109: SessionDataContainer.getInstance().setData("app_mode", appMode); What is the point of putting the application mode in the session? Anywhere we would read that information can't we simply read the configuration? Line 110: setSucceeded(true); Line 111: } Line 112: @Override Line 113: protected boolean canDoAction() { http://gerrit.ovirt.org/#/c/28022/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/LoginUserParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/LoginUserParameters.java: Line 11: private String password; Line 12: private Object authRecord; Line 13: } Line 14: Line 15: private AuthenticationInformation authInfo; you should be able to make this a final field and initialize it here, that way you don't have to do it in all the constructors. Line 16: Line 17: private String profileName; Line 18: private VdcActionType actionType; Line 19: private boolean isAdmin; http://gerrit.ovirt.org/#/c/28022/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/SetDataOnSessionParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/SetDataOnSessionParameters.java: Line 3: public class SetDataOnSessionParameters extends VdcActionParametersBase { Line 4: Line 5: private static final long serialVersionUID = -4340219620005637644L; Line 6: private String key; Line 7: private Object value; these can be final. Line 8: Line 9: public SetDataOnSessionParameters() { Line 10: } Line 11: Line 5: private static final long serialVersionUID = -4340219620005637644L; Line 6: private String key; Line 7: private Object value; Line 8: Line 9: public SetDataOnSessionParameters() { What is the point of this empty constructor, you don't have any setters for the fields. Line 10: } Line 11: Line 12: public SetDataOnSessionParameters(String key, Object value) { Line 13: this.key = key; http://gerrit.ovirt.org/#/c/28022/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionParametersBase.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionParametersBase.java: Line 84: compensationEnabled = false; Line 85: parentCommand = VdcActionType.Unknown; Line 86: commandType = VdcActionType.Unknown; Line 87: imagesParameters = new ArrayList<VdcActionParametersBase>(); Line 88: sessionid = engineSessionId; Why not simply make the constructor like this: public VdcActionParametersBase(String engineSessionId) { this(); sessionId = engineSessionId; } No code duplication and much shorter. Line 89: } Line 90: Line 91: public Guid getCommandId() { Line 92: return commandId; http://gerrit.ovirt.org/#/c/28022/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetValueBySessionQueryParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetValueBySessionQueryParameters.java: Line 6: /** Line 7: * Line 8: */ Line 9: private static final long serialVersionUID = 6548937521017309017L; Line 10: private String key; this can be final. Line 11: Line 12: public GetValueBySessionQueryParameters() { Line 13: } Line 14: Line 9: private static final long serialVersionUID = 6548937521017309017L; Line 10: private String key; Line 11: Line 12: public GetValueBySessionQueryParameters() { Line 13: } What is the point of this empty constructor, there are no setters. Line 14: Line 15: public GetValueBySessionQueryParameters(String sessionId, String key) { Line 16: this.key = key; Line 17: setSessionId(sessionId); http://gerrit.ovirt.org/#/c/28022/38/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryParametersBase.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryParametersBase.java: Line 29: this(null); Line 30: } Line 31: Line 32: public VdcQueryParametersBase(String sessionId) { Line 33: this.sessionId = sessionId; Why not flip this around, have the constructor with the sessionId call the constructor without the parameter? That way you don't have to pass null into the constructor. Functionally it makes no difference, just reads a little nicer IMO. Line 34: refresh = true; Line 35: } Line 36: Line 37: public String getSessionId() { http://gerrit.ovirt.org/#/c/28022/38/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GenericApiGWTServiceImpl.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GenericApiGWTServiceImpl.java: Line 185: assert !newSession.equals(originalSession) : "new session the same as old session"; //$NON-NLS-1$ Line 186: Line 187: params.setSessionId(getEngineSessionId()); Line 188: params.setActionType(loginType); Line 189: VdcLoginReturnValueBase returnValue = (VdcLoginReturnValueBase) getBackend().login(params); Findbugs will probably report an unchecked cast here, maybe change login to return a VdcLoginReturnValueBase here? Line 190: if (returnValue.getSucceeded()) { Line 191: this.getThreadLocalResponse().addHeader("OVIRT-SSO-TOKEN", getSession().getId()); //$NON-NLS-1$ Line 192: getSession().setAttribute("ovirt_aaa_engineSessionId", returnValue.getSessionId()); //$NON-NLS-1$) Line 193: } Line 188: params.setActionType(loginType); Line 189: VdcLoginReturnValueBase returnValue = (VdcLoginReturnValueBase) getBackend().login(params); Line 190: if (returnValue.getSucceeded()) { Line 191: this.getThreadLocalResponse().addHeader("OVIRT-SSO-TOKEN", getSession().getId()); //$NON-NLS-1$ Line 192: getSession().setAttribute("ovirt_aaa_engineSessionId", returnValue.getSessionId()); //$NON-NLS-1$) the "ovirt_aaa_engineSessionId" string is used in several different locations, could we make it a constant somewhere that we can reference from everywhere? That makes it easier to change the value if we needed to. Line 193: } Line 194: return returnValue; Line 195: } Line 196: -- To view, visit http://gerrit.ovirt.org/28022 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5536d123b6407acf41b6946dde796bd67d1e073 Gerrit-PatchSet: 38 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
