Remove legacy CSRF protection for approve page #932
Conversation
| public String ERROR = "error"; | ||
| public String LOGIN_REQUIRED = "login_required"; | ||
| String ERROR = "error"; | ||
| String LOGIN_REQUIRED = "login_required"; |
There was a problem hiding this comment.
The public modifiers here were removed by IntelliJ IDEA. According to the JLS (§9.4),
Every method declaration in the body of an interface is implicitly public (§6.6). It is permitted, but discouraged as a matter of style, to redundantly specify the public modifier for a method declaration in an interface.
|
Please revert the removal of "public" from the interface definition as this makes it inconsistent with the rest of the project's style. I'm aware of that style suggestion but it is not applied here. Thanks. |
Instead, we rely on the Spring Security CSRF protection, like we already do for the login page. Additionally, we remove the authentication check in`isApproved`, because this is already done by Spring Security (and if not, we have bigger problems to worry about).
praseodym
force-pushed
the
fix-approve-csrf
branch
from
29dc4ed to
b5c298e
Compare
|
Done. |
|
Was tripped up by this for the last day :P This fix sounds great. Would it be possible to merge this and re-release @jricher ? I can't get 1.2.1 working without manually doing all this in my overlay project. Thanks @praseodym! |
|
actually, I'm not in production with this yet so I can get around with other testing for now by taking away the csrf protection in user-context.xml for now. |
|
Thanks! I can possibly do the pull and release next week. I was having trouble with the integration test environment just before the last release which is how it slipped by. Oops. |
|
Thanks for being all over this! Overlay updated with 1.2.2. Cheers! |
Instead, we rely on the Spring Security CSRF protection, like we already do for the login page. Additionally, we remove the authentication check in
isApproved, because this is already done by Spring Security (and if not, we have bigger problems to worry about).Resolves #931.
Because this issue impacts core behaviour, I'd think this would be sufficient reason for a release of 1.2.2.