Welcome Guest, Not a member yet? Register   Sign In
Avoid using other libraries in own library
#1

(This post was last modified: 08-03-2016, 11:58 AM by Call-Me-Captain.)

Heya guys!

I have another (probably stupid - I apologize in advance) question.

After getting a lot of advice in one of my previous threads, I figured that I'd like to move most of my controllers' functionality into libraries.
Also, in order to keep those libraries as independent as possible, I am trying to avoid using other libraries in my own.

To be even more specific: Right now, I'm implementing a basic user system as a library. For the reason above and also because I would like to keep my login (And other functionality) functional even if no HTML form is provided, I do not want to use libraries such as CI's form validator in my library's code.
Basically, the code in the library should only give basic functionality that runs depending on a certain input. The input shouldn't be read-out by itself, but rather by the code in the controllers, or some other place.

I hope I'm making sense, but if I'm not, here's the code I currently have - Followed by a small description of my actual problem.
(I hope that I created this thread in the right sub-forum, but it looks like a "Best practice" problem to me, since I could easily solve it by removing the barriers I put up myself - Obviously, I do not want to do that, though.)

In my library:
PHP Code:
To clarify"cUser" is a CI Model used to perform queries to the database.
CI is an instance of CodeIgniter assigned to a class variable by $this->CI = &get_instance();
/**
 * Log-in an user with his given credentials. Logged-in user's ID will be stored in his session.
 * $username - Username of the user.
 * $password - Not yet hashed password of the user.
 * Return - array ( "result" => true/false, "message" => "Specify the result" )
**/
function login($username$password){
    
//Check if the user is already logged in. If he is, return success message.
    
if( isset($this->CI->session->auth_uid) ) return array("result" => true"message" => "Already logged in.");
    
    
//Check if the username and password match in the database. Returns -1 on failure or userID on success.
    
$qr $this->CI->cUser->matchUserPass($usernameMD5($password));
    
    
//Failure? Return the information.
    
if($qr == -1) return array("result" => false"message" => "Username or Password invalid.");
    
    
//Success? Set session userID. Then, return success.
    
else $this->CI->session->auth_uid $qr;
    return array(
"result" => true"message" => "Successfully logged in.");


In my controller (Old function, not adapted to the use of my library. My following question will be about how to best adapt it.):
PHP Code:
function login($lastURL "/user/"){ //set default to view user (todo)
    
if( isset($this->session->auth_uid) ){
        
$this->session->set_flashdata('auth_msg''Already logged in.'); //todo languages
        
redirect($lastURL);
    }
    
$this->load->library('form_validation');
    
$this->form_validation->set_rules("username""Username""required");//todo add more rules
    
$this->form_validation->set_rules("password""Password""required");
    if(
$this->form_validation->run() == FALSE){
        
$this->load->view("Login");
    }else{ 
//todo dont send pass in plain text (hash on client side or https)
        
$qr $this->user->matchUserPass($this->input->post('username'), MD5($this->input->post('password')));
        if(
$qr == -1){
            
$this->session->auth_msg "Username or Password invalid.";
            
$this->session->mark_as_flash('auth_msg');
            
$this->load->view("Login");
        }else{
            
$this->session->set_flashdata('auth_msg''Successfully logged in.'); //todo languages
            
$this->session->auth_uid $qr;
            
redirect($lastURL);
        }
    }

Last code before moving on to my question. Here's approximatively how I'd like my new login function to look:
PHP Code:
//My library is loaded in the contructor or something.

function login($lastURL "/user/"){ //set defaul to view user //todo:

    
$this->load->library('form_validation');
    
$this->form_validation->set_rules("username""Username""required");
    
$this->form_validation->set_rules("password""Password""required");
    if(
$this->form_validation->run() == FALSE){
        
$this->load->view("Login");
    }else{ 
//todo dont send pass in plain text
        
$result $this->usersystem->login($this->input->post("username"), $this->input->post("password");
 
               $this->session->set_flashdata('auth_msg'$result['message']);
        if(
$result['result']) redirect($lastURL);
 
               else $this->load->view("Login");
    }

There are multiple problems with that code, so I obviously can't leave it like that.
1) I can't check if the user is already logged in. Because if he is, the login page will get displayed anyway. Running my library's function before the form validation would also be bullshit though, since I don't have anything I can pass as argument.
2) Even though I wanted to be library independent, I'm still using the session library in my own library. This is a minor problem, but if someone has an idea on how to beautifully avoid this, I'd be glad to hear it!
3) Nah, actually the main problem is the first. I do not want to call my library's function twice, but I don't see another solution that wouldn't completely butcher the code.
Does anyone have an idea Big Grin?
Thanks!
Reply
#2

Trying to combine validating a login form - And - returning a user session - both in the same method - is messy. "A method should do one thing and do it well." (Robert Martin) Codeigniter form validation is awesome because of all the different things you can check. But if all you are checking for is if there is a value in the form field then you could easily do without it. Something to consider - instead of just checking whether a user is logged in, you can also return a $this->user object to then be available in your application. So then instead of something asking 'is this user logged in?' - it can try to get the user object, and if its not returned, redirect to the login page.
Reply
#3

Thanks for your answer!

(08-03-2016, 05:17 PM)cartalot Wrote: Trying to combine validating a login form - And - returning a user session - both in the same method - is messy.
That's why I have one method setting the user session (In my library), and another one used to validate forms (Controller).

Quote:Codeigniter form validation is awesome because of all the different things you can check. But if all you are checking for is if there is a value in the form field then you could easily do without it.
I know, but I will add rules in the future. Plus, form validator has a really nice error system. So I'd like, or even need, to keep using it. Only in my controller though, and not in my library.

Quote: Something to consider - instead of just checking whether a user is logged in, you can also return a $this->user object to then be available in your application.
Well, the thing is that the library, used for authentication, is globally available in every controller.


Quote:So then instead of something asking 'is this user logged in?' - it can try to get the user object, and if its not returned, redirect to the login page.
The method isn't really about checking if the user is logged in, but about logging in the user. Checking if he already is logged in is just a "security" procedure.
And with your method, the same problem as I have with my code persists: I need to check if the user is logged in before validating the form, but that would mean that I'd have to call my library's function twice. Which is ugly...
Reply
#4

Your login page does not need to check if the user is logged in or not. You do not need to check if the user is logged in before logging them in. Finally, not using CI libraries and helpers within your libraries makes no sense to me, and multiple calls to the the same library function is perfectly common and not ugly at all.
Reply
#5

(This post was last modified: 08-04-2016, 05:43 AM by Call-Me-Captain.)

Thanks for your answer!

(08-04-2016, 05:20 AM)PaulD Wrote: Your login page does not need to check if the user is logged in or not. You do not need to check if the user is logged in before logging them in.
Well, yes, I kind of do, since I do not want logged in users to be able to access the login form. So if I do not check if they are logged in, and they visit the login form page by URL, the only way I see to not show them the form is to check if they are already logged in.

Quote:Finally, not using CI libraries and helpers within your libraries makes no sense to me, and multiple calls to the the same library function is perfectly common and not ugly at all.
Hm, well, I guess I'll just do that then.
Reply
#6

I think, the authentication sub-system alone (let us forget about the access to the logging form for now) should check whether a user tries to log-in again. In this case I would log-out the old user and let the new user log-in. The corresponding events might be triggered properly, and they would be recorded within a log (if there is such) in the right sequence.
Reply
#7

I agree with ivantcholakov.

What if your user might have two accounts, and might be switching between them. What if you, as admin, want to test permissions for users and want different accounts for testing.
Reply
#8

First, since no-one has mentioned it yet, read the PHP manual's entry on safe password hashing: http://php.net/manual/en/faq.passwords.php

As far as an authentication library goes, at a minimum you'll need something like isLoggedIn(), login(), and logout().

In general, I would use a single controller to manage most of the authentication-related functionality, such as login, logout, and registration. Then I would probably add a simple (protected) method to MY_Controller which I could call from my other controllers when a user is required to login. This would call the authentication library's isLoggedIn() method, then redirect to the login form if the user is not logged in.

If you really want to use as little of the CI libraries as possible in your authentication library, your best bet would be to start by isolating the calls to CI's libraries in your library, usually by creating protected/private methods in your library which interface with the CI library. Once you've done that, you can extract those methods into an adapter class, which you can inject into your authentication library's constructor as a value in the parameter array (the optional second argument to $this->load->library()). So, you would do something like this when loading your authentication library:

PHP Code:
$this->load->library('authentication_ci_adapter');
$authParams = [
    
'adapter' => $this->authentication_ci_adapter,
];
$this->load->library('authentication'$authParams); 
Reply
#9

Thanks again for all of your answers! I really appreciate it.

(08-04-2016, 05:53 AM)ivantcholakov Wrote: I think, the authentication sub-system alone (let us forget about the access to the logging form for now) should check whether a user tries to log-in again. In this case I would log-out the old user and let the new user log-in. The corresponding events might be triggered properly, and they would be recorded within a log (if there is such) in the right sequence.

(08-04-2016, 06:06 AM)PaulD Wrote: I agree with ivantcholakov.

What if your user might have two accounts, and might be switching between them. What if you, as admin, want to test permissions for users and want different accounts for testing.

While this may be considered useful in some cases, I do not want such a feature in my app - Not because I want to enforce a 1-account-per-person rule, but because it would simply make no sense. Also, I think that it would be much clearer for everyone - Okay, mostly myself - if an user had to properly log out before being able to log in again. But, @ivantcholakov, such an even system still is an epic idea! I totally need to implement that.

Right now, my (working) solution is simply to call my login function twice, once with NULL arguments, and once with the proper arguments. I did change it a little bit so it now looks like this:

PHP Code:
/**
 * Log-in an user with his given credentials. If logged in, sets session userID to the one of the user.
 * Argument: $username - Username of the user.
 * Argument: $password - Not yet hashed password of the user.
 * Argument: $message - Passed by reference, used to give back a message to the caller.
 * Return: Success boolean.
**/
function login($username$password, &$message){        
    
//Check if the user is already logged in. If he is, return success message.
    
if( $this->loggedIn() ){
        
$message 'Already logged in.';
        return 
true;
    }
        
    
//Make sure that the arguments are not NULL
    
if( $username == NULL || $password == NULL){
        
$message 'There was an internal error.';
        return 
false;
    }
        
    
//Check if the username and password match in the database. Returns -1 on failure or userID on success.
    
$qr $this->CI->cUser->matchUserPass($usernameMD5($password));
        
    
//Failure? Return the information.
    
if($qr == -1){
        
$message 'Username or Password invalid.';
        return 
false;
    }
        
    
//Success? Set session userID. Then, return success.
    
$this->CI->session->auth_uid $qr;
    
$this->initUser();
    
    
$message 'Successfully logged in.';
    return 
true;

I'm honestly not sure anymore how many good practices I've completely butchered with that or not. Any opinions Big Grin?


(08-04-2016, 06:38 AM)mwhitney Wrote: First, since no-one has mentioned it yet, read the PHP manual's entry on safe password hashing: http://php.net/manual/en/faq.passwords.php
Right, I actually read about the same problem once, but was too lazy to implement it yet. Maybe I should do that, lol.


Quote:As far as an authentication library goes, at a minimum you'll need something like isLoggedIn(), login(), and logout().
Yup, I have that right now.


Quote:In general, I would use a single controller to manage most of the authentication-related functionality, such as login, logout, and registration. Then I would probably add a simple (protected) method to MY_Controller which I could call from my other controllers when a user is required to login. This would call the authentication library's isLoggedIn() method, then redirect to the login form if the user is not logged in.
As of now, I made it so that every controller automatically includes my user system library, meaning that the isLoggedIn() function can be called from everywhere.


Quote:If you really want to use as little of the CI libraries as possible in your authentication library, your best bet would be to start by isolating the calls to CI's libraries in your library, usually by creating protected/private methods in your library which interface with the CI library. Once you've done that, you can extract those methods into an adapter class, which you can inject into your authentication library's constructor as a value in the parameter array (the optional second argument to $this->load->library()). So, you would do something like this when loading your authentication library:

PHP Code:
$this->load->library('authentication_ci_adapter');
$authParams = [
    
'adapter' => $this->authentication_ci_adapter,
];
$this->load->library('authentication'$authParams); 

Oh well, that also is a way to do it I guess Big Grin!



Thanks again Smilea
Reply




Theme © iAndrew 2016 - Forum software by © MyBB