Michael Pasternak has posted comments on this change.
Change subject: core, webadmin, restapi: Detach password from VdcUser
......................................................................
Patch Set 3: Code-Review-1
(8 comments)
....................................................
Commit Message
Line 12: passwords, and that is not always true, as some authentication
Line 13: mechanisms don't use passwords for authentication. This change detaches
Line 14: the password from the VdcUser class, storing it independently in the
Line 15: session, so that it will be easier later to use other types of
Line 16: authentication mechanisms.
to be honest i don't like separating the password from the user, even if it not
required for some authentication methods,
in my view while it in the VdcUser it well encapsulated,
also logically it does not make sense, simply because when one seeing password
in TL/SessionDataContainer, he can't immediately correlate the
password to the user, simply because he would expect it to see in user context
if it related to user ...,
if in some cases there is no user instantiated, we can create new object (like
LoginContext) in TL/SessionDataContainer and refer it from VdcUser or using it
as is, (btw in a future you may find that other details has to be extracted
from VdcUser, - this will solve this as well)
i think it would be better
Line 17:
Line 18: Change-Id: I0ab9321816ee28355a4118910086891f5a552014
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmLogonCommand.java
Line 40: final VM vm = getVm();
Line 41:
Line 42: // Send the log on command to the virtual machine:
Line 43: final VdcUser currentUser = getCurrentUser();
Line 44: final String password =
SessionDataContainer.getInstance().getPassword();
having it as string will make JVM storing password in a string-pool, please
consider using char array instead
Line 45: final String domainController = currentUser != null ?
currentUser.getDomainControler() : "";
Line 46: final boolean sentToVM = Backend
Line 47: .getInstance()
Line 48: .getResourceManager()
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/LdapBrokerCommandBase.java
Line 32: }
Line 33:
Line 34: protected void initCredentials(String domain) {
Line 35: VdcUser curUser;
Line 36: String curPassword;
having it as string will make JVM storing password in a string-pool, please
consider using char array instead
Line 37: SessionDataContainer sessionDataContainer =
SessionDataContainer.getInstance();
Line 38: if (StringUtils.isEmpty(getParameters().getSessionId())) {
Line 39: curUser = sessionDataContainer.getUser(false);
Line 40: curPassword = sessionDataContainer.getPassword();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/session/SessionDataContainer.java
Line 224: *
Line 225: * @return an array of characters containing the password or
Line 226: * <code>null</code> if the password is not available
Line 227: */
Line 228: public String getPassword(String sessionId) {
consider returning char array here instead, this will force storing password as
such and prevent JVM from storing it in a string-pool
Line 229: return (String) GetData(sessionId, PASSWORD_PARAMETER_NAME,
false);
Line 230: }
Line 231:
Line 232: /**
Line 234: *
Line 235: * @return an array of characters containing the password or
Line 236: * <code>null</code> if the password is not available
Line 237: */
Line 238: public String getPassword() {
same
Line 239: return (String) GetData(PASSWORD_PARAMETER_NAME, false);
Line 240: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/security/auth/LoginValidator.java
Line 94: VdcQueryReturnValue queryReturnValue =
backend.RunPublicQuery(VdcQueryType.ValidateSession, params);
Line 95: if (queryReturnValue != null) {
Line 96: VdcUser vdcUser = (VdcUser)
queryReturnValue.getReturnValue();
Line 97: if (vdcUser != null) {
Line 98: principal = new Principal(vdcUser.getUserName(), null,
vdcUser.getDomainControler());
the secrete is used at LoginValidator#validate, why you set it to null? if
following changes are address such behaviour, please mention it/them in the
patch or make it dependant on these change.
thanks
Line 99: sessionHelper.setSessionId(sessionId);
Line 100: current.set(vdcUser);
Line 101: }
Line 102: }
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/BaseApplicationInit.java
Line 99: }
Line 100:
Line 101: protected void performLogin(T loginModel) {
Line 102: VdcUser loggedUser = loginModel.getLoggedUser();
Line 103: String loginPassword = (String)
loginModel.getPassword().getEntity();
having it as string will make JVM storing loginPassword in a string-pool,
please consider using char array instead
Line 104:
Line 105: // UiCommon login preparation
Line 106: Frontend.setLoggedInUser(loggedUser);
Line 107: Frontend.setLoginPassword(loginPassword);
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/AbstractRdp.java
Line 29: public String getVdcUserName() {
Line 30: return Frontend.getLoggedInUser().getUserName();
Line 31: }
Line 32:
Line 33: public String getVdcUserPassword() {
return char array?
Line 34: return Frontend.getLoginPassword();
Line 35: }
Line 36:
Line 37: public String getVdcUserDomainController() {
--
To view, visit http://gerrit.ovirt.org/17096
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ab9321816ee28355a4118910086891f5a552014
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[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
