Vojtech Szocs has posted comments on this change.
Change subject: Introduction of filters to unify AAA flows for UI and REST-API
......................................................................
Patch Set 44:
Hi guys, below are my review comments.
1, HTTP header names (strings such as "Authorization" or "WWW-Authenticate")
should become constants in FiltersHelper
2, InvalidateSessionIfAuthorizationHeaderFilter - potential issue: typical web
browser _always and unconditionally_ sends HTTP "Authorization" header once
it's set for given origin, what will happen if this HTTP header will be present
in each request? Will this make REST API persistent session mechanism unusable?
3, working with InitialContext like this:
InitialContext context = new InitialContext();
try {
VdcLoginReturnValueBase returnValue = (VdcLoginReturnValueBase)
FiltersHelper.getBackend(context)...
} finally {
context.close();
}
I think it would be much easier and less error-prone to have some utility
method in FiltersHelper to take care of above try/finally pattern, for example:
public static BackendLocal getBackendFromInitialContext() {
InitialContext context = new InitialContext();
try {
return getBackend(context);
} finally {
context.close();
}
}
4, it would be nice to have all "private static Log" + "private static
ThreadLocal" marked as final too
5, double-checked locking pattern in NegotiationFilter.profiles - can't we just
use initialization-on-demand holder pattern, which is a lot simpler and less
error-prone, instead? Reference:
http://blog.temposwc.com/2011/02/getting-around-double-checked-locking.html
6, usage of java.lang.Object in LoginUserParameters - IIRC this implies the
need for custom field serializer for LoginUserParameters, but I didn't see one
in this patch
7, in GenericApiGWTServiceImpl replace "ovirt_aaa_engineSessionId" with
SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY
8, in GwtDynamicHostPageServlet extract
"request.getSession().getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY)"
into protected getEngineSessionId method for subclasses to reuse
9, in WebAdminHostPageServlet replace
"request.getSession().getAttribute(SessionConstants.HTTP_SESSION_ENGINE_SESSION_ID_KEY)"
with getEngineSessionId method defined in parent class (see above)
10, what is the purpose of BasicAuthenticationFilter & NegotiationFilter in
context of WebAdmin & UserPortal web application? (I think I'm missing
something)
--
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: 44
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
