-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add custom 403 response #1736
Add custom 403 response #1736
Conversation
IHP/AuthSupport/Authorization.hs
Outdated
@@ -5,21 +5,21 @@ Copyright: (c) digitally induced GmbH, 2020 | |||
-} | |||
module IHP.AuthSupport.Authorization where | |||
|
|||
import IHP.Prelude | |||
import IHP.Controller.Render |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving all the functions related to 403 responses
I'm not sure.
- IMO we should keep
handleAccessDeniedFound
inIHP/ErrorController.hs
next to thehandleNotFound
- they are very similar, and maybe we can even create helper functions to reduce them. renderNotFound
andrenderAccessDenied
also feel natural insideIHP/Controller/Render.hs
So I think the question is only about accessDeniedUnless
and accessDeniedWhen
. Why not inside IHP.ErrorController
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpscholten How about IHP.Controller.Render
? As it's really just a render function in the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll try so separate to IHP.Controller.AccessDenied
and IHP.Controller.NotFound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
IHP/ErrorController.hs
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpscholten A bunch of surprising errors as I try to split modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think again ihp-hsx isn't loaded properly for me. I'll try to grab later master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did nix flake update
and the error on VScode is gone. But the compiler error from above is still there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
The new error is likely caused by not adding |
@mpscholten We need to untangle more, but I need some advice here:
But they need |
I'll try to move |
@mpscholten I've added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
IHP/Controller/NotFound.hs
Outdated
module IHP.Controller.NotFound | ||
( notFoundWhen | ||
, notFoundUnless | ||
, handleNotFound | ||
, buildNotFoundResponse | ||
) | ||
where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same indentation issue here
IHP/Controller/Render.hs
Outdated
@@ -100,7 +101,21 @@ renderJson' additionalHeaders json = respondAndExit $ responseLBS status200 ([(h | |||
-- | |||
renderNotFound :: (?context :: ControllerContext) => IO () | |||
renderNotFound = do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as IHP.Controller.NotFound.renderNotFound
, can we remove this one in IHP.Controller.Render
?
IHP/Controller/Render.hs
Outdated
-- 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, duplicate of IHP.Controller.AccessDenied.renderAccessDenied
@@ -187,12 +186,13 @@ library | |||
, IHP.Controller.Layout | |||
, IHP.Controller.BasicAuth | |||
, IHP.Controller.Cookie | |||
, IHP.Controller.AccessDenied | |||
, IHP.Controller.NotFound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, IHP.Controller.NotFound | |
, IHP.Controller.NotFound | |
, IHP.Controller.Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amitaibu IHP.Controller.Response
still needs to be added here to fix the build
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 <[email protected]>
@mpscholten I think something is wrong with the GH action: |
Tests are passing, but the build is showing as failed. Anyway, the last thing for the PR - I'll update the docs. |
I've updated the docs, and the tests are passing. Not sure why other builds have not |
Can you run |
IHP/ControllerPrelude.hs
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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
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 |
Fixed the error. The new error looks related to the fact my branch is on my own fork, not on IHP itself |
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? |
@mpscholten there's a new error on the ARM64 build |
Just fixed the expired cachix key and retriggered a build |
Still red |
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 👍 |
fixes #1734