Les, 1. Sure thing. 2. I don't think this is a good idea. From what I understand, this string value is what a user would need to specify in their logging pattern to access this parameter. As such, a really long identifier would seem to impede the readability of that pattern. Note that this is not storing something in the thread context, but rather in the MDC. 3. Here, it seems that the goal of adding something to the MDC is to make it available as a String to the logging framework. So the goal (I think, I took this directly from the patch provided) is to provide a useful String representation of the subject. Perhaps we could expand on that. We could add several values to MDC - shiroPrimaryPrincipal, shiroAllPrincipals, shiroIsAuthenticated, shiro.isRemembered, shiro.isRunAs, shiro.previousPrimaryPrincipal, shiro.previousPrincipals. Or, we could pare it down, and format some interesting strings - one that includes all principals and one that only includes the primary one. This could indicate the "is" values and the runAs principal info. 4. I can certainly do this. However, do you think it wise to propagate exceptions from MDC, or should we simply swallow and log them?
Thanks for the feedback! -Jared On Tue 15 Jan 2013 11:41:50 AM CST, Les Hazlewood wrote: > Hi Jared, > > I have some questions about this commit: > > 1. Can we rename the constant to be MDC_SUBJECT_KEY? We typically > prefix more specific information to names in variables/constants/class > names. > 2. Can we make it a qualified key like the other constants in that > class? e.g. ThreadContext.class.getName() + "_MDC_SUBJECT_KEY"; > 3. In the bind implementation you put only the primary principal in > the MDC - any reason why not the Subject as the constant name > indicates? If not putting in the subject, shouldn't we call the > constant MDC_PRIMARY_PRINCIPAL_KEY? > 4. MDC operations can throw exceptions - can you ensure that the > Shiro binding logic is either executed first or put in a finally block > to ensure guaranteed execution? > > Thanks for any insight! > > Best, > > -- > Les Hazlewood | @lhazlewood > CTO, Stormpath | http://stormpath.com | @goStormpath | 888.391.5282 > PMC Chair, Apache Shiro: http://shiro.apache.org > Stormpath wins GigaOM Structure Launchpad Award! http://bit.ly/MvZkMk > > > On Tue, Jan 15, 2013 at 5:32 AM, <[email protected] > <mailto:[email protected]>> wrote: > > Author: jbunting > Date: Tue Jan 15 13:32:12 2013 > New Revision: 1433407 > > URL: http://svn.apache.org/viewvc?rev=1433407&view=rev > Log: > SHIRO-391 -- ThreadContext.bind(Subject) also binds the subject's > primary principal to slf4j's MDC > > Modified: > > shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java > > > shiro/trunk/support/guice/src/test/java/org/apache/shiro/guice/ShiroSessionScopeTest.java > > Modified: > shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java > URL: > > http://svn.apache.org/viewvc/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java?rev=1433407&r1=1433406&r2=1433407&view=diff > > ============================================================================== > --- > shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java > (original) > +++ > shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java > Tue Jan 15 13:32:12 2013 > @@ -22,6 +22,7 @@ import org.apache.shiro.mgt.SecurityMana > import org.apache.shiro.subject.Subject; > import org.slf4j.Logger; > import org.slf4j.LoggerFactory; > +import org.slf4j.MDC; > > import java.util.HashMap; > import java.util.Map; > @@ -52,6 +53,12 @@ public abstract class ThreadContext { > public static final String SECURITY_MANAGER_KEY = > ThreadContext.class.getName() + "_SECURITY_MANAGER_KEY"; > public static final String SUBJECT_KEY = > ThreadContext.class.getName() + "_SUBJECT_KEY"; > > + /** > + * The key of the subject in SLF4Js Mapped Diagnostic Context > ({@link MDC}). This can be used to > + * show the subject in the logs. The subject is displayed in > the logs using the pattern <pre>%X{shiroSubject}</pre>. > + */ > + private static final String SUBJECT_KEY_MDC = "shiroSubject"; > + > private static final ThreadLocal<Map<Object, Object>> > resources = new InheritableThreadLocalMap<Map<Object, Object>>(); > > /** > @@ -285,6 +292,7 @@ public abstract class ThreadContext { > public static void bind(Subject subject) { > if (subject != null) { > put(SUBJECT_KEY, subject); > + MDC.put(SUBJECT_KEY_MDC, > String.valueOf(subject.getPrincipal())); > } > } > > @@ -303,6 +311,7 @@ public abstract class ThreadContext { > * @since 0.2 > */ > public static Subject unbindSubject() { > + MDC.remove(SUBJECT_KEY_MDC); > return (Subject) remove(SUBJECT_KEY); > } > > > Modified: > > shiro/trunk/support/guice/src/test/java/org/apache/shiro/guice/ShiroSessionScopeTest.java > URL: > > http://svn.apache.org/viewvc/shiro/trunk/support/guice/src/test/java/org/apache/shiro/guice/ShiroSessionScopeTest.java?rev=1433407&r1=1433406&r2=1433407&view=diff > > ============================================================================== > --- > > shiro/trunk/support/guice/src/test/java/org/apache/shiro/guice/ShiroSessionScopeTest.java > (original) > +++ > > shiro/trunk/support/guice/src/test/java/org/apache/shiro/guice/ShiroSessionScopeTest.java > Tue Jan 15 13:32:12 2013 > @@ -34,14 +34,14 @@ public class ShiroSessionScopeTest { > public void testScope() throws Exception { > Subject subject = createMock(Subject.class); > try { > - ThreadContext.bind(subject); > - > final Key<SomeClass> key = Key.get(SomeClass.class); > Provider<SomeClass> mockProvider = > createMock(Provider.class); > Session session = createMock(Session.class); > > SomeClass retuned = new SomeClass(); > > + > expect(subject.getPrincipal()).andReturn("testUser").anyTimes(); > + > expect(subject.getSession()).andReturn(session); > expect(session.getAttribute(key)).andReturn(null); > expect(mockProvider.get()).andReturn(retuned); > @@ -52,6 +52,8 @@ public class ShiroSessionScopeTest { > > replay(subject, mockProvider, session); > > + ThreadContext.bind(subject); > + > ShiroSessionScope underTest = new ShiroSessionScope(); > > // first time the session doesn't contain it, we > expect the provider to be invoked > > >
