Juan Hernandez has posted comments on this change.

Change subject: core: Introduce new authentication interfaces
......................................................................


Patch Set 8:

(9 comments)

....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticatedRequestWrapper.java
Line 4: import javax.servlet.http.HttpServletRequest;
Line 5: import javax.servlet.http.HttpServletRequestWrapper;
Line 6: 
Line 7: /**
Line 8:  * This class wraps a HTTP request when the authentication process has 
suceeded in order to replace the principal used
Done
Line 9:  * by default by the container with one containing the name of the user 
that has been authenticated by the our own
Line 10:  * authentication mechanism.
Line 11:  */
Line 12: public class AuthenticatedRequestWrapper extends 
HttpServletRequestWrapper {


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationFilter.java
Line 14: import javax.servlet.http.HttpServletResponse;
Line 15: import javax.servlet.http.HttpSession;
Line 16: 
Line 17: /**
Line 18:  * This filter should be added to the {@code web.xml} file to the 
applications that wan't to use the authentication
Done
Line 19:  * mechanism implemented in this package.
Line 20:  */
Line 21: public class AuthenticationFilter implements Filter {
Line 22:     // The authentication profiles used to perform the authentication 
process:


Line 25:     // We store a boolean flag in the HTTP session that indicates if 
the user has been already authenticated, this is
Line 26:     // the key for that flag:
Line 27:     private static final String AUTHENTICATED_ATTR = 
AuthenticationFilter.class.getName() + ".authenticated";
Line 28: 
Line 29:     // When an user has been authenticated we store its login name in 
the HTTP session, this is the key for that name:
Mooli, my English grammar skills are really poor, thanks. Done.
Line 30:     private static final String NAME_ATTR = 
AuthenticationFilter.class.getName() + ".name";
Line 31: 
Line 32:     // In order to support several alternative authenticators we store 
their names in a stack inside the HTTP session,
Line 33:     // this is the key for that stack:


Line 39:     }
Line 40: 
Line 41:     @Override
Line 42:     public void destroy() {
Line 43:         // Nothing.
Done
Line 44:     }
Line 45: 
Line 46:     /**
Line 47:      * Lazyly find all the profiles that that support negotiation and 
store them reversed to simplify the creation of


Line 70:         // authentication to perform:
Line 71:         findNegotiatingProfiles();
Line 72:         if (profiles.isEmpty()) {
Line 73:             chain.doFilter(req, rsp);
Line 74:             return;
Note that the administrator of the engine may have selected to use a password 
authenticator instead of a negotiating authenticator. In that case we need to 
let the request go to the application, so that it can show the user/password 
dialog and perform the authentication.
Line 75:         }
Line 76: 
Line 77:         // Perform the authentication:
Line 78:         doFilter((HttpServletRequest) req, (HttpServletResponse) rsp, 
chain);


Line 99:             stack = new Stack<String>();
Line 100:             for (AuthenticationProfile profile : profiles) {
Line 101:                 stack.push(profile.getName());
Line 102:             }
Line 103:             session.setAttribute(STACK_ATTR, stack);
Mooli is right.
Line 104:         }
Line 105: 
Line 106:         while (!stack.isEmpty()) {
Line 107:             // Resume the negotiation with the profile at the top of 
the stack:


Line 111: 
Line 112:             // If the negotiation isn't finished then we assume that 
the response has been populated by the
Line 113:             // authenticator and we just let the container sent it 
back to the client:
Line 114:             if (result == null) {
Line 115:                 return;
It does interfere, and that is on purpose. The idea is that if the 
authentication is in progress and being handled by an authenticator then the 
request shouldn't make any further progress in the filter chain. It is the 
responsibility of that authenticator to populate the response as required.

From the point of view of the user will look as the authenticator determines. 
For a Basic authenticator, for example, the user won't notice anything because 
the browser handles it automatically.

Imagine that we want to create an authenticator that shows to the user a page 
with a captcha in order to perform login. This authenticator will be 
responsible for generating that HTML page. This is why the authenticator 
receives the request and response parameters, it can do anything a servlet can 
do, including generating HTML pages. The user will see that page, fill the 
form, and submit it. The submitted data will be processed again by the 
authenticator, and if the form is correct it will finish the authentication, 
returning something different to null.

All we can do here is remember what authenticator is performing the 
authentication, that is why we have the stack. If the authenticator itself 
needs to remember something it should store it in the session, in the URL as 
parameter, in a hidden field, etc.
Line 116:             }
Line 117: 
Line 118:             // If the negotiation is finished and authentication 
succeeded then we have to remember in the session that
Line 119:             // the user has been authenticated and its login name, 
also we need to clean the stack of authenticators and


Line 118:             // If the negotiation is finished and authentication 
succeeded then we have to remember in the session that
Line 119:             // the user has been authenticated and its login name, 
also we need to clean the stack of authenticators and
Line 120:             // replace the request with a wrapper that contains the 
user name returned by the authenticator:
Line 121:             if (result.isAuthenticated()) {
Line 122:                 String name = result.getName() + "@" + 
profile.getName();
You both have good points. In order to settle, and being lazy, I will keep what 
I currently have.
Line 123:                 session.setAttribute(AUTHENTICATED_ATTR, true);
Line 124:                 session.setAttribute(NAME_ATTR, name);
Line 125:                 session.removeAttribute(STACK_ATTR);
Line 126:                 req = new AuthenticatedRequestWrapper(req, name);


Line 137:         // If we are here then there are no more authenticators to 
try so we need to invalidate the session and reject
Line 138:         // the request:
Line 139:         session.invalidate();
Line 140:         rsp.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
Line 141:     }
Yes Mooli, it is used here: http://gerrit.ovirt.org/21024


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If84a0c9d6553d81cdbbe224972696f169cca90d4
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Martin PeÅ™ina <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Ravi Nori <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: mooli tayer <[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