Add custom 403 response #1736

amitaibu · 2023-07-01T14:28:51Z

amitaibu · 2023-07-01T14:52:10Z

IHP/AuthSupport/Authorization.hs

@@ -5,21 +5,21 @@ Copyright: (c) digitally induced GmbH, 2020
 -}
 module IHP.AuthSupport.Authorization where

-import IHP.Prelude
+import IHP.Controller.Render


@mpscholten I might need your help in untangling this part which creates a cyclic import. I couldn't find an easy way

Another option is to move accessDeniedUnless into IHP.ErrorController or somewhere else

What do you think about moving all the functions related to 403 responses (including accessDeniedUnless) to it's own IHP.Controller.AccessDenied module? And then importing it where needed (and e.g. rexporting it from IHP.ControllerPrelude)

What do you think about moving all the functions related to 403 responses

I'm not sure.

IMO we should keep handleAccessDeniedFound in IHP/ErrorController.hs next to the handleNotFound - they are very similar, and maybe we can even create helper functions to reduce them.

renderNotFound and renderAccessDenied also feel natural inside IHP/Controller/Render.hs

So I think the question is only about accessDeniedUnless and accessDeniedWhen. Why not inside IHP.ErrorController?

The IHP.ErrorController should only have code related to actually error handling. accesDeniedUnless is not directly related to that.

Maybe we can move accessDeniedUnless to IHP.ControllerSupport?

Yeah, I like that - let me try it out.

Another concern I have. We have this code that I think caches the the 404 pages?

ihp/IHP/Server.hs

Line 100 in 45902cc

{ Static.ss404Handler = Just ErrorController.handleNotFound

But there's no Static.ss403Handler. Should we create a PR for Network.Wai.Application.Static?

@mpscholten How about IHP.Controller.Render? As it's really just a render function in the end

Hm, doesn't feel like it belongs there. I think this might get tricky when we don't want to move it into it's own dedicated module.

If we end up with something like IHP.Controller.AccessDenied we should also consider doing IHP.Controller.NotFound btw. The ErrorController module is already quite large, so it would be good to split up (this helps with compile times as well)

Ok, I'll try so separate to IHP.Controller.AccessDenied and IHP.Controller.NotFound

I'll also add notFoundWhen. Sometimes one would like to return 404 instead of 403 -- to prevent sniffing URLs and knowing they hit the right one by getting a 403.

amitaibu · 2023-07-01T20:47:37Z

IHP/ErrorController.hs

@@ -210,6 +114,7 @@ postgresHandler exception controller additionalInfo = do
        handlePostgresOutdatedError exception errorText = do
            let ihpIdeBaseUrl = ?context.frameworkConfig.ideBaseUrl
            let title = [hsx|Database looks outdated. {errorText}|]
+            -- @todo: Causes error


@mpscholten A bunch of surprising errors as I try to split modules.

I think again ihp-hsx isn't loaded properly for me. I'll try to grab later master

vscode is showing me an error, which tells me it's somehow not updated

Changing flake.nix to lastest master didn't help, also rm -rf .devenv .direnv.

@mpscholten when you get a chance, can you please check if you see the same error?

I did nix flake update and the error on VScode is gone. But the compiler error from above is still there

I could not reproduce this error. I've pushed a fix for all the other errors I did hit when running locally. It's up at amitaibu#3

It seems HSX is not correctly loaded from the ihp-hsx directory. Inside my ghci I had to type this to get it working:

:set -iihp-hsx :set -package ghc :l Test/Main

mpscholten · 2023-07-10T17:30:37Z

The new error is likely caused by not adding IHP.Controller.NotFound to the ihp.cabal

amitaibu · 2023-07-15T19:03:50Z

@mpscholten We need to untangle more, but I need some advice here:

ControllerSupport -> ErrorController -> NotFound/AccessDenied

But they need respondAndExit exposed in ControllerSupport

amitaibu · 2023-07-15T19:10:58Z

I'll try to move respondAndExit to own file

amitaibu · 2023-07-15T19:24:48Z

@mpscholten I've added IHP.Controller.Response and it's compiling! But it needs your approval that it's in the right direction, as it's likely a breaking change. On the other hand, it cleans IHP.ControllerSupport further.

mpscholten

Looks good 👍

IHP/Controller/AccessDenied.hs

mpscholten · 2023-07-16T08:48:54Z

IHP/Controller/NotFound.hs

+module IHP.Controller.NotFound
+    ( notFoundWhen
+    , notFoundUnless
+    , handleNotFound
+    , buildNotFoundResponse
+    )
+ where


Same indentation issue here

mpscholten · 2023-07-16T08:50:12Z

IHP/Controller/Render.hs

@@ -100,7 +101,21 @@ renderJson' additionalHeaders json = respondAndExit $ responseLBS status200 ([(h
 --
 renderNotFound :: (?context :: ControllerContext) => IO ()
 renderNotFound = do


This is the same as IHP.Controller.NotFound.renderNotFound, can we remove this one in IHP.Controller.Render?

mpscholten · 2023-07-16T08:50:30Z

IHP/Controller/Render.hs

+-- You can override the default not found error page by creating a new file at @static/403.html@. Then IHP will render that HTML file instead of displaying the default IHP access denied page.
+--
+renderAccessDenied :: (?context :: ControllerContext) => IO ()
+renderAccessDenied = do


Same here, duplicate of IHP.Controller.AccessDenied.renderAccessDenied

mpscholten · 2023-07-16T08:51:19Z

ihp.cabal

@@ -187,12 +186,13 @@ library
        , IHP.Controller.Layout
        , IHP.Controller.BasicAuth
        , IHP.Controller.Cookie
+        , IHP.Controller.AccessDenied
+        , IHP.Controller.NotFound


Suggested change

, IHP.Controller.NotFound

, IHP.Controller.NotFound

, IHP.Controller.Response

@amitaibu IHP.Controller.Response still needs to be added here to fix the build

mpscholten · 2023-07-16T08:54:28Z

I've added IHP.Controller.Response and it's compiling! But it needs your approval that it's in the right direction, as it's likely a breaking change. On the other hand, it cleans IHP.ControllerSupport further.

Looks like the right direction to me 👍 As these functions are typically imported via some Prelude, it shouldn't cause much breaking in most IHP app

Co-authored-by: Marc Scholten <marc@digitallyinduced.com>

amitaibu · 2023-07-18T15:59:44Z

@mpscholten I think something is wrong with the GH action:

https://github.com/digitallyinduced/ihp/actions/runs/5589604810/jobs/10218133895?pr=1736#step:5:2784

amitaibu · 2023-07-23T15:18:29Z

Tests are passing, but the build is showing as failed. Anyway, the last thing for the PR - I'll update the docs.

amitaibu · 2023-07-28T07:50:35Z

I've updated the docs, and the tests are passing. Not sure why other builds have not

mpscholten · 2023-07-28T17:18:22Z

Can you run nix build locally in the IHP directory? This will output some instructions to run nix log .... If you run that nix log command then, it will show the full error (you might need to scroll a bit up to find it, but it's typically highlighted red in my terminal)

amitaibu · 2023-07-28T19:57:12Z

I'm not getting instructions:

mpscholten · 2023-07-29T12:11:26Z

IHP/ControllerPrelude.hs

@@ -82,3 +86,4 @@ import IHP.FileStorage.Preprocessor.ImageMagick
 import IHP.Pagination.ControllerFunctions
 import IHP.HSX.QQ (hsx)
 import IHP.HSX.ToHtml ()
+import qualified Network.TLS as IHP.Controller


[115 of 203] Compiling IHP.ControllerPrelude ( IHP/ControllerPrelude.hs, dist/build/IHP/ControllerPrelude.o, dist/build/IHP/ControllerPrelude.dyn_o ) IHP/ControllerPrelude.hs:89:1: error: Could not load module ‘Network.TLS’ It is a member of the hidden package ‘tls-1.5.8’. Perhaps you need to add ‘tls’ to the build-depends in your .cabal file. Use -v (or `:set -v` in ghci) to see a list of the files searched for. | 89 | import qualified Network.TLS as IHP.Controller

This line here is causing the error @amitaibu

mpscholten · 2023-07-29T12:12:44Z

I'm not getting instructions:

Oh same here, looks like we don't export any kind of package from the IHP framework itself.

I did get it to work by putting this into an empty IHP project and then setting ihp.url = "path:///Users/marc/digitallyinduced/ihp"; in the project flake.nix. Then the nix build in that project gave me a nix log .. command where I could find the error

amitaibu · 2023-07-29T13:42:09Z

Fixed the error. The new error looks related to the fact my branch is on my own fork, not on IHP itself

mpscholten · 2023-07-30T10:01:44Z

Just pushed a fix for the github actions onto master via 8c44e84 Can you merge master into this branch and then let the github actions rerun?

amitaibu · 2023-07-31T07:07:35Z

@mpscholten there's a new error on the ARM64 build

mpscholten · 2023-07-31T07:52:16Z

Just fixed the expired cachix key and retriggered a build

amitaibu · 2023-07-31T17:24:56Z

Still red

mpscholten · 2023-07-31T18:33:14Z

I could fix this for the mac build, but the ubuntu build is still failing. I guess it's something with the github action secrets. Likely the secret key for cachix is not shared with your fork. Will merge it now 👍

amitaibu added 3 commits July 1, 2023 17:28

Add custom 403 response

419ff81

Add renderAccessDenied

31dc7c5

Try to import

4dd08ed

amitaibu commented Jul 1, 2023

View reviewed changes

amitaibu added 7 commits July 1, 2023 23:10

Move accessDenied* to IHP.ControllerSupport

04532ee

Try to move to IHP/Controller/Render.hs

126aac8

Start separating modules

ef696ee

More wiring

1422139

More wiring

d58980b

Revert comment out

535cb26

Add tood

0710c5e

amitaibu commented Jul 1, 2023

View reviewed changes

amitaibu and others added 3 commits July 2, 2023 00:02

More changes

034225c

Merge branch 'master' into 1734-access-denied

516410f

Fixed tests (#3)

a2e3da0

amitaibu added 3 commits July 15, 2023 21:32

Expose controller

4e836b7

More wiring

ff00386

Remove todo

bf3119e

Add IHP.Controller.Response

55fe7ad

Merge branch 'master' into 1734-access-denied

3f08617

mpscholten reviewed Jul 16, 2023

View reviewed changes

amitaibu and others added 4 commits July 17, 2023 23:07

Apply suggestions from code review

123a15c

Co-authored-by: Marc Scholten <marc@digitallyinduced.com>

Merge branch 'master' into 1734-access-denied

f2f385c

Code review fixes

abe6161

Add to prelude

98edcb4

Fix copy paste

a6ac858

Merge branch 'master' into 1734-access-denied

d9c80e6

amitaibu added 4 commits July 27, 2023 23:58

Merge branch 'master' into 1734-access-denied

579726c

Fix error

7340574

Add missing IHP.Controller.Response

f54b3b6

Update docs

82a6dce

amitaibu requested a review from mpscholten July 28, 2023 13:07

Merge branch 'master' into 1734-access-denied

2163ced

mpscholten reviewed Jul 29, 2023

View reviewed changes

Remove wrong import

0bc09a7

mpscholten marked this pull request as ready for review July 30, 2023 10:01

amitaibu added 4 commits July 30, 2023 21:12

Merge master

494ed73

Another fix

436a8a1

Update docs

0853aae

More fixes by the compiler

f1892f3

mpscholten approved these changes Jul 31, 2023

View reviewed changes

mpscholten merged commit 8050973 into digitallyinduced:master Jul 31, 2023
4 of 8 checks passed

amitaibu deleted the 1734-access-denied branch July 31, 2023 19:30

	, IHP.Controller.NotFound
	, IHP.Controller.NotFound
	, IHP.Controller.Response

Add custom 403 response #1736

Add custom 403 response #1736

Conversation

amitaibu commented Jul 1, 2023

amitaibu Jul 1, 2023 • edited Loading

Choose a reason for hiding this comment

mpscholten Jul 1, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpscholten commented Jul 10, 2023

amitaibu commented Jul 15, 2023

amitaibu commented Jul 15, 2023

amitaibu commented Jul 15, 2023 • edited Loading

mpscholten 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

mpscholten commented Jul 16, 2023

amitaibu commented Jul 18, 2023

amitaibu commented Jul 23, 2023

amitaibu commented Jul 28, 2023

mpscholten commented Jul 28, 2023

amitaibu commented Jul 28, 2023

Choose a reason for hiding this comment

mpscholten commented Jul 29, 2023 • edited Loading

amitaibu commented Jul 29, 2023

mpscholten commented Jul 30, 2023

amitaibu commented Jul 31, 2023

mpscholten commented Jul 31, 2023

amitaibu commented Jul 31, 2023

mpscholten commented Jul 31, 2023

amitaibu Jul 1, 2023 •

edited

Loading

mpscholten Jul 1, 2023 •

edited

Loading

amitaibu commented Jul 15, 2023 •

edited

Loading

mpscholten commented Jul 29, 2023 •

edited

Loading