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

Pluggable auth #24189

Merged
merged 26 commits into from
May 11, 2016
Merged

Pluggable auth #24189

merged 26 commits into from
May 11, 2016

Conversation

ChristophWurst
Copy link
Contributor

@ChristophWurst ChristophWurst commented Apr 22, 2016

This revives #1975

I removed the two-factor code from this PR to keep it simple.

TODO:

  • Create session token for new browser session (login)
    • Save hashed token + encrypted login password
  • Invalidate (delete) session token on logout
  • Add background job to kill old session tokens
    • Don't kill permanent client tokens
  • Check browser session on every request -> logout if invalidated/not found
  • Check login credentials each x minutes and log the user out if the check fails
  • Update token 'last_activity' in every request
  • Invalidate tokens if the session is killed in base.php? -> Invalidate browser session token when maximum session lifetime is reached #24542
  • Add controller where clients can get a new access token
    • Can be tested with curl --request POST -F 'user=youruser' -F 'password=yourpassword' localhost:8080/token/generate
  • Don't use sessions if the client sends an auth token -> Don't use session if the client uses an auth token #24543
  • Add DB index on 'last_activity'
  • Add token type column
  • Migrate OCS auth
  • Adjust sabre auth backend
  • Increase version number to trigger db migration
  • Fix existing unit tests
  • Add new unit tests for token auth
  • Fix existing integration test
  • Fix apache auth (tested with shibboleth sso)
  • Show login errors
  • Add new integration tests for token auth
  • Squash/arrange commits

BUGS:

  • setup is broken
  • user agent is always undefined, so the default is used
  • new user can not log in
  • Changing own password kicks you out, hence the session token needs to be updated on password change -> Changing own password logs you out #24544
Exception: {"Exception":"OCP\\Files\\NotFoundException","Message":"","Code":0,"Trace":"#0 \/home\/christoph\/workspace\/owncloud\/apps\/files\/controller\/viewcontroller.php(118): OC_Helper::getStorageInfo('\/', false)\n#1 \/home\/christoph\/workspace\/owncloud\/apps\/files\/controller\/viewcontroller.php(180): OCA\\Files\\Controller\\ViewController->getStorageInfo()\n#2 [internal function]: OCA\\Files\\Controller\\ViewController->index('', '')\n#3 \/home\/christoph\/workspace\/owncloud\/lib\/private\/AppFramework\/Http\/Dispatcher.php(159): call_user_func_array(Array, Array)\n#4 \/home\/christoph\/workspace\/owncloud\/lib\/private\/AppFramework\/Http\/Dispatcher.php(89): OC\\AppFramework\\Http\\Dispatcher->executeController(Object(OCA\\Files\\Controller\\ViewController), 'index')\n#5 \/home\/christoph\/workspace\/owncloud\/lib\/private\/AppFramework\/App.php(110): OC\\AppFramework\\Http\\Dispatcher->dispatch(Object(OCA\\Files\\Controller\\ViewController), 'index')\n#6 \/home\/christoph\/workspace\/owncloud\/lib\/private\/AppFramework\/Routing\/RouteActionHandler.php(45): OC\\AppFramework\\App::main('OCA\\\\Files\\\\Contr...', 'index', Object(OC\\AppFramework\\DependencyInjection\\DIContainer), Array)\n#7 [internal function]: OC\\AppFramework\\Routing\\RouteActionHandler->__invoke(Array)\n#8 \/home\/christoph\/workspace\/owncloud\/lib\/private\/route\/router.php(280): call_user_func(Object(OC\\AppFramework\\Routing\\RouteActionHandler), Array)\n#9 \/home\/christoph\/workspace\/owncloud\/lib\/base.php(871): OC\\Route\\Router->match('\/apps\/files\/')\n#10 \/home\/christoph\/workspace\/owncloud\/index.php(39): OC::handleRequest()\n#11 {main}","File":"\/home\/christoph\/workspace\/owncloud\/lib\/private\/helper.php","Line":605}
  • Not possible to add an account to the desktop client – I suspect the login url change to cause that. @LukasReschke what are we gonna do about that? It works, I was testing with wrong credentials 🙈
  • "Unable to generate a URL for the named route \\\"login#showLoginForm\\\" as such route does not exist.
  • Adjust column width
    ORA-12899: value too large for column "AUTOTEST"."oc_authtoken"."token" (actual: 128, maximum: 100)
  • username !== uid, Pluggable auth #24189 (comment)
  • Login with email doesnt work

Next steps (follow-up PRs):

@ChristophWurst ChristophWurst added this to the 9.1-current milestone Apr 22, 2016
<default_enable/>
<types>
<!-- this is used to disable the feature of enabling an app for specific groups only because this would break this app -->
<filesystem/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove, authentication also blocks disabling such apps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This provider app example will be removed, it's just there for testing purposes

@ChristophWurst ChristophWurst changed the title Pluggable auth Pluggable auth / 2FA Apr 22, 2016

/**
* @param string $token
* @return boolean|string user UID if token is valid, false when invalid
Copy link
Member

Choose a reason for hiding this comment

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

I'm personally not a huge fan of APIs that return multiple types. Can we make it throw an exception when invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, using an exception sounds better

@ChristophWurst ChristophWurst force-pushed the pluggable-auth branch 2 times, most recently from b51cfe1 to 9efb81f Compare April 25, 2016 07:40
@ChristophWurst ChristophWurst force-pushed the pluggable-auth branch 2 times, most recently from 94e9f01 to a9de92b Compare April 25, 2016 12:10
@ChristophWurst ChristophWurst changed the title Pluggable auth / 2FA Pluggable auth Apr 25, 2016
* @param array $errors
* @param string[] $messages
*/
public static function displayLoginPage($errors = array(), $messages = []) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that like, dead in master? :)

@guruz
Copy link
Contributor

guruz commented May 18, 2016

Clients should switch to token auth if the server supports it

@dragotin @danimo How does this relate to the discussions you had yesterday?

@lock
Copy link

lock bot commented Aug 5, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants