Yair Zaslavsky 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" Done 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 right. anyway, i had to change the code, sync should be fixed up, altough I have some "heretic" thought about why we should have sync at all. i'll elaborate in a separate email. 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 renamed back, i will take care of maven issues later on. 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. Done 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. Done 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 Done 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? leftover from the day it extended Directory. removed. 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? Please check KerberosLdapAuthn.doAuthenticate - for me it was a simple way (after all, we use maps :) ) to pass the name of authz to the params. 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 Done 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. Not sure I fully understood why does it matter. I am perfectly ok with this implementation if you are. 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 see above. 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 buil if i use a single StringBuilder allover, it will make the recusion complex, as the StringBuilder serves as a "reference". i will not be able to simply append to the beginning of the StringBuilder, etc... as in the case of nested filter where you don't have a fixed number of nested filters during compilation time of engine, it is perfectly normal to use StringBuilder to make the concatination. 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 :) as used more than once in the code, i prefer to keep it. 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 Done 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 <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [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
