Repository navigation
[LINKED][Improvement] A more flexible Auth:checkAuthentication with arguments #701
Description
EDITED to address some concerns, see below
After seeing the new Auth::checkAdminAuthentication() I was excited because I had already implemented a similar class elsewhere and decided to merge the two.
The idea is that you only need the one function Auth:checkAuthentication() and you can supply arguments to it. If left alone with no argument, it will perform the usual check to see if a user is logged in, and if so, it will treat the user as having a default account 1.
Yet if you supply an argument, say 7, it will redirect the user to the dashboard if their user account type does not equal 7, else it will allow the protected view to be rendered. All assuming the user is actually logged in in the first place.
So, to use it is simple....very simple. Add the function to any constructor of a controller or inside a controller-method to limit the users access.
I would love to know what you think!!
Here is an Example of how to use it
class AdminController extends Controller
{
public function __construct()
{
parent::__construct();
Auth::checkAuthentication(7);
}
public function index()
{
$this->View->render('admin/index');
}
}And Here is the actual Auth Class
class Auth
{
// Notice the argument $type is set to 1 which is the default user account type, adjust accordingly
public static function checkAuthentication($type = 1)
{
// Initialize Session if it doesn't exist
Session::init();
//If user is not logged in, end of story, redirect
if (!Session::userIsLoggedIn()) {
// Destroy the session and redirect
Session::destroy();
header('location: ' . Config::get('URL') . 'login?redirect=' . urlencode($_SERVER['REQUEST_URI']));
exit();
}
/**
* If user account type is not 1 (default) and user type does not match supplied argument, redirect to dashboard
* Notice: the default user account type is set to 1 here as well
*/
if( 1 != $type AND Session::get("user_account_type") != $type){
/**
* Important! Do not supply an argument when using a controller that matches the redirect or this will
* cause a loop ending in a redirect failure!
* ie. dashboard or dashboard/index can use Auth::checkAuthentication() but not Auth::checkAuthentication($type)
*/
header('location: ' . Config::get('URL') . 'dashboard/index');
exit();
}
}
}EDITS - For those who like keeping the additional Auth::checkAdminAuthentication(), and would also like a way to check other account types
If you want to keep the Admin separated
public static function checkAdminAuthentication()
{
$admin = 7;
self::checkAuthentication($admin);
}If you want to add other levels such as a Premium Member, or whatever you may desire.
public static function checkPremiumAuthentication()
{
$premium = 3;
self::checkAuthentication($premium);
}With this proposed class, you can perform shortcuts to the above same methods using...
Auth::checkAuthentication() // Basic Member
Auth::checkAuthentication(3) // Premium Member
Auth::checkAuthentication(7) // Admin...without the need to make any specific methods such as...
public static function checkPremiumAuthentication() {}
// or
public static function checkAdminAuthentication() {}
Activity
OmarElgabry commentedon Aug 19, 2015
If user's account type doesn't match this seems a critical security issue, you need to destroy the session and kick the current user out to your login form,
Another thing is, keeping two method could be handy if you need to extend or change the way you authenticate for admins only or for any user, having one method could result in having in one BIG method later; better code resuse and maintainable code
videsignz commentedon Aug 19, 2015
@OmarElgabry I don't see how that is a security issue...Why would you need to destroy the session and kick the user to login screen? The user is logged in at this point so they at least have a 'basic' account. All we do now is compare which account type they have.
As far as separating the methods, I personally just don't see the need. Up until a month ago this whole project had one small authentication method. Is there really a need to complicate it and make it bloated? If someone really has the need to add a custom method, its easy to do.
OmarElgabry commentedon Aug 19, 2015
What do you think about a legitimate normal user trying to access actions performed only by admins(these actions are hidden by default)?
What you are suggesting is the complicated, less resuse & maintainable version. If someone needs to add a new method say adminAuthentication(), he has to change all places in the code that needs admin authentication, and if someone needs to add more checking for admin, your function will be BIG and needs to be separated. Always keep a method for normal users and another one for admins.
I am not a big fan of long conversations, I told you what I'm sure of based on my knowledge and good understanding, So, take it or leave it.
videsignz commentedon Aug 19, 2015
@OmarElgabry
I am trying to understand where you are coming from, but your points are not clear to me.
This particular class
Authhas a very simple purpose and use...restrict access. Don't you agree??I'm looking at the big picture here. For example, on my website we have Non Members and 4 levels of account types...Member (1), Agent (2), Agent Admin (3), & Admin (4).
Using my class is ideal for this (When used in the constructor of a controller or a controller method)
To allow access to All Members, I would simply put
Auth:checkAuthentication();(No argument)To allow access to Only Agents, I would simply put
Auth:checkAuthentication(2);To allow access to Only Agent Admins only, I would simply put
Auth:checkAuthentication(3);To allow access to Only Admins only, I would simply put
Auth:checkAuthentication(4);Each one of these calls will redirect users that are not logged in first and foremost...period.
No matter what, when you start adding methods you are going to have to change your code somewhere else, so your point on that does not make sense.
Overall I guess we will have to agree to disagree. Maybe if you tested this class out you would see the benefits.
panique commentedon Aug 19, 2015
Easy gentlemen! :)
In general in would say: Let's make things as simple as possible, only catching the most common use case! Lot's of nice projects were cool in the beginning but then got bloated and unusable...
videsignz commentedon Aug 19, 2015
@panique Haha, things got heated in here ;)
This to me is simple. Handles both of the current methods within one method, yet can authenticate more than just a normal user and admin, using less code. I am having trouble seeing the downside. Maybe you could explain it better than Omar?
videsignz commentedon Aug 19, 2015
@OmarElgabry Would adding this agree with your "knowledge and good understanding"?
That way if you change the account type of an admin, you would only need to change it within this method :)
OmarElgabry commentedon Aug 19, 2015
To end up the story, do whatever you like, What i want to say(that's actually what forced me to comment on this issue), if you ever got a wrong account type, this is seems a security issue, and you need to destroy the session, and log out the current user. As i said before, any normal user can send a request to perform an action that can be only performed ONLY by admin, keep in mind normal users can't see actions of admins(like delete, edit user).
panique commentedon Aug 19, 2015
Please let us keep this ticket open for a while and collect some opinions from others! Please also let's keep this project super-peaceful and respectful, we are doing this here for free, for fun and in our freetime, so no stress :)
videsignz commentedon Aug 19, 2015
Sounds good to me @panique !
My case for this is simple....
Currently, to restrict access to members only, we say
if (!Session::userIsLoggedIn())Currently, to restrict access to admins only, we say
if (!Session::userIsLoggedIn() || Session::get("user_account_type") != 7)All I am doing is adding flexibility to the rigid number 7, allowing a check for different levels of users (Premium Member for example) and doing it with less code, that's all.
Ultimately, the 'security' rests only on this one statement...
if (!Session::userIsLoggedIn() || Session::get("user_account_type") != 7)If you have faith in this, then my class is just as trustworthy as these are both included in my class.
@OmarElgabry Lastly, to your points
Developer error is always a possibility (so the additional method I added back in would minimize the chance of error)
What makes you say this? It is just not true. Whether you use
Auth:checkAdminAuthentication()orAuth:checkAuthentication(7)the result is the same. The restriction is in the AdminController so both would returnfalseand prevent the view from being rendered and the action from being executed. Try it out.Can we both agree that if the user is logged in they at least have an account?
Assuming you agree, my class checks their account type, and if it doesn't match, they are treated as basic low level members and redirects them back to their dashboard (Which all members have access to). It is unnecessary to log them out, they are members you know.
Unless you can show me how this is a security issue with some testing, I see it it as perfectly safe.
Mind you, I am not being a jerk in any way, I am only trying to understand your real concerns and addressing them with my points, which have been tested.
videsignz commentedon Aug 19, 2015
@panique
I also wanted to propose a separate method in the
Authclass. This function would check the user account type without all of the redirects and session destroys. It would be perfect for use in templates when you would want to show certain links or menus only available to certain members. I think it would be fitting to put this method within theAuthclass since its sole purpose is to authenticate a user account type.And within a template you could use it like....
[-][Improvement] A more flexible Auth:checkAuthentication with arguments[/-][+][LINKED][Improvement] A more flexible Auth:checkAuthentication with arguments[/+]panique commentedon Oct 11, 2015
Hey, I'll close this ticket but it's now linked from the readme inside the "future features" section, so people who are interesting in building a deeper role system will find this ticket.