Handle 401, 403, and 404 errors in frontend #2249

askvortsov1 · 2020-07-29T01:44:50Z

Changes proposed in this pull request:
Handle 401, 403, and 404 errors in frontend (see commits for specific granular changes)

Reviewers should focus on:

See GH review comments

Confirmed

Frontend changes: tested on a local Flarum installation.
Backend changes: tests are green (run composer test).

franzliedke

I already expressed my doubts about this approach on the issue, but I'd like to add a few general concerns:

Again, first and foremost, I can only recommend to avoid complexity like this on error pages - from painful experience.
This would render the forum frontend for any 404, even on API and admin, if I am not mistaken.
"Model not found" errors must result in 404 status codes, too, to prevent e.g. search engines indexing error pages, and be good internet citizens - if I understand correctly, you would check this in the frontend now, at which point you could not set the status code anymore.

src/Http/Content/NotAuthenticated.php

franzliedke · 2020-09-25T12:45:01Z

src/Http/Middleware/HandleErrors.php

+                if (! $handled) {
+                    throw $e;
+                }
+            }


This seems to re-implement logic similar to what the registry already does, and the meaning of the additional parameter doesn't seem very intuitive.

js/src/forum/components/ErrorPage.js

js/src/forum/routes.js

src/Foundation/ErrorHandling/FrontendFormatter.php

askvortsov1 · 2020-10-02T05:11:43Z

#1847 is a MUCH better way to get the actor, without needing to create a second error handler. Thanks Franz!

stale · 2020-12-31T11:36:13Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum.
In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

askvortsov1 · 2021-03-25T17:25:50Z

This would render the forum frontend for any 404, even on API and admin, if I am not mistaken.

No API errors will be handled like this, since it's not on the API middleware stack. And neither will admin 404s, as ALL requests to admin routes are sent to the admin frontend, defaulting to the dashboard page if the route isn't found.

"Model not found" errors must result in 404 status codes, too, to prevent e.g. search engines indexing error pages, and be good internet citizens - if I understand correctly, you would check this in the frontend now, at which point you could not set the status code anymore.

I don't think so? At least from my testing, it returns the page with a 404 response code.

Again, first and foremost, I can only recommend to avoid complexity like this on error pages - from painful experience.

We wouldn't be doing it on proper "error" pages, only on 4xxs, where nothing has gone wrong, it's just that the client's input is in some way invalid.

askvortsov1 · 2021-03-25T17:43:53Z

Other TODO:

Once User::fromRequest method #2703 and Access request actor in error handler #2410 are merged, we should switch to using one ErrorHandler
I think we should add a handleInFrontend method to the HandledError class, and provide the list of errors to be handled in the frontend to Registry.
We might want to provide both FrontendFormatter and ViewFormatter to HandleErrors? Although that'd be inconsistent with what's needed for API.
Alternatively, we could merge FrontendFormatter and ViewFormatter into the same class
Design the error page.

…eware can be used on only some exceptions if wanted

Improve ErrorPage

[ci skip] [skip ci]

askvortsov1 · 2021-04-13T05:45:02Z

src/Foundation/ErrorServiceProvider.php

@@ -57,6 +63,15 @@ public function register()
            ];
        });

+        $this->container->singleton('flarum.error.contents', function () {
+            return [
+                NotAuthenticatedException::class => NotAuthenticated::class,


Should these be codes or status strings? That might be cleaner for exceptions added by extensions.

askvortsov1 · 2021-04-13T05:47:32Z

src/Foundation/ErrorHandling/Middleware/ExecuteToFrontend.php

+use Psr\Http\Server\MiddlewareInterface;
+use Psr\Http\Server\RequestHandlerInterface;
+
+class ExecuteErrorToFrontend implements MiddlewareInterface


Is this too verbose?

askvortsov1 · 2021-04-13T05:48:42Z

src/Foundation/ErrorHandling/Middleware/ExecuteErrorToFrontend.php

+use Psr\Http\Server\MiddlewareInterface;
+use Psr\Http\Server\RequestHandlerInterface;
+
+class ExecuteErrorToFrontend implements MiddlewareInterface


Is this too verbose? ExecuteError?

Verbosity === self-documentation in my eyes! :)

askvortsov1 · 2021-04-13T05:48:58Z

src/Foundation/ErrorHandling/HandledError.php

@@ -65,6 +67,11 @@ public function shouldBeReported(): bool
        return $this->type === 'unknown';
    }

+    public function contentClass()
+    {
+        return $this->frontendContentClass;


Not sure I like the naming, especially of the prop

askvortsov1 · 2021-04-13T05:49:35Z

src/Forum/ForumServiceProvider.php

+
+            foreach ($middleware as $middlewareClass) {
+                if (!in_array($middlewareClass, [
+                    'flarum.forum.error_handler',


Anything else we want to exclude?

askvortsov1 · 2021-04-13T05:49:54Z

src/Forum/ForumServiceProvider.php

+
+            foreach ($middleware as $middlewareClass) {
+                if (!in_array($middlewareClass, [
+                    'flarum.forum.error_handler',


We can't include the error handler to avoid a circular resolution dependency

askvortsov1 · 2021-04-13T05:50:37Z

js/src/common/components/ErrorPage.js

+      <div className="ErrorPage">
+        <div className="container">
+          <h2>{this.title}</h2>
+          <p>{app.translator.trans(`core.forum.error.${this.attrs.errorCode}_text`)}</p>


@frontend folks, any ideas for making this look... not horrible?

My first idea would be bigger text. 13px is fine for forum posts and dense pages, but an error page needs to be padded out in some form, I think.

We don't want to do too much theming as that should be up to forum maintainers, though.

davwheat

If I go to a 404 page, click return to forum, then the back button in my browser, I end up with a broken error page:

We might be able to fix this by using history.replaceState() to store the info about the error as part of the browsing history state.

https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState#examples

askvortsov1 force-pushed the as/frontend_error_handler branch from c041882 to 68175ad Compare July 29, 2020 01:46

askvortsov1 marked this pull request as draft July 29, 2020 01:47

askvortsov1 mentioned this pull request Aug 24, 2020

[Frontend Rewrite] Bring back current routeName #2266

Closed

askvortsov1 mentioned this pull request Sep 7, 2020

Allow extensions to use route resolvers #2275

Merged

franzliedke reviewed Sep 25, 2020

View reviewed changes

stale bot added the stale Issues that have had over 90 days of inactivity label Dec 31, 2020

askvortsov1 added the org/keep Issues we want to keep open label Jan 5, 2021

stale bot removed the stale Issues that have had over 90 days of inactivity label Jan 5, 2021

askvortsov1 force-pushed the as/frontend_error_handler branch 2 times, most recently from 9f25f1e to df99992 Compare March 25, 2021 17:13

askvortsov1 force-pushed the as/frontend_error_handler branch from b9f8ef5 to 2a79917 Compare March 25, 2021 17:40

askvortsov1 and others added 15 commits April 12, 2021 23:16

Add errorClasses parameter to HandleErrors so that HandleErrors middl…

053b3fd

…eware can be used on only some exceptions if wanted

Add a formatter for rendering errors via the frontend

f879182

Add middleware in admin and forum to some errors via the frontend

194cf55

Fix missing imports for exceptions to process with frontend formatter

122f566

Add frontend component page for not found

bf20ac6

Improve ErrorPage

Use content for error page

7035625

Apply fixes from StyleCI

5eff7b0

[ci skip] [skip ci]

Cleanup

f5fbaf4

More cleanup and fixes

827366f

Run frontend handler before route resolver

b5d2cbd

Use resolver instead of catch-all route for errors

058f0c5

Apply fixes from StyleCI

6b79902

[ci skip] [skip ci]

Simplify frontend error handling

c842468

Re-implement via middleware pipe

235dfeb

Fix URL changing to homepage on 404

897dd57

askvortsov1 added 2 commits April 13, 2021 01:22

Update docblock

43d00fc

Add content to error page, drop 404 page, move translations

7cf0df8

askvortsov1 force-pushed the as/frontend_error_handler branch from 2b0c4ae to 7cf0df8 Compare April 13, 2021 05:43

Provide frontend string as param to ExecuteErrorToFrontend handler

2c8cc0c

askvortsov1 force-pushed the as/frontend_error_handler branch from c2e786b to 2c8cc0c Compare April 13, 2021 05:47

Remove redundant class

d7a5bf7

askvortsov1 force-pushed the as/frontend_error_handler branch from c5d7146 to d7a5bf7 Compare April 13, 2021 05:48

Apply fixes from StyleCI

72f7c3c

[ci skip] [skip ci]

askvortsov1 commented Apr 13, 2021

View reviewed changes

askvortsov1 marked this pull request as ready for review April 13, 2021 05:50

askvortsov1 requested review from SychO9, davwheat and luceos April 13, 2021 05:51

davwheat requested changes Apr 20, 2021

View reviewed changes

davwheat added this to the 1.1 milestone May 12, 2021

davwheat assigned askvortsov1 May 12, 2021

askvortsov1 modified the milestones: 1.1, 1.2 Sep 28, 2021

SychO9 removed this from the 1.2 milestone Oct 1, 2021

askvortsov1 closed this Oct 20, 2021

Uh oh!

Handle 401, 403, and 404 errors in frontend #2249

Handle 401, 403, and 404 errors in frontend #2249

Uh oh!

Conversation

askvortsov1 commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

franzliedke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

askvortsov1 commented Oct 2, 2020

Uh oh!

stale bot commented Dec 31, 2020

Uh oh!

askvortsov1 commented Mar 25, 2021

Uh oh!

askvortsov1 commented Mar 25, 2021

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!

davwheat Apr 20, 2021 • 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!

davwheat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

askvortsov1 commented Jul 29, 2020 •

edited

Loading

davwheat Apr 20, 2021 •

edited

Loading