On Tue, Aug 25, 2009 at 6:38 AM, Les Hazlewood<[email protected]> wrote:
> 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.
Based on that and without knowing all the details, I'd scrap it
immediately. You can always resurrect it later, but where ever
possible it makes sense to limit the user facing surface area of the
API as much as possible since it's more difficult to remove than add.
I see that Subject.login() now works again, but RememberMe is broken,
because the new DefaultSecurityManager.createSubject(Map context)
doesn't call getRememberedIdentity at any point. Compare to the old
createSubject() that does:
if (session != null) {
if
(session.getAttribute(SessionSubjectBinder.PRINCIPALS_SESSION_KEY)
== null) {
remembered = getRememberedIdentity();
}
}
Should I open an issue or are you still working on it to complete this
refactoring?
Kalle
> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
