Fix Authenticator Class (getCredentials) example #7065
Conversation
The current wording surrounding the getCredentials is misleading. Returning null does stop authentication, but it causes authentication to be successful. The example implies that X-AUTH-TOKEN is required and should be correct. For this behavior, either an AuthenticationException should be thrown or the credential variables should be initialized as empty and passed on to getUser().
| // no token? Return null and no other methods will be called | ||
| return; | ||
| // No token? Cause authentication to fail. | ||
| throw new AuthenticationException(); |
There was a problem hiding this comment.
What about setting $token to null then?
There was a problem hiding this comment.
I think throwing the exception in getCredentials is more explicit about the fact this authenticator will not allow any request not having this header (and won't let other authenticators try).
|
Thanks for the feedback. I wasn't sure which solution would match Symfony's style better (exception vs empty variables) and had just picked one. I've adjusted the solution and comments. |
| * Called on every request. Return whatever credentials you want, | ||
| * or null to stop authentication. | ||
| * Called on every request. Return whatever credentials you want to | ||
| * be passed to getUser(). Returning null will cause authentication |
There was a problem hiding this comment.
It'll not necessary cause the authentication to be successful, given that you can have multiple guard authenticators. It only means this authenticator was unable to extract credentials from the request, and thus will give hand to the next ones.
|
Thanks for the feedback @ogizanagi . It didn't occur to me that multiple authenticators could be set up, but that makes sense. |
| return; | ||
| // No token? | ||
| // Throwing an exception will cause authentication | ||
| // to fail and prevent other authenticators from |
There was a problem hiding this comment.
Can you please reduce the number of spaces here so that all comments are aligned?
|
👍 Status: Reviewed |
| * Called on every request. Return whatever credentials you want, | ||
| * or null to stop authentication. | ||
| * Called on every request. Return whatever credentials you want to | ||
| * be passed to getUser(). Returning null will cause this authenticator |
There was a problem hiding this comment.
- * be passed to getUser(). Returning null will cause this authenticator
+ * be passed to getUser(). Returning null will cause this authenticator| // Throwing an exception will cause authentication | ||
| // to fail and prevent other authenticators from | ||
| // attempting to authenticate. | ||
| throw new AuthenticationException('No token provided.'); |
There was a problem hiding this comment.
this is a bad implementation, because it prevents any other auth system to be used (including the anonymous auth). We should not advocate it. The Symfony security system is precisely meant to allow multiple auth strategies in parallel.
There was a problem hiding this comment.
Indeed, this should be reverted and added in a note or caution block.
|
Status: needs work |
|
Thank you @thtroyer. |
This PR was submitted for the 3.1 branch but it was merged into the 2.8 branch instead (closes #7065). Discussion ---------- Fix Authenticator Class (getCredentials) example The current wording surrounding the getCredentials is misleading. Returning null does stop authentication, but it causes authentication to be successful. The example implies that X-AUTH-TOKEN is required and should be correct. For this behavior, either an AuthenticationException should be thrown or the credential variables should be initialized as empty and passed on to getUser(). Commits ------- 52d56f4 Fix Authenticator Class (getCredentials) example
The current wording surrounding the getCredentials is misleading. Returning null does stop authentication, but it causes authentication to be successful. The example implies that X-AUTH-TOKEN is required and should be correct. For this behavior, either an AuthenticationException should be thrown or the credential variables should be initialized as empty and passed on to getUser().