Automattic / vip-support

Manages the VIP Support Users on your site
GNU General Public License v2.0
12 stars 4 forks source link

Add support for restricting allowed users by email #96

Closed nickdaugherty closed 2 years ago

nickdaugherty commented 2 years ago

This allows individual sites to restrict which support users have access by defining VIP_SUPPORT_USER_ALLOWED_EMAILS to an array of allowed Automattician email addresses.

Can be used to limit VIP Support access to a subset of VIP staff.

WPprodigy commented 2 years ago

Looks like the new method is just used in is_verified_automattician(), which is only used in is_automattician() check in mu-plugins. That function is used for a few things, but I don't think login restrictions?

So if the goal here is to prevent adding support users except from supported email addresses, I think maybe it doesn't do that quite yet?

nickdaugherty commented 2 years ago

@WPprodigy the permission cap filter uses is_proxied_automattician() (#), which in turn uses is_automattician() (#)...which itself uses is_verified_automattician() (#) to grant the permission.

It's quite a tangle - I'd really like to get this repo merged into mu-plugins, I think the overhead of having it separate is too high.

WPprodigy commented 2 years ago

Ah I see, it is quite circular 😬 . "There and back again, an authentication check by bilbo baggins".

WPprodigy commented 2 years ago

Yeah I do agree that usage in the future could become confusing (I mean kinda already is!). Suppose we could clean it all up when we merge this into mu-plugins, but I'm thinking a more clear top-level api would be nice to have. Such as:

The first would be used in most places still, but then is_automattician_with_login_access() would help hide the extra logic away and would always be used when determining if the user can login / can be added / has capabilities access.

nickdaugherty commented 2 years ago

Those are good ideas - since mu-plugins calls this directly, and that's a separate repo, it's a harder to coordinate the changes if we were to rename this function, since mu-plugins calls it already, so it may not be worth the effort at this point. I like the name of is_allowed_email()...but I do agree that is_verified_automattician() is now slightly misnamed.

WPprodigy commented 2 years ago

Should be alright to ship this and deal with cleanup after we've merged repos.