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

[EARLY WIP] Move authentication code out of base.php + Two Factor Auth #19752

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@LukasReschke
Member

LukasReschke commented Oct 13, 2015

⚠️ ⚠️ ⚠️ ⚠️ ⚠️ Early WIP - do not merge ⚠️ ⚠️ ⚠️ ⚠️ ⚠️

This refactors the authentication logic out of base.php into it's own controller and also adds support for Two-Factor-Authentication by offering an easy and extensible interface. Also token-based auth and other magic 🙊

It's in an early WIP phase and just there so that people can comment on the approach. Yes, it has bugs at the moment 😉

Todo:

  • Verify that a logout does also delete remember me tokens
  • Verify that Basic Auth is still possible on official API endpoints
  • Add middleware for Apache Auth 
  • Write and fix existing unit tests
  • Full regression testing 
  • Remove example app. That's only here for the reference for now. 
  • Add logic for remember me tokens 
  • Copyright headers 
  • Adjust other endpoints using \OCP\IUserSession::login to check if two factor auth is enabled
    • Make application specific tokens live in core…? Also related to  #19529
  • Once done: Split into proper commits. 

Regular authentication:
regular

Second factor authentication:
second-factor

Valid token:
valid-token

⚠️ ⚠️ ⚠️ ⚠️ ⚠️ Early WIP - do not merge ⚠️ ⚠️ ⚠️ ⚠️ ⚠️

@cmonteroluque

This comment has been minimized.

Show comment
Hide comment
@cmonteroluque

cmonteroluque Oct 13, 2015

Contributor

Yeah, no kidding no merge! LOL

Contributor

cmonteroluque commented Oct 13, 2015

Yeah, no kidding no merge! LOL

@DeepDiver1975 DeepDiver1975 added this to the 9.0-next milestone Oct 13, 2015

*
* @return array
*/
public function getAuthenticationTokens();

This comment has been minimized.

@LukasReschke

LukasReschke Oct 13, 2015

Member

@DeepDiver1975 Thoughts about the API design? This will basically act as password replacement since Two Factor Auth cannot be used with all clients. Would also solve #19529.

Login logic would then basically be:

$user = \OC::$server->getUserManager()->get($username);

if($user instanceof IUser &&
    $user->isTwoFactorEnforced()) {
    if(in_array($password, $user->getAuthenticationTokens(), true)) {
        return true;
    }
    return false;
}

if(\OC_User::login($username, $password)) {
    return true;
} else {
    return false;
}
@LukasReschke

LukasReschke Oct 13, 2015

Member

@DeepDiver1975 Thoughts about the API design? This will basically act as password replacement since Two Factor Auth cannot be used with all clients. Would also solve #19529.

Login logic would then basically be:

$user = \OC::$server->getUserManager()->get($username);

if($user instanceof IUser &&
    $user->isTwoFactorEnforced()) {
    if(in_array($password, $user->getAuthenticationTokens(), true)) {
        return true;
    }
    return false;
}

if(\OC_User::login($username, $password)) {
    return true;
} else {
    return false;
}
* @return bool
*/
public function verifyChallenge($challenge);
}

This comment has been minimized.

@LukasReschke

LukasReschke Oct 13, 2015

Member

Public API. @DeepDiver1975 @MorrisJobke Thoughts?

@LukasReschke

LukasReschke Oct 13, 2015

Member

Public API. @DeepDiver1975 @MorrisJobke Thoughts?

* @return IProvider[]
*/
public function getProvider();
}

This comment has been minimized.

@LukasReschke

LukasReschke Oct 13, 2015

Member

Public API.

@MorrisJobke @DeepDiver1975 Thoughts?

@LukasReschke

LukasReschke Oct 13, 2015

Member

Public API.

@MorrisJobke @DeepDiver1975 Thoughts?

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Oct 14, 2015

Member

as soon as we want various login mechanisms in apps: yes

but is that something we need? no idea

@DeepDiver1975

DeepDiver1975 Oct 14, 2015

Member

as soon as we want various login mechanisms in apps: yes

but is that something we need? no idea

This comment has been minimized.

@LukasReschke

LukasReschke Oct 14, 2015

Member

as soon as we want various login mechanisms in apps: yes

No. This is not related to the normal authentication mechanisms. This is simply for two factor authentication which can be plugged on top of the regular auth mechanism. See the example app that I included.

So what we have in auth now is:

  • Login with regular user credentials
    • Credentials valid
      • Two-Factor enforced: Delegate to second factor authentication app
        • If second factor is valid: Login
        • If second factor is invalid: Show error
      • Two-Factor not enforced: Login
    • Credentials invalid
      • Show error

Thing is that when touching the login we can also add things as such already as it anyways require a full regression testing. The logic of the second-factor auth is NOT in core. Only the registration and so on.

@LukasReschke

LukasReschke Oct 14, 2015

Member

as soon as we want various login mechanisms in apps: yes

No. This is not related to the normal authentication mechanisms. This is simply for two factor authentication which can be plugged on top of the regular auth mechanism. See the example app that I included.

So what we have in auth now is:

  • Login with regular user credentials
    • Credentials valid
      • Two-Factor enforced: Delegate to second factor authentication app
        • If second factor is valid: Login
        • If second factor is invalid: Show error
      • Two-Factor not enforced: Login
    • Credentials invalid
      • Show error

Thing is that when touching the login we can also add things as such already as it anyways require a full regression testing. The logic of the second-factor auth is NOT in core. Only the registration and so on.

This comment has been minimized.

@LukasReschke

LukasReschke Oct 14, 2015

Member
*
* @return string
*/
public function getAuthenticationJavascript();

This comment has been minimized.

@LukasReschke

LukasReschke Oct 13, 2015

Member

I'm personally not too happy with getAuthenticationHtml and getAuthenticationJavascript. This is required since a second factor can also be something more custom such as a U2F smart card which requires to inject JavaScript. It's an a little bit odd API design but I can't think of a better solution to keep it generic.

@LukasReschke

LukasReschke Oct 13, 2015

Member

I'm personally not too happy with getAuthenticationHtml and getAuthenticationJavascript. This is required since a second factor can also be something more custom such as a U2F smart card which requires to inject JavaScript. It's an a little bit odd API design but I can't think of a better solution to keep it generic.

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Oct 14, 2015

Member

What about passing back a url where we redirect to?

That url is then implemented within the auth mechanism/app.

@DeepDiver1975

DeepDiver1975 Oct 14, 2015

Member

What about passing back a url where we redirect to?

That url is then implemented within the auth mechanism/app.

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Oct 13, 2015

Member

i appreciate the idea and work. but this needs to be tought through to understand all the consequences. can we do this once 8.2 is done please? :-)

Member

karlitschek commented Oct 13, 2015

i appreciate the idea and work. but this needs to be tought through to understand all the consequences. can we do this once 8.2 is done please? :-)

@danimo

This comment has been minimized.

Show comment
Hide comment
@danimo

danimo Nov 11, 2015

Contributor

@MTRichards Here's the 2FA PR I was talking about.

Contributor

danimo commented Nov 11, 2015

@MTRichards Here's the 2FA PR I was talking about.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jan 13, 2016

Member

@LukasReschke rebase? kill? 9.1? 🙈

Member

DeepDiver1975 commented Jan 13, 2016

@LukasReschke rebase? kill? 9.1? 🙈

@LukasReschke LukasReschke modified the milestones: 9.1-next, 9.0-current Jan 13, 2016

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@LukasReschke

LukasReschke Jan 13, 2016

Member

Thou shall not simply kill PRs of mine 😉 - Let's talk about it later in 9.1 😄

Member

LukasReschke commented Jan 13, 2016

Thou shall not simply kill PRs of mine 😉 - Let's talk about it later in 9.1 😄

@ChristophWurst ChristophWurst referenced this pull request Apr 22, 2016

Merged

Pluggable auth #24189

32 of 32 tasks complete

LukasReschke and others added some commits Oct 13, 2015

WIP

@ChristophWurst ChristophWurst self-assigned this Apr 25, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 13, 2016

Member

Is this obsoleted by @ChristophWurst's rework ?

Member

PVince81 commented May 13, 2016

Is this obsoleted by @ChristophWurst's rework ?

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst May 13, 2016

Contributor

Is this obsoleted by @ChristophWurst's rework ?

I'd say yes. Both the pluggable auth PR and the 2FA PR are based on this one.

Contributor

ChristophWurst commented May 13, 2016

Is this obsoleted by @ChristophWurst's rework ?

I'd say yes. Both the pluggable auth PR and the 2FA PR are based on this one.

@PVince81 PVince81 closed this May 13, 2016

@PVince81 PVince81 deleted the two-factor-auth branch May 13, 2016

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