Alexander Wels has posted comments on this change.
Change subject: core: Automatic login filter
......................................................................
Patch Set 1:
So I am trying to understand exactly what this filter does, and I am having a
hard time following it. This might be due to the fact that the doFilter method
is 100+ lines of code. After reading it a few times I think it does the
following:
1. get the Principal from the header.
2. If the Principal doesn't exist, follow the chain.
3. If the Principal exists, extract the profile(domain? after the @sign) and
check if it exists. if not return a not authorized.
4. If the profile exists, look up the associated Directory, if it doesn't
exists return a not authorized.
5. If it does exist, look up the user in the database, if the user doesn't
exist return a not authorized.
6. If the user exists add the user to the session.
Is it possible to split up the giant method into smaller pieces, each piece
implementing a step of the process something like this:
Principal principal = req.getUserPrincipal();
if (principal != null && !userLoggedInAlready()) {
try {
String loginName = getLoginFromPrinciple(principle);
String profileName = getProfilePromPrinciple(principle);
AuthenticationProfile profile = validateProfile(profileName);
DirectoryUser directoryUser = validateDirectoryUser(profile);
DbUser user = lookupDbUser(profile, directoryUser);
validateDbUser(user);
isUserAdmin(user);
attachUserToSession(user);
} catch (AuthenticationException e) {
rsp.sendError(HttpServletResponse.SC_UNAUTHORIZED);
}
} else {
chain.doFilter(req, rsp);
}
All the methods being called are basically everything in the giant method split
up into small methods, which throw the AuthenticationException when needed.
This will make the intend much clearer (I discovered I missed things while
writing the pseudo code). And it will allow you write unit tests for each
section easily as there is no dependency on previous code being run, you can
mock out a lot of the needed information.
--
To view, visit http://gerrit.ovirt.org/21023
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0b8cbee89b5bb96271c3706e9e75acbf2890a0b8
Gerrit-PatchSet: 1
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: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Martin Peřina <[email protected]>
Gerrit-Reviewer: Ravi Nori <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
