Yair Zaslavsky has posted comments on this change.

Change subject: aaa: Added mapper usage
......................................................................


Patch Set 5:

(5 comments)

http://gerrit.ovirt.org/#/c/26970/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LoginBaseCommand.java:

Line 349:                             )
Line 350:                         );
Line 351:             }
Line 352:             result = outputMap.<ExtMap> 
get(Authn.InvokeKeys.AUTH_RECORD);
Line 353: 
> what about the map user? it should be in this command as well, no?
done
Line 354:         }
Line 355:         return result;
Line 356:     }
Line 357: 


http://gerrit.ovirt.org/#/c/26970/5/backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapMapper.java
File 
backend/manager/modules/builtin-extensions/src/main/java/org/ovirt/engine/extensions/aaa/builtin/kerberosldap/KerberosLdapMapper.java:

Line 11: public class KerberosLdapMapper implements Extension {
Line 12: 
Line 13:     private ExtMap context;
Line 14:     private Properties config;
Line 15:     private Object authzName;
> why object?
mistake.
Line 16: 
Line 17:     @Override
Line 18:     public void invoke(ExtMap input, ExtMap output) {
Line 19:         try {


Line 49:                 ).mput(
Line 50:                         Base.ContextKeys.BUILD_INTERFACE_VERSION,
Line 51:                         Base.INTERFACE_VERSION_CURRENT);
Line 52:         config = context.<Properties> 
get(Base.ContextKeys.CONFIGURATION);
Line 53:         authzName = config.get("org.ovirt.engine.config.authz.name");
> it is not authz name at all, it is default suffix.
1. I was sure under impression that we need a different plugin. I also told you 
at first I'm coding an InternalMapper, and once realizing it is not needed I 
stated i am going to code the KerberosLdapMapper - you didn't say I should not 
:)
i assume "re" stands for regular expression.
why pattern.1 and replace.1? do you intend to have several patterns and 
replaces?
what is the logic here?  find the name part from the regex, and then replace by 
appending  the suffix part to it?
and after that toLower on the entire resukt?
how general would u like this to be?
Line 54:     }
Line 55: 
Line 56:     private void doMapping(ExtMap input, ExtMap output) {
Line 57:         ExtMap authRecord = input.<ExtMap> 
get(Authn.InvokeKeys.AUTH_RECORD);


Line 54:     }
Line 55: 
Line 56:     private void doMapping(ExtMap input, ExtMap output) {
Line 57:         ExtMap authRecord = input.<ExtMap> 
get(Authn.InvokeKeys.AUTH_RECORD);
Line 58:         ExtMap outRecord = (ExtMap) authRecord.clone();
> interesting if we need clone... I was thinking of doing all by reference. w
that we should use clone and not change the input.
Line 59:         if (!authRecord.<String> 
get(Authn.AuthRecord.PRINCIPAL).contains("@")) {
Line 60:             outRecord.put(Authn.AuthRecord.PRINCIPAL, 
authRecord.<String> get(Authn.AuthRecord.PRINCIPAL) + "@"
Line 61:                     + authzName);
Line 62: 


http://gerrit.ovirt.org/#/c/26970/5/backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java
File 
backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java:

Line 90: 
Line 91:     public ExtensionProxy getExtensionByName(String name) throws 
ConfigurationException {
Line 92:         ExtensionEntry entry = loadedEntries.get(name);
Line 93:         if (entry == null) {
Line 94:             return null;
> so you need to be consistent, change anything that returns nothing? for exa
Done
Line 95: 
Line 96:         }
Line 97:         if (!entry.activated) {
Line 98:             throw new ConfigurationException(String.format(


-- 
To view, visit http://gerrit.ovirt.org/26970
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5ac853e9011bb6118796a4cd14c0b7425308f3b
Gerrit-PatchSet: 5
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

Reply via email to