Liran Zelkha has posted comments on this change.
Change subject: core: Use new authentication interfaces
......................................................................
Patch Set 2: Code-Review+1
(6 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AdGroupsHandlingCommandBase.java
Line 46: protected DirectoryGroup getAdGroup() {
Line 47: if (mGroup == null && !getGroupId().equals(Guid.Empty)) {
Line 48: DbGroup dbGroup =
DbFacade.getInstance().getDbGroupDao().get(getGroupId());
Line 49: Directory directory =
DirectoryManager.getInstance().getDirectory(dbGroup.getDomain());
Line 50: // XXX: Should check for null directory.
Change to TODO:
Line 51: mGroup = directory.findGroup(dbGroup.getExternalId());
Line 52: }
Line 53: return mGroup;
Line 54: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java
Line 102: updates.addAll(refreshed);
Line 103: }
Line 104:
Line 105: // Actually update the users in the database (note that this
should be done with a batch update, but we don't
Line 106: // have support for that yet):
Please add a TODO for that - I'd be happy to add the batch update capabilities
in a separate patch.
Line 107: for (DbUser dbUser : updates) {
Line 108: dao.update(dbUser);
Line 109: }
Line 110: }
Line 184: }
Line 185:
Line 186: // Compare the attributes of the database user with those of
the directory, copy those that changed and update
Line 187: // the flag that indicates that the database needs to be
updated:
Line 188: if (!StringUtils.equals(dbUser.getFirstName(),
directoryUser.getName())) {
Do you think all those conditions belong in a method of the DbUser class?
Line 189: dbUser.setFirstName(directoryUser.getName());
Line 190: update = true;
Line 191: }
Line 192: if (!StringUtils.equals(dbUser.getLastName(),
directoryUser.getLastName())) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java
Line 24: import org.slf4j.Logger;
Line 25: import org.slf4j.LoggerFactory;
Line 26:
Line 27: public abstract class LoginBaseCommand<T extends LoginUserParameters>
extends CommandBase<T> {
Line 28: // The log:
Remove this comment
Line 29: private static final Logger log =
LoggerFactory.getLogger(LoginBaseCommand.class);
Line 30:
Line 31: public LoginBaseCommand(T parameters) {
Line 32: super(parameters);
Line 95: if (password == null) {
Line 96: log.error(
Line 97: "Can't login user \"{}\" because no password has been
provided.",
Line 98: loginName
Line 99: );
How does this work with other patches, for header supplied username and no
password (SSO usage, for instance)?
Line 100: return false;
Line 101: }
Line 102:
Line 103: // Check that the authentication profile name has been
provided:
Line 127: Authenticator authenticator = profile.getAuthenticator();
Line 128: if (!(authenticator instanceof PasswordAuthenticator)) {
Line 129: log.error(
Line 130: "Can't login user \"{}\" because the authentication
profile \"{}\" doesn't support password " +
Line 131: "authentication.",
Why do we care about that? Isn't that the authenticator responsibility?
Line 132: loginName,
Line 133: profileName
Line 134: );
Line 135:
addCanDoActionMessage(VdcBllMessages.USER_FAILED_TO_AUTHENTICATE);
--
To view, visit http://gerrit.ovirt.org/20412
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d5d6ee41b5373328eaf2f8448efe8e598609651
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Martin Peřina <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Ravi Nori <[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
