Improve behaviour for Access Denied with Remember Me #54318
Comments
|
I push a PR in this way, user can choose exact behaviour he want |
The big question (that was never answered in the previous issue even if I brought it up several time) is that it is very hard to determine that "special case": doing a security check on The current behavior is based on the fact that if you don't know whether the decision depends on the trust level of the token, it makes sense to redo an attempt with a higher trust level. Solving the issue would involve proposing a solution to resolve this "don't know" state. |
|
that's why I allow in my PR configure that by a custom handler, We have 2 possibilites in the core :
and if user want a particular behaviour, he can make his handler with his particulars conditions, |
|
we can declare a function in AuthenticationTrustResolverInterface for custom Resolver to be called instaed of isFullFledged if present to check with Token. What do you think about it ? |
|
having to implement a custom handler able to know about the logic implemented by all custom voters to know whether the decision that denied access included some logic depending on the trust level would be totally unmaintainable.
I don't understand what you propose here. |
|
whitch voters? at this point you just have an exception and a AuthenticationTrustResolverInterface. My proposal is in AuthenticationTrustResolverInterface add a facultative function interceptExceptionNotFullFledged taken Exception and token to re 8000 spond if yes or no have to call the NotFullFledgedHandler For in handleAccessDeniedException have : $token = $this->tokenStorage->getToken();
$intercept = method_exists($this->authenticationTrustResolver, 'interceptExceptionNotFullFledged') ?
$this->authenticationTrustResolver->interceptExceptionNotFullFledged($token) :
!$this->authenticationTrustResolver->isFullFledged($token);
if ($intercept) {
$startAuthenticationResponse = function () use($exception, $event, $token) {
... |
|
@eltharin the If you want to avoid useless redirections to the login page for partially trusted tokens (to replace them with a fully trusted token) in case it won't change the decision, you need to know whether the decision depended on the trust level of the token.
This still does not explain what this |
|
interceptExceptionNotFullFledged can read the exception to get all attributes and subject and decide if has to call custom notfullfledgedhandler or not. maybe the names are not good, same is for remember me behaviour is SAME as normal login behaviour |
|
Else, I can create a notfullfledgedhandler for recreate actual behaviour, and call it directly. With that, user can create his own handler whitch is called in all cases, and he makes his conditions and changes he want and if return null, follow the rest of the handleAccessDeniedException function |
|
If the exception were passed, I don't see what would be the benefit over your NotFullFledgedHandlerInterface (having to customize the TrustResolver is a lot more complex than implementing an handler, as it require implementing all the other methods). But this does not solve the fact that you expect this method to be aware of how a decision was taken by voters to know which decision involved the trust level. This is exactly the part that is totally unmaintained in any big projects. You would have to keep the implementation of your handler in sync with the implementation of all voters of the projects. |
|
have to forget interceptExceptionNotFullFledged I think it's better to integrate condition in NotFullFledgedHandler. |
|
new Version, all informations are passed to handler token, exception, ... I create a handler to cover actual behaviour, so I don't see where is a prblem. |
|
@stof : |
|
I just want to understand where a problem can come, |
|
What I understand here: voters can decide to reject based on the authentication level of the token, and we have no clue when this happens. This reminds me #46493, which tries to give back reasons why voters rejected. The reason is currently thought as a simple string for humans. But we could also make voters able to return a codified reasons that we could then leverage to achieve this file-grained auth UX? |
|
I also continued the work of #46493 in #58107. |
Description
This is a continuation of #16026 I thought carsonbot would re-open but it's been a week and nothing has happened, so I'm opening this.
In summary, I would like to have better control of what happens when a user who is authenticated with "remember me" is denied access. As a general rule, I expect that a user authenticated via remember me should have the same experience as a fully authenticated user, so if they are denied access to something, the response should be a 403 Forbidden. But currently, the response is a redirect to login. This is not a good experience for the user, who still won't get access even after being fully authenticated. The only exception to this is when I want to protect a sensitive part of the website (e.g. changing password) and it requires
IS_AUTHENTICATED_FULLY. Any other access check should result in a 403 Forbidden, even for "remember me" users.In fact, I don't think that redirecting "remember" me users to the login page in order to become fully authenticated is ideal, and I think this would be a good opportunity to improve the process. Being redirected to the login page makes it appear as though they aren't signed in at all. From the user's point of view, it's hard to tell whether their session has expired, or if they are simply being prompted to fully authenticate. What I would prefer is if the user sees a different page that says "you are accessing a sensitive area, please verify your password" or something along those lines. That makes it clear that they are still logged in, but they just need to provide their password (not username, because they're already signed in). Of course, I can implement this myself now by adding checks to the login page and displaying different content to anonymous users vs users authenticated with "remember me", but to me this is standard behaviour that I would add to every single project I work on, and so I feel it's worth building in to the framework. So just like we have a special case for "login" and "logout", I think it's worth having a special route for "upgrade" as well. In case you don't need different content, then you could simply configure the "upgrade" route to be the same as your "login" route.
The current behaviour also makes it difficult to write javascript that handles dynamic content. If I'm loading dynamic content with an AJAX request and the request gets redirected to the login page, it's hard to determine whether this was due to a session timeout or access denied. But if there was a different route for login vs upgrade, that would make it possible to write javascript that can distinguish these cases, and display an appropriate error message.
So finally, this is what I propose:
This would align closer with what I (and I assume most developers) expect should happen by default, it improves the user experience, and makes it easier to write javascript for dynamic content.
Thanks
Example
No response
The text was updated successfully, but these errors were encountered: