Alon Bar-Lev has posted comments on this change.
Change subject: aaa: add support for basic athentication
......................................................................
Patch Set 4:
(8 comments)
ok, nice!
is it working with apache?
we should make the settings at application context, and read it from
configuration file [conf.d style], same like we do with the engine, using
%{xxx} within web.xml and shell like config.
http://gerrit.ovirt.org/#/c/37299/4/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/BasicAuthServlet.java
File
backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/BasicAuthServlet.java:
Line 22: protected void service(HttpServletRequest request,
HttpServletResponse response)
Line 23: throws ServletException, IOException {
Line 24: try {
Line 25: if (!containsUserCredentials(request)) {
Line 26: throw new AuthenticationException("Credentials
Required");
you can skip the exception, no? just set header and error here?
Line 27: } else {
Line 28:
request.getSession(true).setAttribute(SSOUtils.USER_CREDENTIALS,
getUserCredentials(request));
Line 29: response.sendRedirect(request.getContextPath() +
SSOUtils.LOGIN_PHASE2_URI + request.getQueryString());
Line 30: }
Line 24: try {
Line 25: if (!containsUserCredentials(request)) {
Line 26: throw new AuthenticationException("Credentials
Required");
Line 27: } else {
Line 28:
request.getSession(true).setAttribute(SSOUtils.USER_CREDENTIALS,
getUserCredentials(request));
hmmm... I understand now... you throw exception out of getUserCredentials.... I
would have just returned null if no credentials, as it is not an error, then
you can also remove the containsUserCredentials call above.
Credentials creds = getCreds();
if (creds == null) {
redirect
} else {
store
}
Line 29: response.sendRedirect(request.getContextPath() +
SSOUtils.LOGIN_PHASE2_URI + request.getQueryString());
Line 30: }
Line 31: } catch (AuthenticationException ex) {
Line 32: response.setHeader("WWW-Authenticate", "Basic realm=\"" +
realm + "\"");
Line 42: private Credentials getUserCredentials(HttpServletRequest request)
throws AuthenticationException {
Line 43: String[] creds = new String(
Line 44:
Base64.decodeBase64(request.getHeader(SSOUtils.HEADER_AUTHORIZATION).substring("Basic".length())),
Line 45: Charset.forName("UTF-8")
Line 46: ).split(":", 2);
if you do not have 2 components, you should assume that you do not have
credentials, please do not generate exceptions in authentication.
Line 47: return translateUser(creds[0], creds[1]);
Line 48: }
Line 49:
Line 50: private Credentials translateUser(String user, String password)
throws AuthenticationException {
http://gerrit.ovirt.org/#/c/37299/4/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSOServlet.java
File
backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/EngineSSOServlet.java:
Line 46
Line 47
Line 48
Line 49
Line 50
not sure exception here is also good, better just to redirect to login page.
I just wounder what Engine within servlet means...
Line 1: package org.ovirt.engine.core.sso.servlets;
Line 2:
Line 3: import org.apache.commons.lang.StringUtils;
Line 4: import org.ovirt.engine.core.sso.utils.AuthenticationException;
Line 52: enableBasicAuth = Boolean.parseBoolean(strVal);
Line 53: }
Line 54: boolean needsBasicAuthHandling = enableBasicAuth &&
!containsUserCredentialsInParams(request.getParameterMap());
Line 55: try {
Line 56: Credentials userCredentials =
getUserCredentials(enableBasicAuth, request);
a function that gets a selector (enableBasicAuth) is not nightly coupled.
you should notice that we do not really care about basic at this point, we
fallback to session.
better to do:
Credentials creds = credsFromRequest(request);
if (creds == null) {
creds = credsFromSession(session);
}
if (creds != null) {
attempt login
if fail creds = null
}
if (creds != null) {
redirect
} else {
show login
}
and:
finally {
// clear session so we do not hold passwords
credsClearSession(session);
}
Line 57: if (userCredentials == null) {
Line 58: throw new AuthenticationException("Credentials
Required");
Line 59: }
Line 60: try {
http://gerrit.ovirt.org/#/c/37299/4/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOLoginServlet.java
File
backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOLoginServlet.java:
Line 19: if
(StringUtils.isEmpty(request.getParameter(SSOUtils.POST_ACTION_URL))) {
Line 20: throw new RuntimeException("No post action url found in
request.");
Line 21: }
Line 22: boolean enableBasicAuth = false;
Line 23: String strVal =
request.getSession().getServletContext().getInitParameter("enable-basic-auth");
the basic can be at application context, so we have one place to read, parse
and store.
Line 24: if (StringUtils.isNotEmpty(strVal)) {
Line 25: enableBasicAuth = Boolean.parseBoolean(strVal);
Line 26: }
Line 27: if (enableBasicAuth) {
http://gerrit.ovirt.org/#/c/37299/4/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java
File
backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/utils/SSOUtils.java:
Line 35: public static final String HEADER_AUTHORIZATION = "Authorization";
Line 36: public static final String USER_CREDENTIALS = "user_credentials";
Line 37: public static final String LOGIN_EXTERNAL_URI = "/login-external?";
Line 38: public static final String LOGIN_PHASE2_URI = "/login-phase2?";
Line 39: public static final String LOGIN_PHASE3_URI = "/login-phase3";
please be consistent about the trailing '?'.
Line 40:
Line 41: public static boolean isUserAuthenticated(HttpSession session) {
Line 42: return session.getAttribute(SSO_PRINCIPAL_RECORD_ATTR_NAME) !=
null;
Line 43: }
--
To view, visit http://gerrit.ovirt.org/37299
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If09285f0e6cd8909f21aa7e88ae1a3c1a30763c2
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Ravi Nori <[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
