Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow "TwoFactor Nextcloud Notifications" to pull the state of the 2FA again #29056

Conversation

@nickvergessen
Copy link
Member Author

/backport to stable22

@nickvergessen
Copy link
Member Author

/backport to stable21

…A again

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/allow-twofactor-nextcloud-notifications-to-work-again branch from 35c115d to 3710eca Compare October 4, 2021 09:26
@nickvergessen nickvergessen requested review from a team, PVince81, ArtificialOwl and icewind1991 and removed request for a team October 4, 2021 09:36
@nickvergessen
Copy link
Member Author

/backport to stable20

@nickvergessen nickvergessen changed the title Allow "TwoFactor Nextcloud Notifications" to pull the state of the 2F… Allow "TwoFactor Nextcloud Notifications" to pull the state of the 2FA again Oct 4, 2021
Comment on lines +86 to +89
if ($controller instanceof APIController && $methodName === 'poll') {
// Allow polling the twofactor nextcloud notifications state
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work if the app is not locally installed? - If yes: LGTM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik doing instanceof with a non existing class gracefully returns false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://3v4l.org/cnR6V

Yes it works, same as ::class (which is what this uses internally I guess)

@nickvergessen nickvergessen merged commit e597a5a into master Oct 4, 2021
@nickvergessen nickvergessen deleted the bugfix/noid/allow-twofactor-nextcloud-notifications-to-work-again branch October 4, 2021 15:17
@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable20 failed. Please do this backport manually.

@@ -82,6 +83,11 @@ public function __construct(Manager $twoFactorManager, Session $userSession, ISe
* @param string $methodName
*/
public function beforeController($controller, $methodName) {
if ($controller instanceof APIController && $methodName === 'poll') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we could have fixed this bug without introducing app-specifics into the server code base? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could introduce another annotation which is like @PublicPageButWith(out)ExtraSteps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants