Alon Bar-Lev has posted comments on this change. Change subject: aaa: Using extensions API in built-in authz and auth ......................................................................
Patch Set 15: (14 comments) http://gerrit.ovirt.org/#/c/26602/15/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticationProfileRepository.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/AuthenticationProfileRepository.java: Line 14: Line 15: public class AuthenticationProfileRepository { Line 16: Line 17: private static final String AUTHN_SERVICE = Authn.class.getName(); Line 18: private static final String AUTHN_AUTHZ_PLUGIN = "ovirt.engine.aaa.authn.authz.plugin"; this should be fixed in "aaa: ExtensionsManager: use the new extension api" Line 19: Line 20: Line 21: private static volatile AuthenticationProfileRepository instance = null; Line 22: private Map<String, AuthenticationProfile> profiles = new HashMap<String, AuthenticationProfile>(); http://gerrit.ovirt.org/#/c/26602/15/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DbUserCacheManager.java: Line 80: Line 81: // Classify the users by directory. Note that the resulting map may have an entry with a null key, that Line 82: // corresponds to the users whose directory has been removed from the configuration. Line 83: Map<String, List<DbUser>> directoryToUsersMap = new HashMap<>(); Line 84: Map<String, ExtensionProxy> directoriesMap = new HashMap<>(); I do not wish you change entire code and naming.... but if you do introduce new variables, please use the new terms. Line 85: Line 86: for (DbUser dbUser : dbUsers) { Line 87: AuthenticationProfile profile = AuthenticationProfileRepository.getInstance().getProfile(dbUser.getDomain()); Line 88: if (profile == null) { http://gerrit.ovirt.org/#/c/26602/15/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java: Line 122: Properties authConfig = new Properties(); Line 123: authConfig.put(Base.ConfigKeys.NAME, "builtin-authn-internal"); Line 124: authConfig.put(Base.ConfigKeys.PROVIDES, Authn.class.getName()); Line 125: authConfig.put(Base.ConfigKeys.MODULE, "org.ovirt.engine.extensions.builtin"); Line 126: authConfig.put(Base.ConfigKeys.CLASS, InternalAuthn.class.getName()); hmmm.... you cannot assume you can import this during build. please remove the maven dependency of built-in plugins from all modules. Line 127: authConfig.put("ovirt.engine.aaa.authn.profile.name", "internal"); Line 128: authConfig.put("ovirt.engine.aaa.authn.authz.plugin", "internal"); Line 129: authConfig.put("config.authn.user.name", Config.<String> getValue(ConfigValues.AdminUser)); Line 130: authConfig.put("config.authn.user.password", Config.<String> getValue(ConfigValues.AdminPassword)); Line 134: Properties dirConfig = new Properties(); Line 135: dirConfig.put(Base.ConfigKeys.NAME, "internal"); Line 136: dirConfig.put(Base.ConfigKeys.PROVIDES, Authz.class.getName()); Line 137: dirConfig.put(Base.ConfigKeys.MODULE, "org.ovirt.engine.extensions.builtin"); Line 138: dirConfig.put(Base.ConfigKeys.CLASS, InternalAuthz.class.getName()); hmmm.... you cannot assume you can import this during build. Line 139: dirConfig.put("config.authz.user.name", Config.<String> getValue(ConfigValues.AdminUser)); Line 140: dirConfig.put("config.authz.user.id", "fdfc627c-d875-11e0-90f0-83df133b58cc"); Line 141: ExtensionsManager.getInstance().load(dirConfig); Line 142: } Line 180: Properties dirConfig = new Properties(); Line 181: dirConfig.put(Base.ConfigKeys.NAME, domain); Line 182: dirConfig.put(Base.ConfigKeys.PROVIDES, Authz.class.getName()); Line 183: dirConfig.put(Base.ConfigKeys.MODULE, "org.ovirt.engine.extensions.builtin"); Line 184: dirConfig.put(Base.ConfigKeys.CLASS, KerberosLdapAuthz.class.getName()); hmmm.... you cannot assume you can import this during build. Line 185: dirConfig.put("config.query.filter.size", Line 186: Config.<Long> getValue(ConfigValues.MaxLDAPQueryPartsNumber)); Line 187: ExtensionsManager.getInstance().load(dirConfig); Line 188: } http://gerrit.ovirt.org/#/c/26602/15/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthn.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthn.java: Line 45: adminUser Line 46: ).mput( Line 47: Authn.AuthRecord.VALID_TO, Line 48: "" Line 49: ) simply do not put VALID_TO, it will mean forever, do not put "" as we won't be able to parse date. Line 50: ); Line 51: output.put(Authn.InvokeKeys.RESULT, Authn.AuthResult.SUCCESS); Line 52: } Line 53: } http://gerrit.ovirt.org/#/c/26602/15/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthz.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/internal/InternalAuthz.java: Line 16: public class InternalAuthz implements Extension { Line 17: /** Line 18: * Line 19: */ Line 20: private static final long serialVersionUID = 6614140186031169227L; why serialized? Line 21: Line 22: private ExtMap context; Line 23: Line 24: private ExtMap adminUser; http://gerrit.ovirt.org/#/c/26602/15/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/Constants.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/Constants.java: Line 7: public static class Keys { Line 8: public static final ExtKey AUTHZ_NAME = new ExtKey("AAA_AUTHZ_NAME", Line 9: String.class, Line 10: "2eb1f541-0f65-44a1-a6e1-014e247595f1"); Line 11: } why do we need this? Line 12: http://gerrit.ovirt.org/#/c/26602/15/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthz.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapAuthz.java: Line 26: public class KerberosLdapAuthz implements Extension { Line 27: /** Line 28: * Line 29: */ Line 30: private static final long serialVersionUID = 1L; should not be serialized Line 31: Line 32: private static final Log log = LogFactory.getLog(KerberosLdapAuthz.class); Line 33: Line 34: private static final String USERS_QUERY_PREFIX = "(&($USER_ACCOUNT_TYPE)"; Line 97: long addedGroups = 0; Line 98: for (LdapGroup ldapGroup : ldapGroups) { Line 99: if (addedGroups++ < pageSize) { Line 100: results.add(mapLdapGroup(ldapGroup)); Line 101: } you can return more than page size for simplicity. Line 102: } Line 103: return output.mput(Authz.InvokeKeys.QUERY_RESULT, results); Line 104: } Line 105: Line 118: int addedUsers = 0; Line 119: for (LdapUser ldapUser : ldapUsers) { Line 120: if (addedUsers++ < pageSize) { Line 121: results.add(mapLdapUser(ldapUser)); Line 122: } same Line 123: } Line 124: return output.mput(Authz.InvokeKeys.QUERY_RESULT, results); Line 125: } Line 126: Line 264: ); Line 265: } Line 266: Line 267: private String generateFromFilter(ExtMap queryFilterRecord) { Line 268: StringBuilder result = new StringBuilder(); if you are to use string builder, please alter recursion to use string builder private String generateFromFilter(StringBuilder ret, ExtMap queryFilterRecord) { } so that single string builder be used all over, without transition to strings. Line 269: List<ExtMap> filter = queryFilterRecord.<List<ExtMap>> get(Authz.QueryFilterRecord.FILTER); Line 270: if (filter == null) { Line 271: ExtKey key = queryFilterRecord.<ExtKey> get(Authz.QueryFilterRecord.KEY); Line 272: result.append( Line 265: } Line 266: Line 267: private String generateFromFilter(ExtMap queryFilterRecord) { Line 268: StringBuilder result = new StringBuilder(); Line 269: List<ExtMap> filter = queryFilterRecord.<List<ExtMap>> get(Authz.QueryFilterRecord.FILTER); personally, I would not use this temp var, but that's me :) Line 270: if (filter == null) { Line 271: ExtKey key = queryFilterRecord.<ExtKey> get(Authz.QueryFilterRecord.KEY); Line 272: result.append( Line 273: String.format( http://gerrit.ovirt.org/#/c/26602/15/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapAuthenticateUserCommand.java File backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/LdapAuthenticateUserCommand.java: Line 68: user.getUserName() Line 69: ).mput( Line 70: Authn.AuthRecord.VALID_TO, Line 71: "" Line 72: ) please do not put valid to if it should be infinite Line 73: ); Line 74: Line 75: } else { Line 76: log.errorFormat("Failed authenticating. Domain is {0}. User is {1}. The user doesn't have a UPN", -- To view, visit http://gerrit.ovirt.org/26602 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I916012eab61a96bdb0f366d9dc8462325d7f726f Gerrit-PatchSet: 15 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
