Yep you're right - it still is not fixed just yet. I didn't have the time late yesterday to make that fix, but hopefully I can focus on that first thing today.
On the SecurityManager comment - it is an interesting question. I think it might be a good idea to pass in the Subject instance to the login method in addition to the authentication token - that way the SecurityManager could execute logic based on that particular subject. For example, during login, if a RememberMeManager is available, it will be invoked to remember that subject. Currently that logic is based on the AuthenticationInfo only, but in the future, it could be valuable to have a handle to the Subject in case other things need to be done for that Subject - things we can't foresee now, but certainly might appear in the future. Such is the benefit of coarse-grained method calls - we could easily react to change without changing the public-facing API. The logout method already takes this approach, and I'm thinking it should be the same for login as well. Thoughts? Your thoughts also make me wonder about the need for the org.apache.shiro.mgt.SubjectBinder interface. It was a new addition after 0.9, so it can be scrapped if we no longer need it since 1.0 hasn't been released yet. It was added to allow custom binding and retrieval of Subject instances after they had been created - to allow someone to save that Subject state somewhere if necessary so it can be retrieved later on. But its method signatures imply ThreadLocal-based behavior, which isn't very clean. On Tue, Aug 25, 2009 at 12:51 AM, Kalle Korhonen<[email protected]> wrote: > Ok, I see that after your latest changes (in r807476) the login itself > in DefaultSecurityManager succeeds, but after that login does: > Subject subject = createSubject(token, info); > bind(subject); > return subject; > > which will cause the same: > Caused by: java.lang.IllegalStateException: Subject context map must > contain a javax.servlet.ServletRequest instance to support Web Subject > construction. > at > org.apache.shiro.web.mgt.DefaultWebSubjectFactory.getServletRequest(DefaultWebSubjectFactory.java:40) > at > org.apache.shiro.web.mgt.DefaultWebSubjectFactory.createSubject(DefaultWebSubjectFactory.java:70) > at > org.apache.shiro.mgt.DefaultSecurityManager.getSubject(DefaultSecurityManager.java:403) > at > org.apache.shiro.mgt.DefaultSecurityManager.createSubject(DefaultSecurityManager.java:274) > at > org.apache.shiro.mgt.DefaultSecurityManager.login(DefaultSecurityManager.java:370) > at > org.apache.shiro.subject.DelegatingSubject.login(DelegatingSubject.java:245) > > Should the SecurityManager return Subject at all anymore if the > creation of it is managed elsewhere in the builders? > > Kalle > > > On Mon, Aug 24, 2009 at 12:07 PM, Kalle > Korhonen<[email protected]> wrote: >> I'm ahead of you - I already added WebUtils.bind.. calls in my >> implementation corresponding to ShiroFilter (that's the first issue I >> reported and worked around). The login issue still stands. >> >> Kalle >> >> >> On Mon, Aug 24, 2009 at 11:51 AM, Les Hazlewood<[email protected]> wrote: >>> Ah yes, you're right - sorry about that. I'll see if I can get that >>> working now. It is definitely a high priority to have this working >>> asap. >>> >>> On Mon, Aug 24, 2009 at 2:27 PM, Kalle >>> Korhonen<[email protected]> wrote: >>>> Naturally I already did what the ShiroFilter does to get any Subject >>>> checks going. However Subject.login() doesn't work because of the >>>> reasons mentioned in my previous post (see the stacktrace - it enters >>>> DefaultSecurityManager.createSubject where the context is (re-)created >>>> and it won't have the request/response objects in it). >>>> >>>> Kalle >>>> >>>> >>>> On Mon, Aug 24, 2009 at 11:11 AM, Les Hazlewood<[email protected]> >>>> wrote: >>>>> Aha - you've encountered something that wasn't fixed this weekend yet >>>>> - the SecurityManager.login method signature would need to change to >>>>> take in a Subject argument in addition to the AuthenticationToken. >>>>> Naturally this would be hidden from Subject API end-users, but >>>>> represents the final change for this type of state-management: >>>>> >>>>> Currently the SecurityManager implementations assume that an existing >>>>> Subject is bound to the currently executing thread and can rely on the >>>>> ThreadContext. It would then acquire the existing Subject from the >>>>> thread, and call login on that instance. In your example, you use the >>>>> subject immediately after building it - without binding it to the >>>>> thread first. >>>>> >>>>> A SecurityManager API change that accepts the Subject as a parameter >>>>> would eliminate these types of problems since it wouldn't have to >>>>> acquire the Subject from some other location anymore. There are other >>>>> benefits too to keeping that method signature coarse-grained with the >>>>> Subject, but I digress... >>>>> >>>>> In the meantime, you should be able to do what the ShiroFilter does >>>>> and ensure that a Subject is available immediately on the thread after >>>>> construction. Then you could use SecurityUtils everywhere in your >>>>> code after that: >>>>> >>>>> Subject subject = new WebSubjectBuilder(securityManager, request, >>>>> response).build(); >>>>> >>>>> //this API is actively changing and will undergo another change today >>>>> or tomorrow to not accept a request/response since the Subject will >>>>> already have those. >>>>> ThreadStateManager threadState = new WebThreadStateManager(subject, >>>>> request, response); >>>>> threadState.bindThreadState(); >>>>> >>>>> subject.login(authcToken); >>>>> >>>>> If that doesn't work, feel free to reply and I'll get it straightened out. >>>>> >>>>> - Les >>>>> >>>>> On Mon, Aug 24, 2009 at 1:28 PM, Kalle >>>>> Korhonen<[email protected]> wrote: >>>>>> Les, thanks I really appreciate you taking time to talk through the >>>>>> changes - mostly it verifies what I was being able to deduce from the >>>>>> dffs. Yea, sounds like more tests are needed. In the meantime, can you >>>>>> comment on where this is going wrong? >>>>>> >>>>>> I'm trying to call Subject.login. Even if I change it to using the new >>>>>> builder from SecurityUtils: >>>>>> // >>>>>> SecurityUtils.getSubject().login(authenticationToken); >>>>>> (new WebSubjectBuilder(securityManager, request, >>>>>> response).build()).login(authenticationToken); >>>>>> >>>>>> And I get a stack trace: >>>>>> Caused by: java.lang.IllegalStateException: Subject context map must >>>>>> contain a javax.servlet.ServletRequest instance to support Web Subject >>>>>> construction. >>>>>> at >>>>>> org.apache.shiro.web.mgt.DefaultWebSubjectFactory.getServletRequest(DefaultWebSubjectFactory.java:40) >>>>>> at >>>>>> org.apache.shiro.web.mgt.DefaultWebSubjectFactory.createSubject(DefaultWebSubjectFactory.java:70) >>>>>> at >>>>>> org.apache.shiro.mgt.DefaultSecurityManager.getSubject(DefaultSecurityManager.java:405) >>>>>> at >>>>>> org.apache.shiro.mgt.DefaultSecurityManager.createSubject(DefaultSecurityManager.java:275) >>>>>> at >>>>>> org.apache.shiro.mgt.DefaultSecurityManager.login(DefaultSecurityManager.java:372) >>>>>> at >>>>>> org.apache.shiro.subject.DelegatingSubject.login(DelegatingSubject.java:245) >>>>>> at xxx.web.pages.SignIn.onValidateForm(SignIn.java:88) >>>>>> >>>>>> If you look at DefaultSecurityManager.createSubject() operations, they >>>>>> just create the context from scratch (see my previous email). So where >>>>>> is the problem - should it use the builder or should the >>>>>> DefaultWebSecurityManager override these operations or something else? >>>>>> >>>>>> Kalle >>>>>> >>>>>> >>>>>> On Mon, Aug 24, 2009 at 7:32 AM, Les Hazlewood<[email protected]> >>>>>> wrote: >>>>>>> Sounds like we need some test cases for this - it appears that the >>>>>>> exception you're seeing is if there is no Subject accessible to the >>>>>>> thread. That shouldn't be the case since the ShiroFilter should have >>>>>>> bound the subject to the thread via the WebSubjectBuilder and >>>>>>> WebThreadStateManager usage. >>>>>>> >>>>>>> On Mon, Aug 24, 2009 at 1:56 AM, Kalle >>>>>>> Korhonen<[email protected]> wrote: >>>>>>>> No.. this can't be right - for example calling >>>>>>>> SecurityUtils.getSubject().login(authenticationToken) results in: >>>>>>>> java.lang.IllegalStateException: Subject context map must contain a >>>>>>>> javax.servlet.ServletRequest instance to support Web Subject >>>>>>>> construction. DefaultSecurityManager's createSubject operations create >>>>>>>> Subject context map from scratch, and obviously it won't have the >>>>>>>> required objects in the context. Les, care to clarify your refactoring >>>>>>>> plan and how this is supposed to work? >>>>>>>> >>>>>>>> Kalle >>>>>>>> >>>>>>>> >>>>>>>> On Sat, Aug 22, 2009 at 11:22 PM, Kalle >>>>>>>> Korhonen<[email protected]> wrote: >>>>>>>>> I see the internals of Shiro have been changed quite a bit in r806735. >>>>>>>>> ShiroFilter.bind() now does: >>>>>>>>> Subject subject = new WebSubjectBuilder(getSecurityManager(), >>>>>>>>> request, response).build(); >>>>>>>>> WebThreadStateManager threadState = new >>>>>>>>> WebThreadStateManager(subject, request, response); >>>>>>>>> threadState.bindThreadState(); >>>>>>>>> >>>>>>>>> which for Tapestry integration I'm working on results in: >>>>>>>>> >>>>>>>>> java.lang.IllegalStateException: No ServletRequest found in >>>>>>>>> ThreadContext. Make sure WebUtils.bind() is being called. (typically >>>>>>>>> called by ShiroFilter) This could also happen when running >>>>>>>>> integration tests that don't properly call WebUtils.bind(). >>>>>>>>> at >>>>>>>>> org.apache.shiro.web.WebUtils.getRequiredServletRequest(WebUtils.java:351) >>>>>>>>> at >>>>>>>>> org.apache.shiro.web.session.ServletContainerSessionManager.doGetSession(ServletContainerSessionManager.java:69) >>>>>>>>> at >>>>>>>>> org.apache.shiro.session.mgt.AbstractSessionManager.getSession(AbstractSessionManager.java:246) >>>>>>>>> at >>>>>>>>> org.apache.shiro.session.mgt.AbstractSessionManager.checkValid(AbstractSessionManager.java:265) >>>>>>>>> at >>>>>>>>> org.apache.shiro.mgt.SessionsSecurityManager.checkValid(SessionsSecurityManager.java:294) >>>>>>>>> at >>>>>>>>> org.apache.shiro.mgt.DefaultSecurityManager.getSession(DefaultSecurityManager.java:196) >>>>>>>>> at >>>>>>>>> org.apache.shiro.mgt.DefaultSecurityManager.resolveSessionIfNecessary(DefaultSecurityManager.java:437) >>>>>>>>> at >>>>>>>>> org.apache.shiro.mgt.DefaultSecurityManager.getSubject(DefaultSecurityManager.java:403) >>>>>>>>> at >>>>>>>>> org.apache.shiro.subject.SubjectBuilder.build(SubjectBuilder.java:95) >>>>>>>>> at >>>>>>>>> org.trailsframework.security.services.SecurityConfiguration.service(SecurityConfiguration.java:87) >>>>>>>>> >>>>>>>>> I.e. WebSubject requires the request is already bound to thread >>>>>>>>> context, but WebThreadStateManager (that's supposed to bind it) >>>>>>>>> requires a subject to exist. If I call WebUtils.bind(request) >>>>>>>>> before instantiating a WebSubjectBuilder, everything works. Les, is it >>>>>>>>> expected I still need to bind the request/response separately or >>>>>>>>> perhaps this is a defect/refactoring still in progress? >>>>>>>>> >>>>>>>>> Kalle >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
