Oved Ourfali has posted comments on this change. Change subject: Introduction of filters to unify AAA flows for UI and REST-API ......................................................................
Patch Set 47: (2 comments) Minor comments from me. Yair - I think several methods for creating maps / extracting information from them will make the code cleaner and more pleasant to read/review. Please add those. That will also help Juan review. Thanks, Oved http://gerrit.ovirt.org/#/c/28022/47/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/BasicAuthenticationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/BasicAuthenticationFilter.java: Line 107: return result; Line 108: } Line 109: Line 110: Line 111: private void handleCredentials(HttpServletRequest request, String user, String password) { > This method is impossible to understand and review, full of casts and untyp I was able to review and understand it, although I agree there are a lot of casts there. Yair - perhaps you can add several methods here that will ease understanding/reviewing this code? Like getResult(outputMap, relevantResult)) Or isSucceeded(outputMap) or something similar? Also, add create a method for constructing the ExtMap used in order to invoke. Line 112: UserProfile userProfile = translateUser(user); Line 113: if (userProfile == null || userProfile.profile == null) { Line 114: log.error(String.format("Error in obtaining profile %1$s", userProfile.profile)); Line 115: } else { http://gerrit.ovirt.org/#/c/28022/47/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/NegotiationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/NegotiationFilter.java: Line 134: if (profile == null) { Line 135: continue; Line 136: } Line 137: Line 138: ExtMap output = profile.getAuthn().invoke( > This is impossible to understand and review. Again, seems clear to me, but extracting the map creation to some method will probably ease this, as we see that multiple times in this patch. Line 139: new ExtMap().mput( Line 140: Base.InvokeKeys.COMMAND, Line 141: Authn.InvokeCommands.AUTHENTICATE_NEGOTIATE Line 142: ).mput( -- To view, visit http://gerrit.ovirt.org/28022 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5536d123b6407acf41b6946dde796bd67d1e073 Gerrit-PatchSet: 47 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[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
