Juan Hernandez has posted comments on this change.
Change subject: core: Introduce new authentication interfaces
......................................................................
Patch Set 8:
(15 comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationFilter.java
Line 17: /**
Line 18: * This filter should be added to the {@code web.xml} file to the
applications that wan't to use the authentication
Line 19: * mechanism implemented in this package.
Line 20: */
Line 21: public class AuthenticationFilter implements Filter {
Done
Line 22: // The authentication profiles used to perform the authentication
process:
Line 23: private volatile List<AuthenticationProfile> profiles;
Line 24:
Line 25: // We store a boolean flag in the HTTP session that indicates if
the user has been already authenticated, this is
Line 34: private static final String STACK_ATTR =
AuthenticationFilter.class.getName() + ".stack";
Line 35:
Line 36: @Override
Line 37: public void init(FilterConfig filterConfig) throws
ServletException {
Line 38: // Nothing.
I think it isn't worthless, it means that I didn't want to put anything there
on purpose, not just because the IDE generated an empty method. What do you
suggest in such cases?
Line 39: }
Line 40:
Line 41: @Override
Line 42: public void destroy() {
Line 43: // Nothing.
Line 44: }
Line 45:
Line 46: /**
Line 47: * Lazyly find all the profiles that that support negotiation and
store them reversed to simplify the creation of
Done
Line 48: * the stacks of profiles later when processing requests.
Line 49: */
Line 50: private void findNegotiatingProfiles() {
Line 51: if (profiles == null) {
Line 93:
Line 94: // We need to remember which of the profiles was managing the
negotiation with the client, so we store a stack
Line 95: // of the available authenticators in the session:
Line 96: @SuppressWarnings("unchecked")
Line 97: Stack<String> stack = (Stack<String>)
session.getAttribute(STACK_ATTR);
Stack is old, but not deprecated. Anyhow I agree that Deque is better, will
change that.
Line 98: if (stack == null) {
Line 99: stack = new Stack<String>();
Line 100: for (AuthenticationProfile profile : profiles) {
Line 101: stack.push(profile.getName());
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();
String builder is what the Java compiler does internally. This is what the
compiler generates:
new #5 // class java/lang/StringBuilder
dup
invokespecial #6 // Method
java/lang/StringBuilder."<init>":()V
aload_1
invokevirtual #7 // Method getName:()Ljava/lang/String;
invokevirtual #8 // Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
ldc #9 // String @
invokevirtual #8 // Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
aload_2
invokevirtual #7 // Method getName:()Ljava/lang/String;
invokevirtual #8 // Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
Using StringBuilder directly is better if you actually know the size of the
result, as it avoids unecessary resizes of the underlying buffer (the default
size is 16 character), something like this:
StringBuilder buffer = new StringBuilder(
result.getName().length() +
1 +
profile.getName().length()
);
buffer.append(result.getName());
buffer.append("@");
buffer.append(profile.getName());
However this requires extra calls to the "getName()" and "length()" methods, so
all in all I think it isn't worth.
Or maybe you mean a completely different thing when you suggest using
StringBuilder. Can you elaborate?
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/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();
The checstyle rules in the common module avoid initializing fields:
<plugin>
<artifactId>maven-checkstyle-plugin</artifactId>
<configuration>
<propertyExpansion>disallowFinals=true</propertyExpansion>
<propertyExpansion>disallowMemberInit=true</propertyExpansion>
</configuration>
</plugin>
This is to prevent serialization issues in GWT.
I could however make the initialization in a static initialization block:
static {
instance = new DirectoryManager();
}
The only disadvantage of this is that the manager is created when the class is
loaded, whether it is later used or not. With the current idiom is is only
created when needed. Also note that the synch is executed only once (maybe more
there are multiple threads calling getInstance() concurrently). The real cost
here is not the sync, but the volatile variable, as it means that the generated
native code will be less optimized and that there will be more cache
invalidations. I don't have numbers to compare, so I will go with the simpler
alternative as you suggest.
Line 22: }
Line 23: }
Line 24: }
Line 25: return instance;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/Manager.java
Line 20: import org.slf4j.Logger;
Line 21: import org.slf4j.LoggerFactory;
Line 22:
Line 23: /**
Line 24: * This class provides some methods useful for all the managers that
need use a configuration file.
Done
Line 25: *
Line 26: * @param <O> the type of the managed object
Line 27: */
Line 28: public abstract class Manager<O extends Managed> {
Line 24: * This class provides some methods useful for all the managers that
need use a configuration file.
Line 25: *
Line 26: * @param <O> the type of the managed object
Line 27: */
Line 28: public abstract class Manager<O extends Managed> {
Yes, modules are only loaded once and never reloaded.
Not sure I understand. The decides to call createObject(name) or
findObject(name). Most callers will only use findObject(name) and the object
will be created only once. In any case a module is loaded only once, even if
createObject(name) is called multiple times.
Line 29: // The log:
Line 30: private static final Logger log =
LoggerFactory.getLogger(Manager.class);
Line 31:
Line 32: // Names of the parameters:
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>>();
Yes, this maps will have many gets and few puts. Can you elaborate on how do
you suggest to use copy on write?
Line 58: objectsByName = new ConcurrentHashMap<String, O>();
Line 59:
Line 60: // Create the set of already loaded modules:
Line 61: loadedModules = new HashSet<String>();
Line 73: }
Line 74:
Line 75: /**
Line 76: * Finds a factory using the given configuration. The factory will
be located using the {@code type}, and
Line 77: * {@code module} properties. If the configuration file contains
contains the {@code module} parameter the manager
Done
Line 78: * will try to load that module. Then the factories including in
the SPI configuration of the module will be loaded
Line 79: * and registered with their types. For example, if the
configuration file contains the following:
Line 80: *
Line 81: * <pre>
Line 89: * @param file the file containing the configuration for the
instance, used only to generate useful log messages
Line 90: * @param config the configuration already loaded from the
properties file
Line 91: * @return a reference to the factory or {@code null} if the
factory can't be found for any reason
Line 92: */
Line 93: protected Factory<O> findFactory(File file, Configuration config) {
Yes, only one factory per type. The type is like the identifier of the factory.
But you can have multiple factories (and thus multiple types) for the same
interface.
Line 94: // This will be the result:
Line 95: Factory<O> factory = null;
Line 96:
Line 97: // If a module has been specified then load all the factories
inside:
Line 190: for (File file : files) {
Line 191: if (file.getName().endsWith(".conf")) {
Line 192: O object = createObject(file);
Line 193: if (object == null) {
Line 194: log.info(
Yes, that is a bit redundant. I think that saying the same thing twice is
better than risking not saying it at all. However I will see if I can make it
less redundant.
Line 195: "The object configured in file \"{}\" hasn't
been created.",
Line 196: file.getAbsolutePath()
Line 197: );
Line 198: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/PasswordAuthenticator.java
Line 1: package org.ovirt.engine.core.authentication;
Line 2:
Line 3: /**
Line 4: * A password authenticator checks an user name and a password and
returns {@code true} if the password is correct.
Done
Line 5: */
Line 6: public interface PasswordAuthenticator extends Authenticator {
Line 7: /**
Line 8: * Check the given name and password and return {@code true} if
they are correct.
Line 7: /**
Line 8: * Check the given name and password and return {@code true} if
they are correct.
Line 9: *
Line 10: * @param name the name of user being authenticated
Line 11: * @param password the password of the user as a array of
characters so that it can be cleaned once the method
Done
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: */
....................................................
File
frontend/webadmin/modules/gwt-extension/src/main/java/org/ovirt/engine/ui/uioverrides/org/ovirt/engine/core/authentication/DirectoryManager.java
Line 16: /**
Line 17: * Get an instance of the directory manager.
Line 18: */
Line 19: public static DirectoryManager getInstance() {
Line 20: if (instance == null) {
Done (inside an static initializer).
Line 21: instance = new DirectoryManager();
Line 22: }
Line 23: return instance;
Line 24: }
--
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
