BasicAuthentication stores username in "username" attribute of Server…

[wolfy-j]

…RequestInterface.

[schnittstabil]

👍 It would also be nice for DigestAuthentication.

[oscarotero]

I like it, but suggest to no use $request->withAttribute().
Look how it's done in other middlewares like formatNegotiator: https://github.com/oscarotero/psr7-middlewares/blob/master/src/Middleware/FormatNegotiator.php#L111
It also provides a static method to recover the value. In this case it may be something like BasicAuthentication::getUsername($request).
This prevent conflicts with attributes stored by other middlewares.

[wolfy-j]

This seems like unnesessary complexity which does not really solve any real issue (like nested pipeline with same middleware) but add extra coupling to your library.

What if i will replace BasicAuth middleware one day in my application, should i go to every place i was getting username and remove your code?

If you really worrying about collisions, let's try to find some other simplier way. :)

[wolfy-j]

I think the best way to solve this issue is to let developer decide what attribute to be used (but still define some default value).

->withAttribute('auth.username')

If you wish to keep your original functions work you also store attribute name under your array:

'psr7-middlewares' => [
     BasicAuthenticationMiddleware::class => 'user attribute'
]

So it's backward compatible, simplier and even more flexible.

[oscarotero]

The discussion about this started here: https://www.reddit.com/r/PHP/comments/3keqr2/psr7_middleware_pieces/

The benefits of manage middleware attributes in this way is:

• No conflict with other middlewares
• There are many router middlewares saving the uri attributes as request attributes (FastRoute and AuraRouter in this package, for example). So, the path "/profile/{username}" overrides your "username" authentication attribute.
• There are middlewares using attributes of other middlewares. For example, Firewall needs ClientIp. If you change the attribute name used by ClientIp, Firewall cannot get this value (unless you configure firewall to use other attribute name).

So, the best way to handle all this issues is to avoid global variables in the attributes and let each middleware can manage its own attributes.

A way to change the middleware without need to change the code in all places you use its attribute is using a helper in your controller:

class HomeController extends BaseController
{
    public function ($request, $response)
    {
        $username = $this->getUsername($request);
        $response->getBody()->write('Hello '.$username);
    }
}

[wolfy-j]

Got it, let me correct my implementation.

[wolfy-j]

It's been fun to write test for digest auth :)

[oscarotero]

Great!
One last thing: there's the AuthenticationTrait used by both middleware. Maybe we should move some code to the trait in order to avoid code duplications.

[wolfy-j]

I'v been thinking about, the only common code here is constant and static method, i'm not huge fan of defining static methods in traits which are dependend on a constant which you can't declare. :) I think it can be solved by logical exception when constant not defined, up to you.

[oscarotero]

Ah, ok. So it's better to leave the code as is.
Thank you, great job. 😄