Yair Zaslavsky has posted comments on this change. Change subject: Introduction of filters to unify AAA flows for UI and REST-API ......................................................................
Patch Set 38: (10 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(); > I was more worried about getting a null pointer exception when we call setS HI Alexander, IMHO this situation should not occur. We can check and throw exception if occurrs, what do u think? Line 487: } Line 488: ThreadLocalParamsContainer.setSessionId(sessionId); Line 489: return sessionId; Line 490: } 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(); > I would like to believe we always have parameters... and can get rid of the Hi, ThreadLocalParamsContainer is used not just to store the session id on the thread local. if you wish, I can check if we can remove the session id storage , and see if we indeed can assume we have parameters all the way. thoughts? 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 Hi, REST-API is retrieving the application mode via a query. I will check if can be simply read at the specific point from 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 Done 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 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 checkstyle plugin enforces me to have empty CTOR. 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: Done 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 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. see previous comment on checkstyle plugin. 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 Done 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 possible, but it didnt return this error :) 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$) > Actually looks like SessionUtils already defines the constant, why not simp not accessible via FRONTEND, and have some REST-API specific code. 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
