security: return custom permissions from all Realms

Note the behavior change: wildcard permissions aren't expanded anymore. Built-in ("account:*") and custom ("acme:*") permissions will not be expanded in the API (as custom Realms don't necessarily expose this information). This fixes #1133. Signed-off-by: Pierre-Alexandre Meyer <pierre@mouraf.org>
killbill · May 21, 2019 · 046ec54 · 046ec54
1 parent 6cc0078
commit 046ec54
Show file tree

Hide file tree

Showing 10 changed files with 240 additions and 126 deletions.
diff --git a/...es/killbill/src/main/java/org/killbill/billing/server/modules/KillBillShiroWebModule.java b/...es/killbill/src/main/java/org/killbill/billing/server/modules/KillBillShiroWebModule.java
@@ -1,7 +1,7 @@
 /*
  * Copyright 2010-2013 Ning, Inc.
- * Copyright 2014-2018 Groupon, Inc
- * Copyright 2014-2018 The Billing Project, LLC
+ * Copyright 2014-2019 Groupon, Inc
+ * Copyright 2014-2019 The Billing Project, LLC
  *
  * The Billing Project licenses this file to you under the Apache License, version 2.0
  * (the "License"); you may not use this file except in compliance with the
@@ -19,7 +19,7 @@
 package org.killbill.billing.server.modules;
 
 import java.util.Collection;
-import java.util.LinkedList;
+import java.util.Set;
 
 import javax.servlet.ServletContext;
 import javax.servlet.ServletRequest;
@@ -28,11 +28,12 @@
 
 import org.apache.shiro.authc.pam.ModularRealmAuthenticator;
 import org.apache.shiro.authc.pam.ModularRealmAuthenticatorWith540;
-import org.apache.shiro.authz.ModularRealmAuthorizer;
 import org.apache.shiro.cache.CacheManager;
 import org.apache.shiro.guice.web.ShiroWebModuleWith435;
+import org.apache.shiro.mgt.DefaultSecurityManager;
 import org.apache.shiro.mgt.SubjectDAO;
 import org.apache.shiro.realm.Realm;
+import org.apache.shiro.realm.text.IniRealm;
 import org.apache.shiro.session.mgt.SessionManager;
 import org.apache.shiro.session.mgt.eis.SessionDAO;
 import org.apache.shiro.web.filter.authc.BasicHttpAuthenticationFilter;
@@ -45,6 +46,7 @@
 import org.killbill.billing.server.security.KillbillJdbcTenantRealm;
 import org.killbill.billing.util.config.definition.RbacConfig;
 import org.killbill.billing.util.config.definition.RedisCacheConfig;
+import org.killbill.billing.util.config.definition.SecurityConfig;
 import org.killbill.billing.util.glue.EhcacheShiroManagerProvider;
 import org.killbill.billing.util.glue.KillBillShiroModule;
 import org.killbill.billing.util.glue.RealmsFromShiroIniProvider;
@@ -56,7 +58,10 @@
 import org.skife.config.ConfigSource;
 import org.skife.config.ConfigurationObjectFactory;
 
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Inject;
 import com.google.inject.Key;
+import com.google.inject.Provider;
 import com.google.inject.TypeLiteral;
 import com.google.inject.binder.AnnotatedBindingBuilder;
 import com.google.inject.matcher.AbstractMatcher;
@@ -70,10 +75,12 @@
 public class KillBillShiroWebModule extends ShiroWebModuleWith435 {
 
     private final ConfigSource configSource;
+    private final DefaultSecurityManager defaultSecurityManager;
 
     public KillBillShiroWebModule(final ServletContext servletContext, final ConfigSource configSource) {
         super(servletContext);
         this.configSource = configSource;
+        this.defaultSecurityManager = RealmsFromShiroIniProvider.get(configSource);
     }
 
     @Override
@@ -92,9 +99,18 @@ public String getString(final String propertyName) {
             bind(CacheManager.class).toProvider(EhcacheShiroManagerProvider.class).asEagerSingleton();
         }
 
+        final SecurityConfig securityConfig = new ConfigurationObjectFactory(configSource).build(SecurityConfig.class);
+        final Collection<Realm> realms = defaultSecurityManager.getRealms() != null ? defaultSecurityManager.getRealms() :
+                                         ImmutableSet.<Realm>of(new IniRealm(securityConfig.getShiroResourcePath())); // Mainly for testing
+        for (final Realm realm : realms) {
+            bindRealm().toInstance(realm);
+        }
+
         configureShiroForRBAC();
 
         configureShiroForTenants();
+
+        expose(new TypeLiteral<Set<Realm>>() {});
     }
 
     private void configureShiroForRBAC() {
@@ -129,6 +145,32 @@ private void configureShiroForTenants() {
         expose(KillbillJdbcTenantRealm.class);
     }
 
+    @Override
+    protected void bindWebSecurityManager(final AnnotatedBindingBuilder<? super WebSecurityManager> bind) {
+        //super.bindWebSecurityManager(bind);
+        // This following is to work around obscure Guice issues
+        bind.toProvider(KillBillWebSecurityManagerProvider.class).asEagerSingleton();
+    }
+
+    public static final class KillBillWebSecurityManagerProvider implements Provider<DefaultWebSecurityManager> {
+
+        private final Collection<Realm> realms;
+        private final SessionManager sessionManager;
+
+        @Inject
+        public KillBillWebSecurityManagerProvider(final Collection<Realm> realms, final SessionManager sessionManager) {
+            this.realms = realms;
+            this.sessionManager = sessionManager;
+        }
+
+        @Override
+        public DefaultWebSecurityManager get() {
+            final DefaultWebSecurityManager defaultWebSecurityManager = new DefaultWebSecurityManager(realms);
+            defaultWebSecurityManager.setSessionManager(sessionManager);
+            return defaultWebSecurityManager;
+        }
+    }
+
     @Override
     protected void bindSessionManager(final AnnotatedBindingBuilder<SessionManager> bind) {
         // Bypass the servlet container completely for session management and delegate it to Shiro.
@@ -175,20 +217,10 @@ public <I> void hear(final TypeLiteral<I> typeLiteral, final TypeEncounter<I> ty
                 public void afterInjection(final Object o) {
                     final DefaultWebSecurityManager webSecurityManager = (DefaultWebSecurityManager) o;
 
-                    // Other realms have been injected by Guice (bindRealm().toInstance(...) makes Guice throw a ClassCastException?!)
-                    final Collection<Realm> realmsFromShiroIni = RealmsFromShiroIniProvider.get(configSource);
-
-                    if (webSecurityManager.getAuthorizer() instanceof ModularRealmAuthorizer) {
-                        final ModularRealmAuthorizer modularRealmAuthorizer = (ModularRealmAuthorizer) webSecurityManager.getAuthorizer();
-                        final Collection<Realm> realms = new LinkedList<Realm>(realmsFromShiroIni);
-                        realms.addAll(modularRealmAuthorizer.getRealms());
-                        modularRealmAuthorizer.setRealms(realms);
-                    }
-
                     if (webSecurityManager.getAuthenticator() instanceof ModularRealmAuthenticator) {
                         final ModularRealmAuthenticator authenticator = (ModularRealmAuthenticator) webSecurityManager.getAuthenticator();
                         authenticator.setAuthenticationStrategy(new FirstSuccessfulStrategyWith540());
-                        webSecurityManager.setAuthenticator(new ModularRealmAuthenticatorWith540(realmsFromShiroIni, authenticator));
+                        webSecurityManager.setAuthenticator(new ModularRealmAuthenticatorWith540(webSecurityManager.getRealms(), authenticator));
                     }
                 }
             });

diff --git a/profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestSecurity.java b/profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestSecurity.java
@@ -152,15 +152,15 @@ public void testUserWithUpdates() throws KillBillClientException {
         logout();
         login(username, password);
         List<String> permissions = securityApi.getCurrentUserPermissions(requestOptions);
-        Assert.assertEquals(permissions.size(), Permission.values().length);
+        Assert.assertEquals(permissions, ImmutableList.<String>of("*"));
 
-        String newPassword = "IamTheBestWarrior";
+        final String newPassword = "IamTheBestWarrior";
         securityApi.updateUserPassword(username, new UserRoles(username, newPassword, null), requestOptions);
 
         logout();
         login(username, newPassword);
         permissions = securityApi.getCurrentUserPermissions(requestOptions);
-        Assert.assertEquals(permissions.size(), Permission.values().length);
+        Assert.assertEquals(permissions, ImmutableList.<String>of("*"));
 
         final String newRoleDefinition = "somethingLessNice";
         // Only enough permissions to invalidate itself in the last step...

diff --git a/util/src/main/java/org/killbill/billing/util/glue/KillBillShiroModule.java b/util/src/main/java/org/killbill/billing/util/glue/KillBillShiroModule.java
@@ -1,7 +1,7 @@
 /*
  * Copyright 2010-2013 Ning, Inc.
- * Copyright 2014-2018 Groupon, Inc
- * Copyright 2014-2018 The Billing Project, LLC
+ * Copyright 2014-2019 Groupon, Inc
+ * Copyright 2014-2019 The Billing Project, LLC
  *
  * The Billing Project licenses this file to you under the Apache License, version 2.0
  * (the "License"); you may not use this file except in compliance with the
@@ -18,24 +18,31 @@
 
 package org.killbill.billing.util.glue;
 
+import java.util.Collection;
+import java.util.Set;
+
 import org.apache.shiro.cache.CacheManager;
 import org.apache.shiro.guice.ShiroModule;
+import org.apache.shiro.mgt.DefaultSecurityManager;
 import org.apache.shiro.mgt.SecurityManager;
 import org.apache.shiro.mgt.SubjectDAO;
+import org.apache.shiro.realm.Realm;
 import org.apache.shiro.realm.text.IniRealm;
 import org.apache.shiro.session.mgt.DefaultSessionManager;
 import org.apache.shiro.session.mgt.SessionManager;
 import org.apache.shiro.session.mgt.eis.SessionDAO;
 import org.killbill.billing.platform.api.KillbillConfigSource;
 import org.killbill.billing.util.config.definition.RbacConfig;
 import org.killbill.billing.util.config.definition.RedisCacheConfig;
+import org.killbill.billing.util.config.definition.SecurityConfig;
 import org.killbill.billing.util.security.shiro.realm.KillBillJdbcRealm;
 import org.killbill.billing.util.security.shiro.realm.KillBillJndiLdapRealm;
 import org.killbill.billing.util.security.shiro.realm.KillBillOktaRealm;
 import org.skife.config.ConfigSource;
 import org.skife.config.ConfigurationObjectFactory;
 
-import com.google.inject.Provider;
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.TypeLiteral;
 import com.google.inject.binder.AnnotatedBindingBuilder;
 
 // For Kill Bill library only.
@@ -59,9 +66,18 @@ public static boolean isRBACEnabled() {
     }
 
     private final KillbillConfigSource configSource;
+    private final ConfigSource skifeConfigSource;
+    private final DefaultSecurityManager defaultSecurityManager;
 
     public KillBillShiroModule(final KillbillConfigSource configSource) {
         this.configSource = configSource;
+        this.skifeConfigSource = new ConfigSource() {
+            @Override
+            public String getString(final String propertyName) {
+                return configSource.getString(propertyName);
+            }
+        };
+        this.defaultSecurityManager = RealmsFromShiroIniProvider.get(skifeConfigSource);
     }
 
     protected void configureShiro() {
@@ -73,25 +89,22 @@ public String getString(final String propertyName) {
         }).build(RbacConfig.class);
         bind(RbacConfig.class).toInstance(config);
 
-        final ConfigSource skifeConfigSource = new ConfigSource() {
-            @Override
-            public String getString(final String propertyName) {
-                return configSource.getString(propertyName);
-            }
-        };
-
         bind(RbacConfig.class).toInstance(config);
 
-        final Provider<IniRealm> iniRealmProvider = RealmsFromShiroIniProvider.getIniRealmProvider(skifeConfigSource);
-        // Hack for Kill Bill library to work around weird Guice ClassCastException when using
-        // bindRealm().toInstance(...) -- this means we don't support custom realms when embedding Kill Bill
-        bindRealm().toProvider(iniRealmProvider).asEagerSingleton();
+        final SecurityConfig securityConfig = new ConfigurationObjectFactory(skifeConfigSource).build(SecurityConfig.class);
+        final Collection<Realm> realms = defaultSecurityManager.getRealms() != null ? defaultSecurityManager.getRealms() :
+                                         ImmutableSet.<Realm>of(new IniRealm(securityConfig.getShiroResourcePath())); // Mainly for testing
+        for (final Realm realm : realms) {
+            bindRealm().toInstance(realm);
+        }
 
         configureJDBCRealm();
 
         configureLDAPRealm();
 
         configureOktaRealm();
+
+        expose(new TypeLiteral<Set<Realm>>() {});
     }
 
     protected void configureJDBCRealm() {
@@ -112,7 +125,8 @@ protected void configureOktaRealm() {
 
     @Override
     protected void bindSecurityManager(final AnnotatedBindingBuilder<? super SecurityManager> bind) {
-        super.bindSecurityManager(bind);
+        //super.bindSecurityManager(bind);
+        bind.toInstance(defaultSecurityManager);
 
         final RedisCacheConfig redisCacheConfig = new ConfigurationObjectFactory(new ConfigSource() {
             @Override

diff --git a/util/src/main/java/org/killbill/billing/util/glue/RealmsFromShiroIniProvider.java b/util/src/main/java/org/killbill/billing/util/glue/RealmsFromShiroIniProvider.java
@@ -1,7 +1,7 @@
 /*
  * Copyright 2010-2013 Ning, Inc.
- * Copyright 2014-2018 Groupon, Inc
- * Copyright 2014-2018 The Billing Project, LLC
+ * Copyright 2014-2019 Groupon, Inc
+ * Copyright 2014-2019 The Billing Project, LLC
  *
  * The Billing Project licenses this file to you under the Apache License, version 2.0
  * (the "License"); you may not use this file except in compliance with the
@@ -18,59 +18,34 @@
 
 package org.killbill.billing.util.glue;
 
-import java.util.Collection;
-
 import org.apache.shiro.config.ConfigurationException;
 import org.apache.shiro.config.IniSecurityManagerFactory;
 import org.apache.shiro.mgt.DefaultSecurityManager;
 import org.apache.shiro.mgt.SecurityManager;
-import org.apache.shiro.realm.Realm;
-import org.apache.shiro.realm.text.IniRealm;
 import org.apache.shiro.util.Factory;
 import org.killbill.billing.util.config.definition.SecurityConfig;
 import org.skife.config.ConfigSource;
 import org.skife.config.ConfigurationObjectFactory;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.google.common.collect.ImmutableSet;
-import com.google.inject.Provider;
-
 public class RealmsFromShiroIniProvider {
 
     private static final Logger log = LoggerFactory.getLogger(RealmsFromShiroIniProvider.class);
 
-    public static Collection<Realm> get(final ConfigSource configSource) {
+    public static DefaultSecurityManager get(final ConfigSource configSource) {
         final SecurityConfig securityConfig = new ConfigurationObjectFactory(configSource).build(SecurityConfig.class);
 
-        Collection<Realm> realms = null;
         try {
             final Factory<SecurityManager> factory = new IniSecurityManagerFactory(securityConfig.getShiroResourcePath());
             // TODO Pierre hack - lame cast here, but we need to have Shiro go through its reflection magic
             // to parse the [main] section of the ini file. Without duplicating code, this seems to be possible only
             // by going through IniSecurityManagerFactory.
-            final DefaultSecurityManager securityManager = (DefaultSecurityManager) factory.getInstance();
-            realms = securityManager.getRealms();
+            return (DefaultSecurityManager) factory.getInstance();
         } catch (final ConfigurationException e) {
             log.warn("Unable to configure RBAC", e);
         }
 
-        return realms != null ? realms :
-               ImmutableSet.<Realm>of(new IniRealm(securityConfig.getShiroResourcePath())); // Mainly for testing
-    }
-
-    public static Provider<IniRealm> getIniRealmProvider(final ConfigSource configSource) {
-        for (final Realm cur : get(configSource)) {
-            if (cur instanceof IniRealm) {
-                return new Provider<IniRealm>() {
-                    @Override
-                    public IniRealm get() {
-                        return (IniRealm) cur;
-                    }
-                };
-            }
-        }
-
         return null;
     }
 }