Alon Bar-Lev has posted comments on this change.
Change subject: aaa: add support for basic athentication
......................................................................
Patch Set 5:
(7 comments)
one issue with the unauthorized, I think.
also, can we rename the servlets to LoginPhase1, LoginPhase2, ... names? the
first url name which is the important one (as it is the interface) will be
determine in the web.xml.
fine tuning of configuration, see comments.
next phase, I hope that we can perform our negotiation sequence within the
external url only.
sequences to verify are:
1. without apache:
enable basic
try to login
press esc within browser credentials dialog
expected: login form is presented, enter user/password and login
2. with apache: same as (1)
http://gerrit.ovirt.org/#/c/37299/5/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 24: response.setHeader("WWW-Authenticate", "Basic realm=\"" +
realm + "\"");
Line 25: response.sendError(HttpServletResponse.SC_UNAUTHORIZED);
Line 26: } else {
Line 27:
request.getSession(true).setAttribute(SSOUtils.USER_CREDENTIALS, credentials);
Line 28: response.sendRedirect(request.getContextPath() +
SSOUtils.LOGIN_PHASE2_URI);
can't most of redirects be relative?
Line 29: }
Line 30: }
Line 31:
Line 32: private boolean containsUserCredentials(HttpServletRequest
request) {
Line 28: response.sendRedirect(request.getContextPath() +
SSOUtils.LOGIN_PHASE2_URI);
Line 29: }
Line 30: }
Line 31:
Line 32: private boolean containsUserCredentials(HttpServletRequest
request) {
not needed.
Line 33: return
StringUtils.isNotEmpty(request.getHeader(SSOUtils.HEADER_AUTHORIZATION));
Line 34: }
Line 35:
Line 36: private Credentials getUserCredentials(HttpServletRequest request)
{
Line 40: String[] creds = new String(
Line 41:
Base64.decodeBase64(request.getHeader(SSOUtils.HEADER_AUTHORIZATION).substring("Basic".length())),
Line 42: Charset.forName("UTF-8")
Line 43: ).split(":", 2);
Line 44: credentials = translateUser(creds[0], creds[1]);
please check that creds.length() is 2 before access, do not allow exception in
this case.
Line 45: }
Line 46: return credentials;
Line 47: }
Line 48:
Line 52:
Line 53: if (separator != -1) {
Line 54: credentials = new Credentials();
Line 55: credentials.setPassword(password);
Line 56: credentials.setProfile(user.substring(separator + 1));
you still must check if profile is valid, if not, assume no credentials (return
null).
Line 57: credentials.setUsername(user.substring(0, separator));
Line 58: }
Line 59:
Line 60: return credentials;
http://gerrit.ovirt.org/#/c/37299/5/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:
same... please remove the Engine prefix from previous patches as well, we do
not need it.
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;
http://gerrit.ovirt.org/#/c/37299/5/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOContextListener.java
File
backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/SSOContextListener.java:
Line 22: ctx.setAttribute(SSOUtils.SSO_LOCAL_CONFIG, localConfig);
Line 23: ctx.setAttribute(SSOUtils.SSO_PROFILES,
AuthenticationUtils.getAvailableProfiles(ctx));
Line 24:
Line 25: boolean enableBasicAuth = false;
Line 26: String strVal = ctx.getInitParameter("enable-basic-auth");
actually we need three parameters:
1. if we use external authentication, redirect to the external url, this can be
anything externally set. default: yes
2. if we want to accept basic authorization headers at all. default: yes.
3. if we want to activate our basic authorization servlet to prompt for
credentials or not. default: no.
the above default will not prompt for basic credentials but will accept them if
apache is configured like so.
Line 27: if (StringUtils.isNotEmpty(strVal)) {
Line 28: enableBasicAuth = Boolean.parseBoolean(strVal);
Line 29: }
Line 30: ctx.setAttribute(SSOUtils.ENABLE_BASIC_AUTH, enableBasicAuth);
http://gerrit.ovirt.org/#/c/37299/5/backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/UnauthorizedServlet.java
File
backend/manager/modules/enginesso/src/main/java/org/ovirt/engine/core/sso/servlets/UnauthorizedServlet.java:
Line 14:
Line 15: @Override
Line 16: protected void service(HttpServletRequest request,
HttpServletResponse response)
Line 17: throws ServletException, IOException {
Line 18: response.sendRedirect(request.getContextPath() +
SSOUtils.LOGIN_EXTERNAL_URI);
this should go to login I think...
1. redirect to external
2. apache takes over
3. user press escape
4. we reach here
5. we should present login and not redirect to 1 and loop
Line 19: }
--
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: 5
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
