Juan Hernandez has posted comments on this change.
Change subject: core: Introduce new authentication interfaces
......................................................................
Patch Set 8:
(14 comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationFilter.java
Line 46: /**
Line 47: * Lazyly find all the profiles that that support negotiation and
store them reversed to simplify the creation of
Line 48: * the stacks of profiles later when processing requests.
Line 49: */
Line 50: private void findNegotiatingProfiles() {
The reason is that when the "init" method runs the configuration may haven't
been loaded yet. In fact it is loaded during the initialization of the Backend
bean, and that actually happens after running the "init" method.
Line 51: if (profiles == null) {
Line 52: synchronized(this) {
Line 53: if (profiles == null) {
Line 54: profiles = new ArrayList<AuthenticationProfile>(1);
Line 99: stack = new Stack<String>();
Line 100: for (AuthenticationProfile profile : profiles) {
Line 101: stack.push(profile.getName());
Line 102: }
Line 103: session.setAttribute(STACK_ATTR, stack);
The objective of the stack is to try several authenticators in sequence. If the
first fails then the second should be tried, so on. Each authenticator may
require several request/response iterations, so the only way to remember what
authenticators still need to be tried is the session, a local variable isn't
enough.
Line 104: }
Line 105:
Line 106: while (!stack.isEmpty()) {
Line 107: // Resume the negotiation with the profile at the top of
the stack:
Line 118: // If the negotiation is finished and authentication
succeeded then we have to remember in the session that
Line 119: // the user has been authenticated and its login name,
also we need to clean the stack of authenticators and
Line 120: // replace the request with a wrapper that contains the
user name returned by the authenticator:
Line 121: if (result.isAuthenticated()) {
Line 122: String name = result.getName() + "@" +
profile.getName();
Martin is right, at least partially, as using the StringBuilder manually may
require one less call. This is what the compiler generates:
String name = new StringBuilder() // Note the empty constructor
.append(result.getName())
.append("@")
.append(profile.getName())
.toString();
If I write it manually I can do this:
String name = new StringBuilder(result.getName()) // We save one call and one
resize
.append(result.getName())
.append("@")
.append(profile.getName())
.toString();
This requires one call less and potentially one less resize of the underlying
buffer, as the StringBuilder(String str) constructor allocates a buffer of size
str.length() + 16.
Anyhow, this code isn't a hot spot, so probably not worth further discussion.
Currently I am doing (in a not yet published patch set) as Martin suggested. If
you don't mind I won't change it again.
Line 123: session.setAttribute(AUTHENTICATED_ATTR, true);
Line 124: session.setAttribute(NAME_ATTR, name);
Line 125: session.removeAttribute(STACK_ATTR);
Line 126: req = new AuthenticatedRequestWrapper(req, name);
....................................................
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;
I think that someone pointed this out in another patch, don't remember if it
was Martin, Ravi or Mooli. Looks like a popular request. I will change the
logging implementation in all the patch series.
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;
Would you accept to throw a new "ConfigurationException" from here instead of
returning null?
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.
Done
....................................................
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.
Done
....................................................
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:
Done.
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) {
I already changed this idiom in all the patch series (in a patch set that isn't
published yet).
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;
The reason is that JDK6 syntax is forbidden in common, due to the need to be
compatible with GWT.
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.
Done
....................................................
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 {
I will try to remove this interface.
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>>();
I will try to make it clear in the comments.
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);
Having an array of char permits something like this:
String name = name;
char[] password = getThePasswordFromSomewhere();
authenticator.authenticate(name, passwords);
// Clear the password from memory:
Arrays.fill(password, '\0');
This avoids multiple copies of the password in the memory of the JVM. However
as we currently don't use this idiom in any of the multiple
"getThePasswordFromSomewhere()" that we have it is actually useless. I will
change it to String.
--
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: Alexander Wels <[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
