Improve authz and authn mechanics #43

Firehed · 2018-06-04T02:09:08Z

Today, an EndpointInterface must implement an authorize(RequestInterface): self method to integrate both authentication and authorization logic. In practice, the mechanics of this have been tedious to use - even with the provided traits - typically resulting in a lot of misdirection in code flow.

The goal of this is to support centralized authn and authz, since they tend to have nearly-identical requirements across all requests and endpoints, resulting in more straightforward and less duplicated code.

In 3.1.0 (or the next minor version):

Endpoints may choose to implement AuthenticatedEndpointInterface, which extends EndpointInterface. When the routed endpoint does so, the AuthenticationProviderInterface will run, being given a ServerRequestInterface.

Implementations should look at request headers (typically) for e.g. a Bearer token, cookie, HTTP basic auth, etc., and return an AuthenticationContainerInterface. The returned AuthenticationContainerInterface will be provided to the endpoint before execute() is called. Additionally, if an AuthorizationProviderInterface is given to the dispatcher, it will be run AFTER authentication but BEFORE execute; If authn fails, execute will not run.

AuthorizationProviderInterface implementations have complete freedom around how resource protection is performed - it could be a simple class map, RBAC, asking for additional data from the protected endpoint (which should use its own instanceof checks), etc.

Endpoints implementing the new interface SHOULD switch their authentication trait to None (if one is in use) to avoid redundant authentication work.

In 4.0.0:

authenticate() will be removed from the EndpointInterface, and no longer called even if present.

Firehed

Also, add docs!

Firehed · 2018-06-04T02:12:50Z

src/Dispatcher.php

@@ -52,6 +54,16 @@ public function addResponseMiddleware(callable $callback): self
        return $this;
    }

+    public function setAuthenticationProvider(Interfaces\AuthenticationProviderInterface $provider)


It might be preferable to combine these two setters into one, requiring either both or neither. It will rarely make sense to authenticate without authorizing, and a misconfiguration could result in resource protection failing open.

Firehed · 2018-06-04T02:13:31Z

src/Dispatcher.php

+        if ($isSRI && $this->authenticationProvider && $endpoint instanceof Interfaces\AuthenticatedEndpointInterface) {
+            $auth = $this->authenticationProvider->authenticate($this->request);
+            $endpoint->setAuthentication($auth);
+            if ($this->authorizationProvider) {


Per above - this check would be removed, instead always running authorize.

Firehed · 2018-06-04T02:14:09Z

src/Interfaces/AuthenticationContainerInterface.php

+use Psr\Container\ContainerInterface;
+
+/**
+ * SHOULD use FQCN for get/has - etc


Fill out a proper description here - but it's primarily for best practices.

Note: the driving factor here is that authentication isn't always as simple as "do I have a user?", which is why this is so generic. Use cases:

Provide time of authentication to the authorizer

Provide method(s)/factor(s) of authentication to the authorizer

Provide both the user and application from a bearer token

Allow for weird admin panel type work, such as user impersonation

Firehed · 2018-06-04T02:15:57Z

src/Interfaces/AuthenticationContainerInterface.php

+ */
+interface AuthenticationContainerInterface extends ContainerInterface
+{
+    public function isAuthenticated(): bool;


This may be too simple; instead, it may be better to just kick all of the logic into the container values.

Given that, it would be possible to drop this interface entirely and rename the consumers, but the additional semantic value from the name might be worthwhile.

See also: AuthenticationProvider comment

Firehed · 2018-06-04T02:23:30Z

src/Interfaces/AuthorizationProviderInterface.php

+
+interface AuthorizationProviderInterface
+{
+    public function authorize(


This is somewhat limited since it performs all logic pre-execute. This means that in some situations, the endpoint will need to perform additional logic - namely, IDOR prevention. Not sure if there's much to do here beyond best practices docs.

Firehed · 2018-06-04T02:27:17Z

src/extras.php

+            }
+        }
+        return new class implements AuthenticationInterface {
+            public function isAuthenticated(): bool


~~This needs get/has methods, too.~~ Delete this, (re)throw

Firehed · 2018-06-04T02:33:46Z

src/Interfaces/AuthenticationProviderInterface.php

+
+interface AuthenticationProviderInterface
+{
+    public function authenticate(ServerRequestInterface $request): AuthenticationContainerInterface;


How to indicate no authentication was performed (or successful) is currently UB. At the very least, a doc comment should be added.

From a fail-closed perspective, I'm thinking it should throw a (new) AuthenticationException if auth fails in any way - malformed or missing headers, signature verification failing, etc. If an endpoint is requesting authn, there's no use case for returning a not-authn result.

This means that, beyond documentation, an AuthenticationProviderInterfaceTestTrait should be added for success and fail cases, with the latter asserting an appropriate exception is thrown (An abstract dataprovider should suffice)

Firehed · 2018-06-04T02:36:43Z

src/Interfaces/AuthenticationProviderInterface.php

+
+use Psr\Http\Message\ServerRequestInterface;
+
+interface AuthenticationProviderInterface


(generally, I don't like how verbose these names are)

Firehed · 2018-06-04T02:42:47Z

src/Dispatcher.php

+        $this->authenticationProvider = $provider;
+    }
+
+    public function setAuthorizationProvider(Interfaces\AuthorizationProviderInterface $provider)


(and this one)

Firehed · 2018-06-04T02:43:08Z

src/Dispatcher.php

@@ -143,7 +155,17 @@ public function dispatch(): ResponseInterface
            );
        }

+        $isSRI = $this->request instanceof ServerRequestInterface;
+        // Soon: issue a warning if !isSRI


This warning would be a separate pull

codecov · 2018-06-06T07:55:39Z

Codecov Report

Merging #43 into master will increase coverage by 0.01%.
The diff coverage is 96.15%.

@@             Coverage Diff              @@
##             master      #43      +/-   ##
============================================
+ Coverage     95.94%   95.96%   +0.01%     
- Complexity       96      109      +13     
============================================
  Files            23       24       +1     
  Lines           296      322      +26     
============================================
+ Hits            284      309      +25     
- Misses           12       13       +1

Impacted Files	Coverage Δ	Complexity Δ
src/Container.php	`100% <100%> (ø)`	`4 <4> (?)`
src/Dispatcher.php	`98.18% <94.73%> (-0.72%)`	`39 <1> (+9)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b01a56...f0c7b26. Read the comment docs.

Firehed · 2018-06-07T03:45:26Z

Did a quick test of integrating this into a real application

A first rough pass took about an hour, which included only basic additions, some copy-paste, and no cleanup or testing. And that time is still a bit misleading since some in-application structures made it easier and more centralized (endpoints were implementing an internal interface which itself extended the EndpointInterface; and used an auth trait in the same way - which influenced the design somewhat), but is still incredibly promising. The total diff (excluding composer.lock obviously) was only about 100 lines.

What worked:

Single injection site in the front controller
Implementing the authentication logic
Implementing the authorization logic (though this wasn't tested super well)

What was annoying:

No concrete Container implementation/could not use existing PSR one (no surprise here)
Similarity of Authorization and Authentication names (no surprise here)
Working on the latest mbp with a semi-broken keyboard 💻⌨️🔨

Thoughts/takeaways:

Either make a container implementation or just use base PSR
Autodetecting the authn/authz providers via an injected container (or supported in the config) would be nice
Combining the two providers into one might be interesting. Technically nothing is stopping you from doing this (e.g. setAuthProviders($p, $p) and having one class implement both interfaces). It may make the names a bit easier, but comes at the cost of somewhat less flexibility in (easily) trying multiple different strategies for each component.

…easier

Firehed · 2018-06-10T18:18:00Z

Last round of changes was intended to address some annoyances from implementation:

No special ContainerInterface wrapper anymore, just a basic PSR container
A "wrap this array in a PSR container" implementation has been added, for the (very rare?) case of using this framework without an existing container

Firehed · 2018-06-14T21:46:59Z

Only remaining things I found during a real implementation were:

Premade test cases for Authn/Authz ProviderInterface implementations would be nice
Premade Authn implementations (a la Traits\Authentication\BearerToken)

Firehed · 2018-06-15T04:42:20Z

From usability and functionality standpoints, I'm pretty happy with this when it comes to actually using the new tools. There's always room for improvement especially around adding more helpers but I don't want to block moving forward on that (and it will be easier to review and test those separately)

First loose pass at making auth less painful

be4bedf

Firehed added this to the 3.1.0 milestone Jun 4, 2018

Firehed commented Jun 4, 2018

View reviewed changes

Firehed mentioned this pull request Jun 4, 2018

Remove authenticate method from EndpointInterface, stop calling it #45

Closed

Firehed added 9 commits June 3, 2018 23:49

Remove accidental code

1c8b9a6

Merge branch 'master' into auth

d0c0e15

Combine injection of authn/authz providers

4123c1c

Remove comment that was addressed in #48

b857683

Remove unnecessary check since authn and authz are provided as a pair

03596b2

Define how authentication should work

7e8b781

Rename many things

b4fb412

Incremental work and tests

5dcc63d

Unstage extras for now, they can come separately

7941de2

Test setAuthProviders

6a01609

Firehed added 3 commits June 10, 2018 10:35

Remove auth trait for now

3ed896a

Just use PSR\Container

5751f83

Package a small just-array-wrapper container to make auth marginally …

60f43a7

…easier

Firehed added 3 commits June 10, 2018 12:11

Allow auto-detection of auth pieces

593a162

Update README

8e87d43

Handle case of providing no container

adfe8d5

Firehed mentioned this pull request Jun 10, 2018

Improve middleware situation #44

Closed

Fix typo

f0c7b26

Firehed changed the title ~~[WIP] Improve authz and authn mechanics~~ Improve authz and authn mechanics Jun 14, 2018

Firehed merged commit 6b9cebc into master Jun 15, 2018

Firehed deleted the auth branch June 15, 2018 04:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve authz and authn mechanics #43

Improve authz and authn mechanics #43

Firehed commented Jun 4, 2018

Firehed left a comment

Firehed Jun 4, 2018

Firehed Jun 4, 2018

Firehed Jun 4, 2018

Firehed Jun 4, 2018

Firehed Jun 4, 2018

Firehed Jun 4, 2018

Firehed Jun 4, 2018

Firehed Jun 4, 2018

Firehed Jun 4, 2018

Firehed Jun 4, 2018

Firehed Jun 4, 2018

codecov bot commented Jun 6, 2018 •

edited

Loading

Firehed commented Jun 7, 2018

Firehed commented Jun 10, 2018

Firehed commented Jun 14, 2018

Firehed commented Jun 15, 2018


		use Psr\Http\Message\ServerRequestInterface;

		interface AuthenticationProviderInterface

Improve authz and authn mechanics #43

Improve authz and authn mechanics #43

Conversation

Firehed commented Jun 4, 2018

Firehed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 6, 2018 • edited Loading

Codecov Report

Firehed commented Jun 7, 2018

Firehed commented Jun 10, 2018

Firehed commented Jun 14, 2018

Firehed commented Jun 15, 2018

codecov bot commented Jun 6, 2018 •

edited

Loading