Yair Zaslavsky has posted comments on this change.
Change subject: core: Introduce new authentication interfaces
......................................................................
Patch Set 8:
(12 comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationProfileFactory.java
Line 1: package org.ovirt.engine.core.authentication;
Line 2:
Line 3: import java.io.File;
Line 4:
Line 5: import org.slf4j.Logger;
Why not the standard logger we use in other places? (see for example
CommandBase.java)
Line 6: import org.slf4j.LoggerFactory;
Line 7:
Line 8: /**
Line 9: * An authentication profile is just a pair composed of an
authenticator and a directory so this class just looks up
Line 33: "the name of the profile.",
Line 34: config.getAbsoluteKey(NAME_PARAMETER),
Line 35: file.getAbsolutePath()
Line 36: );
Line 37: return null;
return null or maybe throwing a concrete exception indicating the error ?
(cannot load authenticatior, cannot load directory, something else..)
Line 38: }
Line 39:
Line 40: // Try to load the authenticator configuration:
Line 41: Configuration authenticatorView =
config.getView(AUTHENTICATOR_PARAMETER);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Authenticator.java
Line 4: * A authenticator is an object used to verify an identity. This
interface is empty, the real semantics are in the
Line 5: * extensions.
Line 6: */
Line 7: public interface Authenticator extends Managed {
Line 8: // Nothing.
Please follow Martin's comment on the //Nothing issue.
I think that the first lines that you wrote explain well why "nothing" :)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticatorFactory.java
Line 4: * This is just a concrete realization of the generic interface
intended to simplify things for developers of
Line 5: * authenticator factories.
Line 6: */
Line 7: public interface AuthenticatorFactory extends Factory<Authenticator> {
Line 8: // Nothing.
Same.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticatorManager.java
Line 6: /**
Line 7: * The authenticator manager is responsible for managing a collection
of authenticator objects.
Line 8: */
Line 9: public class AuthenticatorManager extends Manager<Authenticator> {
Line 10: // This is a singleton, and this is the instance:
I think it's obvious this is a singleton, but as you choose :)
Line 11: private static volatile AuthenticatorManager instance;
Line 12:
Line 13: /**
Line 14: * Get the instance of the authenticator manager.
Line 13: /**
Line 14: * Get the instance of the authenticator manager.
Line 15: */
Line 16: public static AuthenticatorManager getInstance() {
Line 17: if (instance == null) {
any reason why u dont want to initialize via static initialization block?
Line 18: synchronized (AuthenticatorManager.class) {
Line 19: if (instance == null) {
Line 20: instance = new AuthenticatorManager();
Line 21: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Configuration.java
Line 21: * @throws {@link java.io.IOException} if anything fails while
loading the properties file
Line 22: */
Line 23: public static Configuration load(File file) throws IOException {
Line 24: Properties properties = new Properties();
Line 25: InputStream in = null;
why not use here java7 autoclosable syntax?
As this is common, and GWT needs to be JDK6 compliant or something else?
Line 26: try {
Line 27: in = new FileInputStream(file);
Line 28: properties.load(in);
Line 29: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/DirectoryFactory.java
Line 4: * This is just a concrete realization of the generic interface
intended to simplify things for developers of directory
Line 5: * factories.
Line 6: */
Line 7: public interface DirectoryFactory extends Factory<Directory> {
Line 8: // No additional methods.
See previous comments on this.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/DirectoryManager.java
Line 17: public static DirectoryManager getInstance() {
Line 18: if (instance == null) {
Line 19: synchronized (DirectoryManager.class) {
Line 20: if (instance == null) {
Line 21: instance = new DirectoryManager();
+1
Line 22: }
Line 23: }
Line 24: }
Line 25: return instance;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Managed.java
Line 3: /**
Line 4: * This is a base interface for all managed objects (directories,
authenticators and profiles) included in this package.
Line 5: * The only thing that all these have in common is that they are
managed and that they have a name.
Line 6: */
Line 7: public interface Managed {
Why did you introduce this? Can you give example to something unmanaged? why
distinguish between managed and unmanaged?
Line 8: /**
Line 9: * Returns the name of the object, which usually is the name of the
underlying mechanism, for example, for an LDAP
Line 10: * directory service the name should be based on the suffix of the
LDAP database, so if the suffix is
Line 11: * {@code dc=example,dc=com} then the name should be {@code
example.com}. However this is not enforced at all,
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Manager.java
Line 53: this.factoryInterface = factoryInterface;
Line 54:
Line 55: // Create the indexes for factories and objects:
Line 56: factoriesByType = new ConcurrentHashMap<String, Factory<O>>();
Line 57: factoriesByClass = new ConcurrentHashMap<Class<?>,
Factory<O>>();
Great idea, I liked it !
However, Juan - please document well why you to this solution and not just
usage of ConcurrentHashMap.
Line 58: objectsByName = new ConcurrentHashMap<String, O>();
Line 59:
Line 60: // Create the set of already loaded modules:
Line 61: loadedModules = new HashSet<String>();
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/PasswordAuthenticator.java
Line 12: * returns
Line 13: * @return {@code true} if the password is correct or {@code
false} if it isn't correct or if anything fails while
Line 14: * checking
Line 15: */
Line 16: boolean authenticate(String name, char[] password);
Why is the password an array of char ?
--
To view, visit http://gerrit.ovirt.org/15596
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If84a0c9d6553d81cdbbe224972696f169cca90d4
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[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: mooli tayer <[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
