Closed (fixed)
Project:
SAML Authentication
Version:
8.x-3.0-alpha2
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
20 Nov 2020 at 21:45 UTC
Updated:
6 Apr 2021 at 19:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
pvalappil commentedAdded new url field to config form
Comment #3
pvalappil commentedUpdating patch #2 by adding schema change and fixing the typo
Comment #4
roderikThank you for the patch, it's quite complete.
But can you tell me more about the application of this setting?
Most errors are encountered when a SAML SP is not configured completely. I would guess that such an error page is not interesting in that case, because those kinds of errors are temporary.
Do you need this specifically for a Drupal installation that is configured to not create new users? And in that case, is the reason you want a redirect URL, because you actually want a nice looking separate HTML page for those users (registered at the IDP but not in the Drupal installation) who are not allowed to log in?
Or is the 'real' reason that you need a separate page, because the error messages from the samlauth modules in that case are not good enough?
Comment #5
pvalappil commentedIt actually both, Our system does not create new user if user does not exists, And we wanted to redirect users to an already existing error page.
Comment #7
roderikThank you. I've taken a closer look at the current code again, and:
1) When I was asking, I wasn't sure that the error message when a user does not exist, is good enough. But IMHO it is.
2) I'm committing this change. Documenting my thinking process / why I was asking the question:
At first, I was doubting about adding such a redirect setting to this module. It feels like this functionality could be handled by another contrib module (if not custom code), which has more control over what to do in case of an error - like e.g. the customerror or error_redirect module. (Just like I'm hesitant to add functionality to this module, which automatically redirects to the IdP when a user is not logged in.)
However... that only works if this module throws an AccessDeniedHttpException, instead of redirecting to the front page. I don't want to think about such a change, so close to the 3.0 release. (And maybe I'm wrong, maybe the customerror doesn't actually work well for this.)
So I'm committing this with a @todo to think about it - and if I develop another opinion about how this could be done better, I might remove this setting in the 4.x branch.
The SamlController code needed to be changed because of the whole getTrustedRedirectResponse() rewrite, that was introduced to fight fatal errors earlier this year. I didn't commit the unit tests because they would have to be fixed, and... I don't claim to be an expert on tests, but it feels to me like this is better suitable for a functional test, because a unit test in this case is so closely tied to the implementation that it becomes less useful... and it doesn't actually test that much.
(Which is a large followup: #3202137: Write tests.)
Comment #8
roderikI'm happy this issue was handled, because it allowed/forced me to think more clearly about a followup.
(I added another config option to have an exception fall through - which makes it easier for custom event subscribers to handle the error by themselves. This option will very likely be removed in 4.x, because "doing error handling in event subscribers, which are overridable" will be the standard.)