Login Class

[geeks]

Hi guys,

 

My first real post here

 

I wrote this it is my first attempt at OO.

 

please be harsh.  I am not looking for pats on the back here

 

I know that sessions are not the best option and as the class grows I will move over to database session handling but for now it seems over my head !

 

thanks for any input good or bad 

 

please just be constructive (I want to learn not break down sorry )

 

Looking forward to your input

 

[attachment deleted by admin]

[TeddyKiller]

Depends.. what are you wanting us to do? Improve it? Give you tips? Fix it?

please be harsh

You don't want us to be 'harsh' on you. Seriously..

[ignace]

There is literally nothing good about this class, every class should have only one responsibility, your class has the following responsibilities:

 

1. Cookie/Session management

2. Db Management (incl. table creation)

3. Cryptography

4. Unique ID generation

5. Auth Management

 

Basically you wrote a script and wrapped a class around it. You have much to learn Kemo-Sabe

[geeks]

Depends.. what are you wanting us to do? Improve it? Give you tips? Fix it?

 

 

Thanks, just trying to teach myself OOP so any input,

 

is the script secure ?

is the OOP okay ? - Clearly Ignace does not think so.

 

here is literally nothing good about this class, every class should have only one responsibility

 

thank you for your very harsh reply, clearly tact is not your strong point but I'm a big boy I can handle it

 

I find this critique very confusing.

This class performs one larger function, to keep a site secure, to break it up into a million classes makes no sense

 

so I must have

• a class to create a table that is only ever used from the secure_this class
  
• a class to perform Cryptography that is only ever used from the secure_this class
  
• a class to generate a unique ID that is only ever used from the secure_this class
  

 

that seems to be very counter productive. All of these are part of a single function that is being performed these are all functions that I would never use anywhere else

 

[ignace]

clearly tact is not your strong point but I'm a big boy I can handle it

 

Tact is for wimps who don't have the balls to say how it is. Your client won't use tact when you messed up or when he decides to let another programmer maintain your code, and he tells him that because of your negligence the design is not flexible enough to make the necessary changes and shall require a serious amount of refactoring (money) to implement the new features.

 

to break it up into a million classes makes no sense  ... that seems to be very counter productive

 

There's no need for exaggeration here, besides what made you suddenly an expert? To you it may seem counter-productive, to us it looks like a design that is flexible and allows change.

 

To submerge yourself in OOP you should learn more then just how to use a class, you should learn about principles, patterns, and practices. While you model an application you should be able to recognize patterns and adhere to principles (SOLID) & best practices.

[Mchl]

I'll agree with ignace here. Too much stuff put into one class.

For user authentication, it would be better to have (for example):

User class - which handles retrieving and storing user data (possibly using yet another dbConnection class)

AuthenticationManager class - a singleton that handles logging in and logging out of a User and maybe access control (though this is usually handled by yet another class)

 

There's no one best way to do it, but clearly you went in a direction of packing as mach as possible into a single class, while the genral practice is to keep classes 'slim'.

 

Take it easy... I've been there too. I'm still rewriting some of the classes I created

[TeddyKiller]

Did you write the script? It seems like your providing it for other users to use.

[geeks]

Thanks to all replies,

 

 

Ignace - thank you for the time you have taken and the tactless    to the point approach (I actually prefer it).

 

 

I will re-look at the design I have used and break it up a little more.

 

 

Yes I did write the class, I can only hope that when it it complete others would actually want to use it 

[ignace]

I am not someone who just gives critic but shows no examples (fear of being criticized himself), so here goes in how I would model it:

 

class Session {
  public function regenerateId();
  public function rememberUntil($until = 3600);
  public function destroy();
}

class Auth {
  private $session;
  private $database;
  private $logonObserver;
  
  public function logon($username, $password, $rememberMe = 3600) {}
  public function logout() {}
  public function hasAuth() {}
  
  //1) add IP blocking for too many bad attempts
  public function addListener(SplSubject $l) {}
}

// 4) XSS security checks
class FilterUsername {}
class FilterPassword {}

// 2) use database for session handeling
class SessionDao {
  public function findById($id) {}
}

class UserDao {
  public function findByCredentials($username, $password) {}
}

 

This design allows for everything you wished to accomplish (and more) Using the Observer pattern you can add functionality that checks if a spoofed IP was used, too many attempts have been made, .. The DAO objects separate your database from your application logic. Specialized classes can be used to filter username and password throughout the entire project. The Session class separates the actual $_SESSION from your application logic so that it can vary. Believe it or not you can skip $_SESSION altogether.

 

Of course I just wrote this out of the top of my head and I'm sure this design has some flaws but in overall this provides for better flexibility/maintainability as even you have to admit this is much clearer (easier to understand) then what you wrote?

[ignace]

I can only hope that when it it complete others would actually want to use it 

 

Too many programmers start out by writing code for other programmers to use, and I wonder why? Do you honestly believe millions of people will start using your script once you publish it? That you will become a rock-star amongst the countless number of programmers? That people start worshiping you while you achieved/learned yet nothing? What good is fame if your word is no good? There are rock-stars on these forums but you and I are not one of them (yet).

 

Ask yourself: do you use code from an unknown/unreliable source? Most likely not, as you don't want to be bothered to be debugging something you didn't even write or for that matter understand (and in debugging you want to exclude as many reliable sources as possible in order to pinpoint the root-cause). Don't understand me wrong I highly encourage you to post your code on-line in the form of documentation and refer to it from within your scripts, anyone who will ever have to maintain your work will be grateful, sadly they don't teach these great de facto rules in school.

 

In the first place you should write code for yourself and in the best way possible (maintainable and highly flexible) as you will spend quite some time weeding through it. So, it's beneficial you make this maintenance a breeze. It also has other benefits like easier/low-cost adjustments.

[geeks]

Ignace -

 

okay okay take it easy and read -

 

I can only hope [/color]that when it it complete others would actually want to use it

 

  - I am not writing it for others, the notes are there for myself. but hey should it ever get there I certainly would not stop others from using it.

 

  - I just added the version history etc for the heck of it.

 

I have totally rewritten my class. ( I understand what you mean - implementing it is something else though )

 

please let me know if the structure is better and if the code is okay.

 

[attachment deleted by admin]

[ignace]

1) You don't need unset() in your class method, local variables are cleaned once it leaves the method scope

2) CREATE TABLE queries should only be made during installation, nowhere else

3) Installation is not a responsibility of UserData

4) Favor composition over inheritance

 

class User/* extends Data*/ {
  private $data;
  public function __construct(Data $d) {
    $this->data = $d;
  }
}

 

5) Separation of Concerns UserData should not be responsible for query handling

 

Your design is an overall improvement but still miles away from being flexible.

[ignace]

<?php

class Database {
    public function __construct($name, $password, $username = 'root', $host = 'localhost') {}
    public function __destruct() {$this->disconnect();}
    public function connect() {/*lazy-load*/}
    public function disconnect() {}
    public function select($table, $fields, $where = '') {}
    public function insert($table, $data) {}
    public function update($table, $data, $where = '') {}
    public function delete($table, $where) {}
    public function fetchAll($mode = MYSQL_ASSOC) {}
    public function fetchOne($mode = MYSQL_ASSOC) {}
}

class UserDao {
    public function __construct(Database $db) {}
    public function findByCredentials($username, $password) {/*return new User();*/}
    public function findByUsername($username) {}
    public function findById($id) {}
}

class User {
    private $id, $username, $password;
}

class Auth implements SplSubject {
    private $authObservers;
    
    public function notify() {/*foreach ($this->authObservers as $o)
        if (false === $o->update($this)) return $this->abort();*/}
    public function detach(SplObserver $observer) {}
    public function attach(SplObserver $observer) {}
    
    public function logon($username, $password, $rememberMe = 3600) {/*$username = new FilterUsername($username);*/}
}

class CleanIp implements SplObserver {
    public function update(SplSubject $subject) {}
}

[geeks]

how is this now ?

 

 

thanks for your input so far

 

[attachment deleted by admin]

[TeddyKiller]

It's better, and its hell of alot better than what I can do.

[ignace]

It's overall better but you should stick more to what I gave you especially the DAO objects, use a CRC (Class Responsibility-Collaboration Card) if you have trouble figuring out what a class responsibility is and what isn't.