Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#3679 defect closed fixed (fixed)

new HTTP auth code throws away 1st segment of path

Reported by: philmayers Owned by:
Priority: normal Milestone:
Component: web Keywords: web, http, auth
Cc: Branch: branches/httpauth-wrapper-child-3679
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

The code here:

http://twistedmatrix.com/trac/browser/trunk/twisted/web/_auth/wrapper.py#L98

...seems to have a bug. Specifically the getChildWithDefault does nothing with "path" so the 1st segment of the URL under the HTTPAuthSessionWrapper is throw away. I think this line:

http://twistedmatrix.com/trac/browser/trunk/twisted/web/_auth/wrapper.py#L121

...should read:

d = util.DeferredResource(self._login(credentials))
d.addCallback(lambda res: res.getChildWithDefault(path))
return d

...the symptoms of this bug are that with this:

class root(resource.Resource):

# render code

class child(resource.Resource):

# render code

root.putChild('foo', child())

...if I hit http://thesite/foo I just get another copy of the root page, but if I hit http://thesite/anyrandomstring/foo I get a copy of the child resource.

Change History (8)

comment:1 Changed 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/httpauth-wrapper-child-3679

(In [26504]) Branching to 'httpauth-wrapper-child-3679'

comment:2 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner jknight deleted

Throwing away that first segment was indeed bad. :(

Another bad thing was that the tests should have been catching this. I decided that they were poorly written due to the use of direct calls to getChildWithDefault. I replaced these with getChildForRequest calls and things started to break. Fixing the wrapper to not discard the first path segment fixed them again.

comment:3 Changed 5 years ago by dreid

  • Keywords review removed
  • Owner set to exarkun

Yeah this is a reasonable solution to this. It's unfortunate that there isn't a better way to do this than mucking around with request, but I guess if twisted.web ever grows locateChild we can change this a little.

Awesome, merge it.

comment:4 follow-up: Changed 5 years ago by philmayers

For what it's worth, I'd solved it locally with a subclass like this, without fiddling with the request:

class HTTPAuthSessionWrapper(guard.HTTPAuthSessionWrapper):

"""This class fixes the bug in Twisted 8.2 with getChildWithDefault"""

def getChildWithDefault(self, path, request):

authheader = request.getHeader('authorization')
if not authheader:

return UnauthorizedResource(self._credentialFactories)

factory, respString = self._selectParseHeader(authheader)
if factory is None:

return UnauthorizedResource(self._credentialFactories)

try:

credentials = factory.decode(respString, request)

except credError.LoginFailed:

return UnauthorizedResource(self._credentialFactories)

except:

log.err(None, "Unexpected failure from credentials factory")
return ErrorPage(500, None, None)

else:

d = self._login(credentials)
d.addCallback(self._gcwd, path, request)
return util.DeferredResource(d)

def _gcwd(self, authed, path, request):

return authed.getChildWithDefault(path, request)

No idea if that passes the test.

You're right, it is easy with locateChild - I wrote a nevow-ised version of the the new HTTP stuff:

class HTTPAuthNevowWrapper:

implements(inevow.IResource)

def init(self, portal, credentialFactories):

self._portal = portal
self._credentialFactories = credentialFactories

def renderHTTP(self, ctx):

raise NotImplementedError

def locateChild(self, ctx, segments):

# get the request object from the context
request = inevow.IRequest(ctx)

authheader = request.getHeader('authorization')
if not authheader:

return UnauthorizedResource(self._credentialFactories), ()

factory, respString = self._selectParseHeader(authheader)
if factory is None:

return UnauthorizedResource(self._credentialFactories), ()

try:

credentials = factory.decode(respString, request)

except credError.LoginFailed:

return UnauthorizedResource(self._credentialFactories), ()

except:

log.err(None, "Unexpected failure from credentials factory")
return ErrorPage(500, None, None), ()


d = self._portal.login(credentials, None, inevow.IResource)
d.addCallback(self._locateChild, ctx, segments)
return d

def _locateChild(self, (iface, avatar, logout), ctx, segments):

request = inevow.IRequest(ctx)
request.notifyFinish().addCallback(lambda ign: logout())

return avatar.locateChild(ctx, segments)

def _selectParseHeader(self, header):

"""
Choose an C{ICredentialFactory} from C{_credentialFactories}
suitable to use to decode the given I{Authenticate} header.

@return: A two-tuple of a factory and the remaining portion of the

header value to be decoded or a two-tuple of C{None} if no
factory can decode the header value.

"""
elements = header.split(' ')
scheme = elements[0].lower()
for fact in self._credentialFactories:

if fact.scheme == scheme:

return (fact, ' '.join(elements[1:]))

return (None, None)

comment:5 in reply to: ↑ 4 Changed 5 years ago by philmayers

Bah, sorry. My version:

class HTTPAuthSessionWrapper(guard.HTTPAuthSessionWrapper):
    """This class fixes the bug in Twisted 8.2 with getChildWithDefault"""

    def getChildWithDefault(self, path, request):
        authheader = request.getHeader('authorization')
        if not authheader:
            return UnauthorizedResource(self._credentialFactories)

        factory, respString = self._selectParseHeader(authheader)
        if factory is None:
            return UnauthorizedResource(self._credentialFactories)
        try:
            credentials = factory.decode(respString, request)
        except credError.LoginFailed:
            return UnauthorizedResource(self._credentialFactories)
        except:
            log.err(None, "Unexpected failure from credentials factory")
            return ErrorPage(500, None, None)
        else:
            d = self._login(credentials)
            d.addCallback(self._gcwd, path, request)
            return util.DeferredResource(d)


    def _gcwd(self, authed, path, request):
        return authed.getChildWithDefault(path, request)

...and for purely reasons of interest, the nevow one:

class HTTPAuthNevowWrapper:

    implements(inevow.IResource)

    def __init__(self, portal, credentialFactories):
        self._portal = portal
        self._credentialFactories = credentialFactories

    def renderHTTP(self, ctx):
        raise NotImplementedError

    def locateChild(self, ctx, segments):
        # get the request object from the context
        request = inevow.IRequest(ctx)

        authheader = request.getHeader('authorization')
        if not authheader:
            return UnauthorizedResource(self._credentialFactories), ()

        factory, respString = self._selectParseHeader(authheader)
        if factory is None:
            return UnauthorizedResource(self._credentialFactories), ()
        try:
            credentials = factory.decode(respString, request)
        except credError.LoginFailed:
            return UnauthorizedResource(self._credentialFactories), ()
        except:
            log.err(None, "Unexpected failure from credentials factory")
            return ErrorPage(500, None, None), ()

        
        d = self._portal.login(credentials, None, inevow.IResource)
        d.addCallback(self._locateChild, ctx, segments)
        return d

    def _locateChild(self, (iface, avatar, logout), ctx, segments):
        request = inevow.IRequest(ctx)
        request.notifyFinish().addCallback(lambda ign: logout())

        return avatar.locateChild(ctx, segments)

    def _selectParseHeader(self, header):
        """
        Choose an C{ICredentialFactory} from C{_credentialFactories}
        suitable to use to decode the given I{Authenticate} header.

        @return: A two-tuple of a factory and the remaining portion of the
            header value to be decoded or a two-tuple of C{None} if no
            factory can decode the header value.
        """
        elements = header.split(' ')
        scheme = elements[0].lower()
        for fact in self._credentialFactories:
            if fact.scheme == scheme:
                return (fact, ' '.join(elements[1:]))
        return (None, None)

comment:6 Changed 5 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [26540]) Merge httpauth-wrapper-child-3679

Author: exarkun
Reviewer: dreid
Fixes: #3679

Fix a bug in the HTTP auth resource wrapper which caused it to disregard
the first segment of the request after the segment representing the wrapper.

Also change the unit tests to use the real resource traversal API instead of
bugfully imitating it.

comment:7 Changed 5 years ago by exarkun

#4018 was a duplicate of this.

comment:8 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.