Vojtech Szocs has posted comments on this change.

Change subject: Introduction of filters to unify AAA flows for UI and REST-API
......................................................................


Patch Set 47:

>> 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?

> as far as we understand there is no change in behaviour.

InvalidateSessionIfAuthorizationHeaderFilter is present in REST API web.xml so 
I assume that any request to REST API application containing "Authorization" 
header will cause the current REST API HttpSession to be invalidated, which in 
turn prevents reusing that (now invalidated) HttpSession through REST API 
persistent management feature (RestApiSessionMgmtFilter).

Yair/Alon, is this assumption correct?

If so, web applications running in browser will NOT be able to use REST API 
persistent management feature in following scenario:
* web application acquires auth token via REST API (i.e. NOT via interactive 
login page) by making request with "Authorization: Basic xxx" + "Prefer: 
persistent-auth" headers, as described in [1]
* web application attempts to reuse existing REST API session by passing auth 
token + "Prefer: persistent-auth" header, but browser will also include 
"Authorization: Basic xxx" header unconditionally
* InvalidateSessionIfAuthorizationHeaderFilter invalidates REST API session, 
subsequent filters will cause re-login and re-creation of REST API session
* web application is confused, because even though it sends "Prefer: 
persistent-auth" header, it always gets new auth token

[1] http://www.ovirt.org/Features/RESTSessionManagement

Is above scenario likely to happen? Did you test REST API changes with UI 
plugins that interact with REST API backend?

>> 10, what is the purpose of BasicAuthenticationFilter & NegotiationFilter in 
>> context of WebAdmin & UserPortal web application? (I think I'm missing 
>> something)

> 1. allow basic authentication at that context as well, sync the entire 
> feature set across application.

> 2. allow negotiation authentication, such as SPNEGO, OpenID, apache context, 
> SSOs.

So in practice, client applications will be able to make Basic / negotiation 
authentication requests also against WebAdmin / UserPortal application context? 
I don't see any use case for this. Can you please provide some use case to 
justify this?

In other words, BasicAuthenticationFilter & NegotiationFilter is already 
applied to REST API application context, why have it also for specific GWT UI 
applications?

> 3. "working with InitialContext like this..." - I disagree here, IMHO we 
> should use the bean in the JNDI context.

I was just suggesting that "make InitialContext, try { use context to do 
something } finally { close context }" is sort of extra complexity to deal 
with, it's not a big deal (I agree with bean used in JNDI context, my comment 
was rather focused on how we work with that context in code).

> 5. Thanks for the tip, IMHO this will be an overkill here + the 
> initialization depends on the caps field which is retreived by the request. I 
> do not want to pass getInstance the "caps" value.

Point accepted :-)

> 6, "usage of java.lang.Object in LoginUserParameters ... " - No need for this 
> when used from GUI, the field is not passed.

Client-side GWT code works with LoginUserParameters 
(CommunicationProvider.login) but you're right that "Object authRecord" field 
is not passed via GWT RPC. Technically, LoginUserParameters is subclass of 
VdcActionParametersBase and VdcActionParametersBase is part of GWT RPC 
interface signature (GenericApiGWTService) so it should have custom field 
serializer.

Anyway, I think that "Object authRecord" field in LoginUserParameters will be 
excluded in target JavaScript output, which as I understand, isn't a big deal.

-- 
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 <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

Reply via email to