Outstanding suggestion. I just made the change in AuthUtils accordingly, and
also add 1 test case.
This is much cleaner. Thanks!

From:  Nathan Marz <notifications@github.com>
Reply-To:  nathanmarz/storm
<reply+p-3311003-ef3b34803e518ea80beab38a67b71f158f45f989-2708344@reply.gith
ub.com>
Date:  Sunday, March 10, 2013 3:07 PM
To:  nathanmarz/storm <storm@noreply.github.com>
Cc:  Andy Feng <andy.feng@gmail.com>
Subject:  Re: [storm] Apply Thrift SASL client/server framework for
authentication/authorization/audit (#469)


In src/jvm/backtype/storm/security/auth/AuthUtils.java:
> +import java.net.URL;
> +import java.net.URLClassLoader;
> +import java.util.Map;
> +
> +public class AuthUtils {
> +    public static final String LOGIN_CONTEXT_SERVER = "StormServer";
> +    public static final String LOGIN_CONTEXT_CLIENT = "StormClient";
> +    public static final String SERVICE = "storm_thrift_server";
> +    private static final Logger LOG =
LoggerFactory.getLogger(AuthUtils.class);
> +
> +    /**
> +     * Construct a JAAS configuration object per storm configuration file
> +     * @param storm_conf Storm configuration
> +     * @return
> +     */
> +    public static synchronized Configuration GetConfiguration(Map storm_conf)
{
I think we can improve this code by being explicit about using the file
config implementation of Configuration. This will also remove the need to do
any mucking around with System properties. Let's use
Configuration#getInstance and set the parameters according to
http://docs.oracle.com/javase/6/docs/technotes/guides/security/StandardNames
.html#Configuration and
http://docs.oracle.com/javase/6/docs/api/javax/security/auth/login/Configura
tion.html#getInstance(java.lang.String,
javax.security.auth.login.Configuration.Parameters)

So it should look like: Configuration.getInstance("JavaLoginConfig", new
URIParameter(new URI(loginConfigurationFile)))

This has the benefits of:
1. Being explicit that we're using a config file
2. Throwing an exception if the JVM doesn't have the config file
implementation
3. No side effects, so it's a pure function and we can get rid of the
synchronized
Does that make sense?

‹
Reply to this email directly or view it on GitHub
<https://github.com/nathanmarz/storm/pull/469/files#r3311003> .