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

two-factor authentication via nextcloud notification broken in snap 22.2.0snap1 & 22.2.0snap2 #1897

Closed
rlKoekie opened this issue Oct 10, 2021 · 6 comments

Comments

@rlKoekie
Copy link

Describe the bug

two-factor authentication via "nextcloud notification" no longer works after upgrading from 22.1.1snap2 to 22.2.0snap1 or 22.2.0snap2. The notification message still pops up on my phone, asking to approve "Login attempt from ::1", but hitting Approve does not proceed the login process. Instead the login on pc stays stuck on the screen asking me to accept the request, or use a backup code.
This worked without problem on 22.1.1snap2 (and before that as well).

The ::1 instead of a proper IP is a different issue, it originates from the snap running behind an Apache reverse proxy, and the snap not using the X_FORWARDED_FOR headers. This has not been an issue before, but maybe due to tightened security it now is?

To Reproduce

Steps to reproduce the behavior:

  1. Open a new browser window, possibly private browsing to make sure you are not logged in
  2. Go to the nextcloud instance, fill in username/password and login
  3. Get the Two-factor authentication dialogue, select Nextcloud notification
  4. Nextcloud notification appears on the phone, hit "approve"
  5. The browser does not proceed to log in

Expected behavior

The browser login is supposed to proceed after hitting "approve" on the phone notification

OS/snapd/snap version

$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 18.04.6 LTS
Release: 18.04
Codename: bionic

$ snap version
snap 2.51.7
snapd 2.51.7
series 16
ubuntu 18.04
kernel 5.4.0-89-generic

(I've reverted back to 22.1.1snap2, as logging in is pretty essential :-) The problem shows on 22.2.0snap2)
$ snap list nextcloud --all
Name Version Rev Tracking Publisher Notes
nextcloud 22.1.1snap2 28549 latest/stable nextcloud✓ -
nextcloud 22.2.0snap2 28586 latest/stable nextcloud✓ disabled

Logs

The debugging output is rather long and contains no errors, or warnings, or other notions timestamped with my failed two-factor authentication attempts. If truly needed I'll try to find a moment to reboot the machine, update the snap, and generate a clean log.

@HansHuckebein7
Copy link

Same here running server on debian 10 (buster). On 22.1.1snap2 everything works fine but on 22.2.x not.
I can't patch the nextcloud-snap read-only filesystem - so I'm waiting that the maintainer applies the patch written @ github:
nextcloud/twofactor_nextcloud_notification#551

@kyrofa
Copy link
Member

kyrofa commented Oct 10, 2021

Thanks for the issue ref @HansHuckebein7, seems this is not a problem with the snap specifically, then. Note that while you cannot patch the contents of the snap itself, you CAN edit third-party apps (i.e. the ones you opt to install, like 2fa). Those are all available in /var/snap/nextcloud/current/nextcloud/extra-apps/. You'll need sudo, though.

@kyrofa kyrofa closed this as completed Oct 10, 2021
@HansHuckebein7
Copy link

The patch described in an other posting (nextcloud/twofactor_nextcloud_notification#551) lists files, I can't find under ../extra-apps/ but I do find them in the read-only part of the snap installation

I need to patch: /core/Middleware/TwoFactorMiddleware.php b/core/Middleware/TwoFactorMiddleware.php
which seems to be "core" part:

root@ts:/snap/nextcloud/current/htdocs/core/Middleware# tree
.
└── TwoFactorMiddleware.php

patch:

diff --git a/core/Middleware/TwoFactorMiddleware.php b/core/Middleware/TwoFactorMiddleware.php
index 2ddfcafa0271..fbdb106b7bb5 100644
--- a/core/Middleware/TwoFactorMiddleware.php
+++ b/core/Middleware/TwoFactorMiddleware.php
@@ -31,6 +31,7 @@
use OC\Core\Controller\LoginController;
use OC\Core\Controller\TwoFactorChallengeController;
use OC\User\Session;
+use OCA\TwoFactorNextcloudNotification\Controller\APIController;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Middleware;
@@ -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') {
    
  •   	// Allow polling the twofactor nextcloud notifications state
    
  •   	return;
    
  •   }
    
  •   if ($controller instanceof TwoFactorChallengeController
      	&& $this->userSession->getUser() !== null
      	&& !$this->reflector->hasAnnotation('TwoFactorSetUpDoneRequired')) {
    

diff --git a/psalm.xml b/psalm.xml
index d51dbb9dde68..a075ceb24a78 100644
--- a/psalm.xml
+++ b/psalm.xml
@@ -80,6 +80,7 @@


  •   		<referencedClass name="OCA\TwoFactorNextcloudNotification\Controller\APIController"/>
      	</errorLevel>
      </UndefinedClass>
      <UndefinedFunction>
    

The directory of the TwoFactor-Middleware lists these files only:

root@ts:/var/snap/nextcloud/current/nextcloud/extra-apps/twofactor_nextcloud_notification# tree
.
├── appinfo
│   ├── info.xml
│   ├── routes.php
│   └── signature.json
├── CHANGELOG.md
├── COPYING
├── js
│   ├── twofactor_nextcloud_notification-challenge.js
│   ├── twofactor_nextcloud_notification-challenge.js.LICENSE.txt
│   ├── twofactor_nextcloud_notification-challenge.js.map
│   ├── twofactor_nextcloud_notification-settings.js
│   ├── twofactor_nextcloud_notification-settings.js.LICENSE.txt
│   └── twofactor_nextcloud_notification-settings.js.map
├── l10n
│   ├── af.js
│   ├── af.json
│   ├── ar.js
│   ├── ar.json
│   ├── ast.js
│   ├── ast.json
│   ├── az.js
│   ├── az.json
│   ├── be.js
│   ├── be.json
│   ├── bg.js
│   ├── bg.json
│   ├── bn_BD.js
│   ├── bn_BD.json
│   ├── br.js
│   ├── br.json
│   ├── bs.js
│   ├── bs.json
│   ├── ca.js
│   ├── ca.json
│   ├── cs.js
│   ├── cs.json
│   ├── cy_GB.js
│   ├── cy_GB.json
│   ├── da.js
│   ├── da.json
│   ├── de_DE.js
│   ├── de_DE.json
│   ├── de.js
│   ├── de.json
│   ├── el.js
│   ├── el.json
│   ├── en_GB.js
│   ├── en_GB.json
│   ├── eo.js
│   ├── eo.json
│   ├── es_419.js
│   ├── es_419.json
│   ├── es_AR.js
│   ├── es_AR.json
│   ├── es_CL.js
│   ├── es_CL.json
│   ├── es_CO.js
│   ├── es_CO.json
│   ├── es_CR.js
│   ├── es_CR.json
│   ├── es_DO.js
│   ├── es_DO.json
│   ├── es_EC.js
│   ├── es_EC.json
│   ├── es_GT.js
│   ├── es_GT.json
│   ├── es_HN.js
│   ├── es_HN.json
│   ├── es.js
│   ├── es.json
│   ├── es_MX.js
│   ├── es_MX.json
│   ├── es_NI.js
│   ├── es_NI.json
│   ├── es_PA.js
│   ├── es_PA.json
│   ├── es_PE.js
│   ├── es_PE.json
│   ├── es_PR.js
│   ├── es_PR.json
│   ├── es_PY.js
│   ├── es_PY.json
│   ├── es_SV.js
│   ├── es_SV.json
│   ├── es_UY.js
│   ├── es_UY.json
│   ├── et_EE.js
│   ├── et_EE.json
│   ├── eu.js
│   ├── eu.json
│   ├── fa.js
│   ├── fa.json
│   ├── fi.js
│   ├── fi.json
│   ├── fo.js
│   ├── fo.json
│   ├── fr.js
│   ├── fr.json
│   ├── gl.js
│   ├── gl.json
│   ├── he.js
│   ├── he.json
│   ├── hr.js
│   ├── hr.json
│   ├── hu.js
│   ├── hu.json
│   ├── hy.js
│   ├── hy.json
│   ├── ia.js
│   ├── ia.json
│   ├── id.js
│   ├── id.json
│   ├── is.js
│   ├── is.json
│   ├── it.js
│   ├── it.json
│   ├── ja.js
│   ├── ja.json
│   ├── kab.js
│   ├── kab.json
│   ├── ka_GE.js
│   ├── ka_GE.json
│   ├── km.js
│   ├── km.json
│   ├── kn.js
│   ├── kn.json
│   ├── ko.js
│   ├── ko.json
│   ├── lb.js
│   ├── lb.json
│   ├── lo.js
│   ├── lo.json
│   ├── lt_LT.js
│   ├── lt_LT.json
│   ├── lv.js
│   ├── lv.json
│   ├── mk.js
│   ├── mk.json
│   ├── mn.js
│   ├── mn.json
│   ├── ms_MY.js
│   ├── ms_MY.json
│   ├── nb.js
│   ├── nb.json
│   ├── nl.js
│   ├── nl.json
│   ├── nn_NO.js
│   ├── nn_NO.json
│   ├── oc.js
│   ├── oc.json
│   ├── pl.js
│   ├── pl.json
│   ├── ps.js
│   ├── ps.json
│   ├── pt_BR.js
│   ├── pt_BR.json
│   ├── pt_PT.js
│   ├── pt_PT.json
│   ├── ro.js
│   ├── ro.json
│   ├── ru.js
│   ├── ru.json
│   ├── sc.js
│   ├── sc.json
│   ├── si.js
│   ├── si.json
│   ├── sk.js
│   ├── sk.json
│   ├── sl.js
│   ├── sl.json
│   ├── sq.js
│   ├── sq.json
│   ├── sr.js
│   ├── sr.json
│   ├── sr@latin.js
│   ├── sr@latin.json
│   ├── sv.js
│   ├── sv.json
│   ├── ta.js
│   ├── ta.json
│   ├── th.js
│   ├── th.json
│   ├── tr.js
│   ├── tr.json
│   ├── ug.js
│   ├── ug.json
│   ├── uk.js
│   ├── uk.json
│   ├── ur_PK.js
│   ├── ur_PK.json
│   ├── uz.js
│   ├── uz.json
│   ├── vi.js
│   ├── vi.json
│   ├── zh_CN.js
│   ├── zh_CN.json
│   ├── zh_HK.js
│   ├── zh_HK.json
│   ├── zh_TW.js
│   └── zh_TW.json
├── lib
│   ├── AppInfo
│   │   └── Application.php
│   ├── BackgroundJob
│   │   └── CleanupTokens.php
│   ├── Controller
│   │   ├── APIController.php
│   │   └── SettingsController.php
│   ├── Db
│   │   ├── TokenMapper.php
│   │   └── Token.php
│   ├── Event
│   │   └── StateChanged.php
│   ├── Exception
│   │   └── TokenExpireException.php
│   ├── Listener
│   │   ├── IListener.php
│   │   └── RegistryUpdater.php
│   ├── Migration
│   │   └── Version000100Date20180411172140.php
│   ├── Notification
│   │   └── Notifier.php
│   ├── Provider
│   │   └── NotificationProvider.php
│   ├── Service
│   │   ├── NotificationManager.php
│   │   ├── StateManager.php
│   │   └── TokenManager.php
│   └── Settings
│   └── Personal.php
└── templates
├── challenge.php
└── personal.php

17 directories, 216 files

@kyrofa
Copy link
Member

kyrofa commented Oct 10, 2021

Ah sad, it seems you're correct, it's part of the API for such apps. We'll need to wait for an upstream update, then.

@HansHuckebein7
Copy link

THANK YOU!

@HansHuckebein7
Copy link

btw. Microsoft was disabling print spooler under Windows 10 for security reasons. Hope you don't disable Two-Factor Login for security reasons ;-)

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

No branches or pull requests

3 participants