Redirect guests to login if they follow the link of a comment mention… #11185
Conversation
nickvergessen
added
bug
3. to review
| * @NoCSRFRequired | ||
| * | ||
| * @param string $id the comment ID | ||
| * @return Response | ||
| */ | ||
| public function view($id) { | ||
| $currentUser = $this->userSession->getUser(); | ||
| if (!$currentUser instanceof IUser) { | ||
| return new RedirectResponse( |
There was a problem hiding this comment.
it's a bit of a hack. Would be more comfortable if that was dealt with on the middleware-level
There was a problem hiding this comment.
Yeah, fine by me. The middleware would basically do the same. Just instead of throwing an exception it makes a redirect response...
There was a problem hiding this comment.
Sure, but then we have it at one, reusable place, don't need to soften the restrictions of the controller and are less likely to refactor it away accidentally.
There was a problem hiding this comment.
I would still prefer this to be backported to 14 instead of a middleware change.
Also for the middleware, the constructor still needs to be changed to get rid of Folder I think, but we could test that later.
The other thing I'm not sure about is, if we should do that for all pages which are not PublicPage or if it should be explicit, because it makes no sense to do this for API calls.
There was a problem hiding this comment.
Fair enough.
If we can avoid another annotation we should go for it… I share your doubts about it, too. Thus tending towards an explicit statement.
There was a problem hiding this comment.
So why isn't the default logic kicking in (to redirect if not logged in)?
Anyway lets do this as it is a bit specific to comments anyway.
…-notification Signed-off-by: Joas Schilling <coding@schilljs.com>
rullzer
force-pushed
the
bugfix/noid/redirect-guests-to-login-on-mention-notification
branch
from
dfb0ff0
to
e375107
Compare
rullzer
deleted the
bugfix/noid/redirect-guests-to-login-on-mention-notification
branch
…-notification
Fix #11184