michaelryanmcneill / shibboleth

Shibboleth plugin for WordPress
https://wordpress.org/plugins/shibboleth/
19 stars 11 forks source link

auth: Blank Shibboleth role means use existing WordPress access #37

Closed jrchamp closed 6 years ago

jrchamp commented 6 years ago

shibboleth_get_user_role() is currently used for three things:

  1. When creating a new account, it provides the initial role to set for the new account.
  2. When role updates are enabled, it provides the role to set for the existing account.
  3. When the user is logging in, if the value returned is blank, it prevents access.

The third use is not valid. shibboleth_create_new_user() already checks whether accounts can be created. shibboleth_authenticate_user() already checks to make sure an account exists. And most importantly, if the user account exists and is already tied to Shibboleth, our job as an authentication mechanism is to map the user to the user's account. Authorization should be done by WordPress based on roles that have been assigned to the user.

With this change in place, a couple things start working:

jrchamp commented 6 years ago

@michaelryanmcneill If you have any questions, please ask. I feel pretty good about the code, but I'd like a critical second opinion.

michaelryanmcneill commented 6 years ago

I want to make sure that we still support the constants. Can you add that support in? I'll get it merged once that happens.

jrchamp commented 6 years ago

Ok, I've added the check for SHIBBOLETH_DEFAULT_ROLE. It looks like the readme and admin-options already have this constant in place.

Do you want me to modify this sentence?

Leave this constant empty '' to make the default no allowed access.

It might be fine as is, but I get the feeling that "no role" is more accurate than "no access".

michaelryanmcneill commented 6 years ago

Sorry for the delay, I've just been swamped. Trying to get eyes on this to get a release moving.