Skip to content

AppAPI: allow to bypass Two-Factor - #42479

Merged
bigcat88 merged 1 commit into
masterfrom
appapi-twofactor
Dec 29, 2023
Merged

AppAPI: allow to bypass Two-Factor#42479
bigcat88 merged 1 commit into
masterfrom
appapi-twofactor

Conversation

@bigcat88

@bigcat88bigcat88 commented Dec 25, 2023

Copy link
Copy Markdown
Member
  • Resolves: AppAPI with TwoFactor Auth enabled

Summary

AppAPI must support passing two-factor authentication so that ExApps can work in such systems.
By design they should do so.

Original issue where it was discovered(we just completely forgot about two-factor auth):
nextcloud/app_api#181

Since the context_chat was already released we should provide a fix for this situation.
Also the backport to stable28 branch is required, as this is a bug that blocks all Apps developed for AppAPI to work in most production systems.
If possible, also a backport to stable27 branch will be nice.

Ref:

Checklist

if ($this->session->exists('app_password') || $this->twoFactorManager->isTwoFactorAuthenticated($user)) {
if ($this->session->exists('app_password')
|| $this->session->exists('app_api')
|| $this->twoFactorManager->isTwoFactorAuthenticated($user)) {

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 1 of OC\Authentication\TwoFactorAuth\Manager::isTwoFactorAuthenticated cannot be null, possibly null value provided
@bigcat88bigcat88 changed the title AppAPI: allowed to bypass Two-FactorAppAPI: allow to bypass Two-FactorDec 25, 2023
@AndyScherzinger
AndyScherzinger requested review from sorbaugh and removed request for AndyScherzingerDecember 25, 2023 21:20
Comment threadcore/Middleware/TwoFactorMiddleware.php Outdated
bigcat88 added a commit to nextcloud/app_api that referenced this pull request Dec 27, 2023

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good but didn't test

Please squash your commits into one

@juliusknorr

juliusknorr commented Dec 28, 2023

Copy link
Copy Markdown
Member

CI failure seems related, please fix and squash in this PR

Time: 00:26.907, Memory: 193.03 MB
There was 1 failure:
1) Test\Authentication\TwoFactorAuth\ManagerTest::testNeedsSecondFactorSessionAuth
Expectation failed for method name is "get" when invoked 1 time(s)
Parameter 0 for invocation OCP\ISession::get('app_api') does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'two_factor_auth_passed'
+'app_api'
/drone/src/lib/private/Authentication/TwoFactorAuth/Manager.php:322
/drone/src/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php:640
--

@bigcat88
bigcat88force-pushed the appapi-twofactor branch 2 times, most recently from ce4c53b to 489b816CompareDecember 28, 2023 14:36
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
@bigcat88
bigcat88 merged commit cdc2723 into masterDec 29, 2023
@bigcat88
bigcat88 deleted the appapi-twofactor branch December 29, 2023 08:29
@bigcat88

Copy link
Copy Markdown
MemberAuthor

/backport to stable28

@bigcat88

Copy link
Copy Markdown
MemberAuthor

/backport to stable27

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsbug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@bigcat88@juliusknorr@ChristophWurst@andrey18106@github-advanced-security