Add a logout feature to ldap security manager #2198

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

Member

Need to provide an interface to log out a CLdapSecManager user,
which will be needed for gh-2174 ('ECLWatch should provide LOGOUT
feature')

@RussWhitehead RussWhitehead Add a logout feature to ldap security manager
Need to provide an interface to log out a CLdapSecManager user,
which will be needed for gh-2174 ('ECLWatch should provide LOGOUT
feature')

Signed-off-by: William Whitehead <william.whitehead@lexisnexis.com>
ac60cbe

If this is code that should never be reached, you should use throwUnexpected() rather than UNIMPLEMENTED;

If it's code that CAN be reached, you should really implement it before submitting the pull request (unless it's something we DO intend to implement sometime but NOT in the next release...

Owner

The reason I chose UNIMPLEMENTED was for consistency, since there are 13 other methods in this same class and file that use the same (I am guessing that you were not able to see them because the viewer by default only shows a few lines of code around the changes) . I suppose I could change them all to ThrowUnexpected, although it should be noted that both of these throw an exception

define throwUnexpected() throw MakeStringException(9999, "Internal Error at %s(%d)", FILE, LINE)

define UNIMPLEMENTED throw MakeStringException(-1, "UNIMPLEMENTED feature at %s(%d)", FILE, LINE)

I know they both throw an exception, but they imply different things. And I know we have in the past been a little sloppy about which one we use, on occasion. But I don't think that's a good reason for not correcting things when we spot that they are wrong.

If the others should also be considered as unexpected rather than unimplemented, then they should be changed too.

Owner

@afishbeck @wangkx Please review

Is logout the right term for this functionality? Seems like its really just clearing the cache. The main reason I ask is because there may be a chance we would want to add real session type functionality to a security manager at some point.

Owner

I agree, since you don't really "log in" it doesn't make sense to log out. Now accepting nominations for a better name....

Member
wangkx commented May 1, 2012

Should we have a better design of the login/logout processes? ESP sends requests for authentication check, login, and logout to a security manager. The security manager handles those requests by calling ldap related and/or non-ldap related functions. LDAP works for the security manager...

Member

I think the design should come out of a new issue, or the other issue that you have open (the one that talks about ESP log out support). We need to support the general concept of sessions... but I don't think it's the responsibility of the security managers to handle sessions. Even database security implementations would probably separate sessions from authentication and authorization.

To tell you the truth I don't think this functionality of removing someone from the cache is even needed.

Member

Closing pull request, since we need more discussion...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment