Alexander Wels has posted comments on this change.
Change subject: core: Introduce new authentication interfaces
......................................................................
Patch Set 8:
(3 comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/authentication/AuthenticationFilter.java
Line 46: /**
Line 47: * Lazyly find all the profiles that that support negotiation and
store them reversed to simplify the creation of
Line 48: * the stacks of profiles later when processing requests.
Line 49: */
Line 50: private void findNegotiatingProfiles() {
Any particular reason you are not calling this in the init method of the filter?
Line 51: if (profiles == null) {
Line 52: synchronized(this) {
Line 53: if (profiles == null) {
Line 54: profiles = new ArrayList<AuthenticationProfile>(1);
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);
Why are we storing the stack in the session? Looking at the code there are 2
options.
1. I authenticate and the stack is removed from the session.
2. We don't authenticate and we have an empty stack object in the session, as
we have exhausted all the AuthenticationProfiles.
Why not just have a local variable that has the stack (or whatever structure
you use) and have it disappear after you leave the method?
Line 104: }
Line 105:
Line 106: while (!stack.isEmpty()) {
Line 107: // Resume the negotiation with the profile at the top of
the stack:
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();
If you look at the link that Martin provides and read the comments you will see
that Juan is right in this particular case. Doing the concat is exactly the
same as doing the String builder. So it really doesn't matter in this
particular case.
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);
--
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
