OSC / ood_auth_registration

(DEPRECATED - we now use Keycloak for identity brokering) OSC OnDemand Open ID Connect CI Logon Registration page
MIT License
1 stars 1 forks source link

Limit the registration page's concern to authentication and not authorization #1

Open ericfranz opened 8 years ago

ericfranz commented 8 years ago

Currently we mix authentication code (does the username/password combo successfully bind to LDAP) with authorization code (is the account of the user that authenticated an enabled account via LDAP attribute or shell).

  1. We can remove the code that deals with authorization: https://github.com/OSC/ood_auth_registration/blob/4b60441e0a28d9164d64a0921c240adeb63aadf8/ldap.php#L160-L193
  2. Also we can safely remove the find_ldap function here: https://github.com/OSC/ood_auth_registration/blob/4b60441e0a28d9164d64a0921c240adeb63aadf8/ldap.php#L67-L116
  3. Finally, we can create a new ldap authentication object and an Auth super class. Auth subclasses can reside in a authn_strategies directory - so new ones can just be added.
$auth = LDAPAuthStrategy.new($attrs);
if($auth->authenticate($username, $password)){
  // do mapping
}

Could have a super class with a factory method that instantiates the correct subclass. So:

$auth = AuthStrategy::from($config)
if($auth->authenticate($username, $password)){
  // do mapping
}

Where $config is a string or a path to a file that is YAML or PHP:

authclass: LDAPAuth
attrs:
  primary: ldaps://xxx01.osc.edu:636
  secondary: ldaps://xxx02.osc.edu:636

Of course, this opens the question of where authorization should go.

(above is the Strategy pattern: https://en.wikipedia.org/wiki/Strategy_pattern and https://sourcemaking.com/design_patterns/strategy)

ericfranz commented 8 years ago

So to clarify, the question is: should a user be authorized prior to adding the mapping to the gridmap file? If so, is it the responsibility of the registration PHP script or the responsibility of the mapdn script to authorize the user?

ericfranz commented 8 years ago

FWIW a user's PUN cannot be started if their shell is disabled - we have a check for this.