Unauthenticated actions by markstory · Pull Request #191 · cakephp/authentication

markstory · 2018-03-30T02:49:53Z

This adds a very rudimentary form of access control to this plugin. By default the component will require an identity to be present in all requests. This behavior can be disabled via a setting or by using the allowUnauthenticated() method to whitelist the controller actions.

Refs #188

Don't assign properties.

This adds a *very* rudimentary form of access control to this plugin. By default the component will require an identity to be present in all requests. This behavior can be disabled via a setting or by using the allowUnauthenticated() method to whitelist the controller actions. Refs #188

codecov-io · 2018-03-30T02:56:37Z

Codecov Report

Merging #191 into master will increase coverage by 0.24%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #191      +/-   ##
============================================
+ Coverage     97.89%   98.14%   +0.24%     
- Complexity      321      342      +21     
============================================
  Files            33       34       +1     
  Lines           857      918      +61     
============================================
+ Hits            839      901      +62     
+ Misses           18       17       -1

Impacted Files	Coverage Δ	Complexity Δ
...c/Controller/Component/AuthenticationComponent.php	`98.57% <100%> (+2.49%)`	`21 <7> (+7)`	⬆️
src/UrlChecker/CakeRouterUrlChecker.php	`100% <0%> (ø)`	`6% <0%> (?)`
src/Authenticator/CookieAuthenticator.php	`98.9% <0%> (+0.54%)`	`26% <0%> (+8%)`	⬆️

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 37d9b0b...7ffabad. Read the comment docs.

robertpustulka · 2018-03-30T04:39:05Z

src/Controller/Component/AuthenticationComponent.php

+
+        $identity = $request->getAttribute('identity');
+        if (!$identity) {
+            throw new UnauthorizedException([]);


Should it throw MissingIdentityException?

Or UnauthenticatedException - but MissingIdentityException as generic one can also be fine.

With a loginRedirect or loginAction key present, wouldn't it make sense to redirect to login then instead from the component?
The exception is only useful for stateless (api) access, right?

Redirection is performed from a middleware using unauthorized handlers.

~~@robertpustulka You're right, MissingIdentity would be better, as it is handled by the Unauthorized handlers.~~

Redirection in the middleware is part of authorization and not this plugin.

@dereuromark My worry with a redirect is it makes stateless auth hard. Whereas an exception can be converted into a redirect by the application code.

Should it be a new UnauthenticatedException ?
UnauthorizedException is used for http auth AFAIR

At this point definitely not authorize, but authenticate, yes.

I was attempting to reuse the existing exception which generates a 401 error, which is what I think we want here too.

markstory · 2018-04-01T03:23:27Z

Any other feedback on this?

dereuromark · 2018-04-03T15:56:52Z

docs/Quick-start-and-introduction.md

+
+```php
+// In your controller's beforeFilter method.
+$this->Authentication->allowUnauthorized(['view']);


allowUnauthorized() inside the Authentication module might a sth to trip over?
Should we call it allowUnauthenticated() or just allowPublic()?

At this point it is still either authenticated or not (public access).

The docs were wrong, and fixed in 1f325dc. The method is allowUnauthenticated(). I was worried that allowPublic() could be confused with PHP method visibility.

ADmad · 2018-04-03T17:01:54Z

src/Controller/Component/AuthenticationComponent.php

+     * @param array $actions The action list.
+     * @return $this
+     */
+    public function allowUnauthenticated(array $actions)


Should this have a merge option to allow adding to existing list? Unless there's a method to retrieve existing list so that user can merge himself and reset.

For e.g. I would like to allow 'view' always through AppController and then additional action inside a specific controller.

Would it then merge by default? Calling the param $overwrite (default false) like in other places in core?

We could add a getter which would allow merge operations.

Why a getter? Wouldnt this be

allowUnauthenticated(array $actions, $merge = false)

?

Well, you could always do allowUnauthenticated(['index'], ['merge' => true]) :)

Merging to existing list will be the most common usage. So it should be possible to do that with minimal code 🙂.

So mergeUnauthenticated() is the method name to move forward here?

What about addUnauthenticatedActions()? Merge sounds mire complicated that add or append do.

robertpustulka · 2018-04-23T08:35:08Z

For anyone trying to integrate this feature with Authorization plugin unauthorized handlers, this middleware might come in handy:

$middleware->add(function($req, $res, $next) {
    try {
        return $next($req, $res);
    } catch (UnauthorizedException $e) {
        throw new MissingIdentityException(['identity'], null, $e);
    }
});

markstory added 3 commits March 29, 2018 22:18

Clean up tests.

cad47fd

Don't assign properties.

Add documentation for allowUnauthenticated.

75754d0

markstory added the enhancement label Mar 30, 2018

markstory mentioned this pull request Mar 30, 2018

Silent allow Issue. #188

Closed

robertpustulka reviewed Mar 30, 2018

View reviewed changes

dereuromark reviewed Apr 3, 2018

View reviewed changes

Fix method name in docs.

1f325dc

ADmad reviewed Apr 3, 2018

View reviewed changes

markstory added 2 commits April 3, 2018 22:28

Add accessor for unauthenticated actions.

dfe26b8

Add method to append to unauthenticated actions.

7ffabad

dereuromark merged commit 1dbfa00 into master Apr 14, 2018

dereuromark deleted the unauthenticated-actions branch April 14, 2018 08:07

dereuromark mentioned this pull request May 5, 2018

UnauthorizedException => UnauthenticatedException #207

Closed

Conversation

markstory commented Mar 30, 2018

Uh oh!

codecov-io commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dereuromark Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory commented Apr 1, 2018

Uh oh!

dereuromark Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertpustulka commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Mar 30, 2018 •

edited

Loading

dereuromark Mar 30, 2018 •

edited

Loading

markstory Mar 31, 2018 •

edited

Loading

dereuromark Apr 3, 2018 •

edited

Loading

robertpustulka commented Apr 23, 2018 •

edited

Loading