Can we have the possibility to change this message? I don't want to expose the reason why the request was blocked.
Showing the default access denied page would be a better option than a green message saying "change your source IP to get access".

CommentFileSizeAuthor
#8 interdiff_7-8.txt3.94 KBnace_fr
#8 3104213-8.patch12.28 KBnace_fr
#7 interdiff_5-7.txt4.99 KBkbrodej
#7 3104213-7.patch12.49 KBkbrodej
#5 3104213-5.patch10.15 KBkbrodej
#3 3104213_3.patch3.86 KBAnonymous (not verified)

Comments

d4t3r created an issue. See original summary.

j.b’s picture

j.b commented

This will be a great addition to the module or even perhaps give also the possibility to hide or show the access denied message.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 3104213_3.patch3.86 KB

Hello,
I created a patch that adds two checkboxes into restrict_ip settings. They are at the bottom. One adds the default drupal denied page, which can be changed in basic drupal settings. And the other checkbox creates a blank page. Dunno why you would use that one, but hey, it was requested.
I also added a patch from issue #2957482 by @RoshniPatel.addweb which fixes count error.

drupalbubb’s picture

drupalbubb commented

Thanks DenisCi,
i have some suggestions:
- please remove patch from issue 2957482 again. This leads to confusion.
- the white page is useless, i think j.b talked about disabling the error message completly (and show e.g. the frontpage).
- can we have a textbox where we can change/translate the message and can you change the color of the message to red? A empty textbox could disble the message (see above). Prefilled with the default text.
- changing behavior to output the access denied page is realy nice.

kbrodej’s picture

kbrodej commented
Status: Active » Needs review
StatusFileSize
new 3104213-5.patch10.15 KB

Hi.

Reviewed the patch from #3, it did not apply at first.

I fixed it and added the requests from #4

drupalbubb’s picture

drupalbubb commented

Thanks kbrodej, i successfully applied your patch.
To make the patch a bit more intuitive some things need clarification:
- if i check the first checkbox "default access denied page" do i also need to check the second checkbox?
- why is it possible to have all combinations? Can i display both, message and page?
- the second checkbox tells about the plural of message. See above
- you should prefill the textbox with the default value - my opinion.

kbrodej’s picture

kbrodej commented
StatusFileSize
new 3104213-7.patch12.49 KB
new interdiff_5-7.txt4.99 KB

Hi.

Took a look into your suggestions.

I realised that no matter which combination you used, you could not get redirected to 403 page if user was authenticated. It was always redirecting to only difference was showing the error message notification if it was set or not.

So:
- If you check default access denied page it will redirect you to default 403 page set in config.
- Naming of second check box was unfortunate. Renamed it to notification as it is meant as error message, which displays in red.
- Reordered positions of these settings in the config form to help make it clearer what they do.
- Text should be loaded from config file.

nace_fr’s picture

nace_fr commented
StatusFileSize
new 3104213-8.patch12.28 KB
new interdiff_7-8.txt3.94 KB

Hi. I have checked this one and not all cases were covered in patch #7. I have taken #7 as base and added some cases.

With this patch you can display default page with custom text (text will always be displayed), or you can redirect to home page and choose if you want to display text or not.

I tried to do this in a way we don't restructure the module, since currently access denied page is basically just a blank page with an access denied text. I think there is room for improvement, but since modules functionality is done like this from the beginning, I'm not sure if we should change this. Anyway I have added just a few tweaks to patch #7.

drupalbubb’s picture

drupalbubb commented

I successfully applied the patch, but it still needs some modifications.
If i understand things right, then the second checkbox "access denied notification" controls the visibility of the textbox. So please place the textbox to the end and use the same naming. The checkbox is maybe needless, no text means no output.
If the "access denied notification" checkbox enables the whole patch then it needs to be moved to the top and a renaming is needed.
Additionaly it would be nice to see the patch config somewhere above the black/whitelisting country list, which is long. A title like "Output Control" is missing.

drupalbubb’s picture

drupalbubb commented
Status: Needs review » Needs work

I removed this patch again, as it denied access to my restricted pages for me after a few days. Jumping to the frontpage without message. Something in accesslog about a page /restrict_ip/access_denied or similar.

drupalbubb’s picture

drupalbubb commented

Clearing the router cache seems to solve the problem temporary.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Version: 8.x-2.0-beta2 » 3.x-dev

Cleaning up the module versions with Drupal 8 & 9 compatible 3.x release. We have to sort out outdated issues to focus on 3.x stabilization. Older Drupal 8 versions (8.x-2.x and 8.x-1.x) will be deprecated soon, so it doesn't make much sense anymore to fix them. Please instead retest if this issue is still relevant for latest 3.x version. If you should experience other issues, please create a separate issue for that, if not already existing.

If the issue from this request still exists in 3.x, please create a Merge Request (MR) against the latest 3.x-dev version to fix the issue and tell us about the actual problem and expected result.

Thank you very much! Let's get this module fixed together as community :)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs work » Postponed (maintainer needs more info)

Still an issue in 4.x? Then please switch the version and provide a MR for review to speed things up here. Thanks!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
smustgrave’s picture

smustgrave commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

Since there hasn't been a follow up going to close out if still an issue on 5.0.x please reopen