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

PHP 8 Support #426

Closed
sunjam opened this issue Feb 26, 2021 · 10 comments · Fixed by #449
Closed

PHP 8 Support #426

sunjam opened this issue Feb 26, 2021 · 10 comments · Fixed by #449

Comments

@sunjam
Copy link

sunjam commented Feb 26, 2021

Related to #425 php8 support was introduced in v21. Thanks

@ChristophWurst
Copy link
Member

I don't have time to maintain this app right now.

@nursoda
Copy link
Contributor

nursoda commented Mar 20, 2021

Wow, that had quite severe consequences for me after migrating to NC21 and PHP8: I use 'enforce 2FA'.

One of my users had enabled SMS (via gateway) only (not even backup codes). He could not log on even as I put him in a group that was excluded from enforcement: He only gets a warning that a configured 2FA provider isn't available and a button to set up 2FA – but pressing it results in the same dialog. (1st/user deadlock situation)

So I tried to clean the 2FA associations using OCC:

$ php occ twofactorauth:cleanup USER
All user-provider associations for provider USER have been removed.
$ php occ twofactorauth:state USER
Two-factor authentication is enabled for user USER
Enabled providers:
- gateway_sms
Disabled providers:
- admin
- backup_codes
- email
- gateway_signal
- gateway_telegram
- totp
- twofactor_nextcloud_notification
- u2f

Obviously without result, probably because the app is not working due to NC21/PHP8 incompatibilities. (2nd/admin deadlock situation)

So I tried to disable twofactor_gateway, but occ yielded 'no such provider id'. In the end, I just deleted the app folder but even that didn't solve the above user issue.

Well, yes, I could delete a line in the database, but I try to avoid that at all cost to keep my DB consistent.

In my opinion, this is a general design issue since it happens that apps aren't maintained temporarily or permanently (see Christoph's message above). In such case, Nextcloud should provide proper means to delete associations independent of the availability of apps that were available in a former NC version.

Question: Does this belong here or should a bug be opened somewhere else?

@ChristophWurst
Copy link
Member

In such case, Nextcloud should provide proper means to delete associations independent of the availability of apps that were available in a former NC version.

I'm not sure I can follow. Yes, this app is mostly community driven, hence at times we can do more, other times less maintenance. The app isn't released for Nextcloud 21 nor php8, so we also do not overpromise. Ref https://apps.nextcloud.com/apps/twofactor_gateway.

@nursoda
Copy link
Contributor

nursoda commented Mar 22, 2021

@ChristophWurst Sorry, no offense intended. I just think that the overall system NC (not the app) should provide means to mitigate culprits experienced from "upgrades + no longer available apps". This is a general requirement to long-term system stability.

@ChristophWurst
Copy link
Member

None taken :)

We didn't intentionally abandon the app. We still want to make it available to people. But at this point there are no volunteers to drive the project, hence it stalled.

boppy added a commit to boppy/twofactor_gateway that referenced this issue Apr 3, 2021
Changed Telegram API Class to one not having a Guzzle requirement. Tested with NC 21.0.0 on PHP 8.0.3 (on Alpine Linux Docker Container).

Fixes nextcloud#426 and nextcloud#425
@boppy
Copy link
Collaborator

boppy commented Apr 7, 2021

Can (at least) one of you verify my PR in #438 is working? I tested it with a fresh installation but it would be nice, if you could verify...

@jandr
Copy link

jandr commented May 19, 2021

I downloaded the contents of your PR and replaced my twofactor_gateway folder within the apps directory. I re-enabled the app in my settings, and when I tried logging in, the configured options were available (e.g. Telegram). However, when attempting to use the gateway I was faced with the following error in the URL /login/challenge/gateway_telegram :

Internal Server Error
The server was unable to complete your request.

If this happens again, please send the technical details below to the server administrator.

More details can be found in the server log.

Technical details
Remote Address: 192.168.40.10
Request ID: yGVOk3rGXmrBBWuMdUN4

I don't see anything relevant in my server logs, though, to identify what the error or problem could be, as the only log line I see is:

192.168.40.10 - - [19/May/2021:08:54:51 +0000] "GET /index.php/login/challenge/gateway_telegram?redirect_url=/index.php/apps/dashboard/ HTTP/1.1" 500 5521 "-" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.212 Safari/537.36"

Nothing else shows up on PHP's log or in Apache's error.log.

Probably just replacing the old app with the new files might not be enough, so any pointers on how to update this in a live installation will help test the app there so we can provide feedback on the PR.

@boppy
Copy link
Collaborator

boppy commented May 19, 2021

See my Comment here. You might want to try with a fresh app installation by:

cd apps
mv twofactor_gateway twofactor_gateway-0.17.0
git clone https://github.com/boppy/twofactor_gateway.git
cd twofactor_gateway
git checkout php8-nc21
composer install
npm install
npm run build

@jandr
Copy link

jandr commented May 19, 2021

I have checked this on NC21 with PHP8.0, on an installation that was running version 0.17 of twofactor_gateway, and with the instructions stated, I can confirm it worked for me flawlessly.

@nursoda
Copy link
Contributor

nursoda commented May 21, 2021

I confirm that boppys modified app works fine for SMS on PHP8 (Arch NGINX PHP-FPM MariaDB) with NC 21.0.2.
However, the PHP 7.2 dependency needed to be removed from composer.json (composer config --unset platform) and there are warnings on composer install (and even more on composer update), and the instructions above might improve using composer install --no-dev. They also omit how to clean out dev files after building. I used rm -rf composer.* krankerl.toml package* screenshots tests node_modules.

seyfahni pushed a commit to seyfahni/twofactor_gateway that referenced this issue Jun 3, 2021
Changed Telegram API Class to one not having a Guzzle requirement. Tested with NC 21.0.0 on PHP 8.0.3 (on Alpine Linux Docker Container).

Fixes nextcloud#426 and nextcloud#425
boppy added a commit that referenced this issue Jun 6, 2021
Changed Telegram API Class to one not having a Guzzle requirement. Tested with NC 21.0.0 on PHP 8.0.3 (on Alpine Linux Docker Container).

Fixes #426 and #425

Signed-off-by: Henning Bopp <henning.bopp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants