No need to handle authentication that early aka in here at all

[DeepDiver1975]

Description

For an unauthenticated request there is no need to try to login implicitly in base.php

Login is done in the login controller.

Related Issue

#30398 (comment)

How Has This Been Tested?

• manually browser

  • various apps load and seem to function properly: activity, files, calendar, contacts

• desktop client with oauth

  • sync folder
  • auto completion of sharee
  • share

• ldap

• shib

• mobile clients

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[PVince81]

As discussed, let's see where CI crashes

[SamuAlfageme]

I just got same results as @DeepDiver1975 with the patch; no more traces of the error neither with browser login nor with basic/OAuth2-based logins on the client. I'll check with a mobile client next.

Appart from this; I'm assuming Shibboleth-auth flow was never affected by those errors; as the un-patched 10.0.7 never displayed the "Login failed" messages.

I've also verified that using the app. passwords together with --nonshib flag on the cli client (w/ mapped-user:app-password, being mapped-user ownCloud's, not IDP's) works with no logs.

Still a couple of interesting scenarios I need to check:

• OAuth2 wrapping shibboleth and using an app-password (extra security FTW)
• LDAP provisioned users.

Very likely that these are just fine.

[SamuAlfageme]

Wrapped-Shibboleth-by-OAuth2 also seems to be working, no messages whatsoever.

@davitol will get to check with LDAP tomorrow. Instructions on how to replicate a proper testing scenario are explained in #30398 (comment)

Just, additionally, apply this patch (and the #30365 + owncloud/oauth2#112 combo) and:

1. Install user_ldap connect it to an actual LDAP server.
2. Login via web UI with one of the LDAP users, generate an application password
3. Log out of the browser
4. Use it to log in from the desktop client, if OAuth2 is correctly enabled, the browser will spawn a new tab. Input the app pass. in that form.
5. Observe for exceptions and/or 'Login failed' messages on the Apache/ownCloud logs.

[PVince81]

I still suspect that there are scenarios that did require the current code path.
The scenarios in question might be:

• authentication with basic auth / token auth when connecting to any app framework route (non-OCS, might need third party app with such routes)
• authentication with basic auth against OCS route (capabilities API ?)

[DeepDiver1975]

I still suspect that there are scenarios that did require the current code path.
The scenarios in question might be:

authentication with basic auth / token auth when connecting to any app framework route (non-OCS, might need third party app with such routes)
authentication with basic auth against OCS route (capabilities API ?)

as shown by the integration tests: https://jenkins.owncloud.org/job/owncloud-core/job/core/job/PR-30421/1/#showFailuresLink

[SamuAlfageme]

@PVince81 @DeepDiver1975 of course, a full smoke test will be in place once we build RC2. As said, I've been mostly focusing on reviewing the logs on client/browser auth. scenarios.

[codecov]

Codecov Report

Merging #30421 into master will increase coverage by 0.02%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             master   #30421      +/-   ##
============================================
+ Coverage     61.97%      62%   +0.02%     
- Complexity    19080    19085       +5     
============================================
  Files          1091     1091              
  Lines         61492    61501       +9     
============================================
+ Hits          38109    38131      +22     
+ Misses        23383    23370      -13

Impacted Files	Coverage Δ	Complexity Δ
lib/base.php	4.21% <ø> (+1.47%)	153 <0> (ø)	⬇️
...AppFramework/Utility/ControllerMethodReflector.php	100% <ø> (ø)	10 <0> (ø)	⬇️
lib/private/legacy/json.php	9.52% <0%> (-0.65%)	27 <0> (+2)
...e/AppFramework/DependencyInjection/DIContainer.php	72.43% <100%> (-0.71%)	81 <0> (ø)
...amework/Middleware/Security/SecurityMiddleware.php	98.36% <100%> (+0.17%)	22 <3> (+3)	⬆️
lib/private/User/Session.php	66.75% <0%> (+2.75%)	144% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9317676...ae97bff. Read the comment docs.

[SamuAlfageme]

I'm getting the "Login Failed" error message once again when using the app password during the OAuth2 auth8n flow. Not sure if everything is in place (since I'm applying 3 different patches on top of 10.0.7 RC1), tho 😕

The test environment is getting quite hard to setup IMO. Could we move/merge everything on a branch to make reproducing easier?

0. Start from 10.0.7 RC1
1. Apply Ensure that the basic auth module is the last module in the list #30398
2. Apply No need to handle authentication that early aka in here at all #30421
3. Apply Bugfix/owncloud/oauth2#103 #30365
4. Apply Throw an exception if the Bearer header holds an invalid token oauth2#112

Then, just generating and using an app password during the initial OAuth2 setup (like described in #30398 (comment)) causes the logs to spit "Login failed" once again.

[PVince81]

@DeepDiver1975 is this worth continuing now that we have the other fix ?

[DeepDiver1975]

@DeepDiver1975 is this worth continuing now that we have the other fix ?

we have no fix yet for 'login failed' on public routes which have basic auth in their headers.
this pr is the only solution ....

[PVince81]

Concerns about "setupFS" happening before the user is actually logged in.

• To test: whether external storage with "use user login" still works
• check if public link page works (with and without session, and with link password)

[PVince81]

👍 code looks fine, as discussed with @DeepDiver1975 and concerns were adressed

[PVince81]

merge or do more tests ? we'll anyway need to redo regression testing for all auth-related things as per owncloud/QA#529

[PVince81]

@DeepDiver1975 please backport to stable10

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.