Authentication Library

[abiusx]

I see a lot of flaws here. First, you have two or three very long classes in one file. That is not right.

Second, every static function in BasicPasswordManagement is public. Refer to the convention for access.

Third, User extends BasicPasswordManager. This is essentially wrong. A user does not have a salt. A user might have a password, and that password might have salts and all.

1. You should not pass around db connection. You should handle them somewhere.
2. Why do you get password as a param into newUserObject and existingUserObject
3. There are no optional fields in User object. if someone needs them, they can extend from our User class.
4. It still has a lot of setter/getters. Rid them, this is not Java.
5. namings require much more effort. For example, if you wanted to know if one user's password is expired or not, you would never look for checkIfPasswordExpired. You might probably go for isExpired or isPasswordExpired or passwordExpiry, but not check...
6. I believe you need a user class, and a user management class. You can also put those management functions (such as edit, create, delete) in the same User class, but as static methods.
7. If its authentication library folder, you can move advanced password management here as well.

[abiusx]

I have no idea why github re-orders my numbers. It started from 3 up to 10, but shows as 1 to 7.

[rash805115]

2) If you are creating a new user then the new user would set his password. For existing users, they need to give their passwords to access their accounts.

3) Should I also remove the columns such as name and email from the DB. If they need them, they will create these columns in the extended class.

4) In advanced password management and in user class, both the places I have removed set/get....where have you found those?

6) Isn't user management RBAC that you have created and we will just implement that ?

[rash805115]

1) I have no idea what to do with this point. Lets say I do not pass DB object. Then for SQL commands I need to make an object inside the library itself.

What are my other options...please help me understand this issue better.

[abiusx]

Use static methods from DatabaseManager to retrive a connection.

[abiusx]

You should remove name and email. If you're using email for your advanced functionality, introduce it there.

[abiusx]

2) Users do not use our framework. Developers do. Developers have access to database, they don't need user passwords, and don't have them.

4) These three getUserID, getHashedPassword, getDynamiSalt

6) RBAC handles authorization. It has nothing to do with authentication. It doesn't even know usernames.

[rash805115]

1) Hello. For point 1, if I use DatabaseManager inside the library to create connection to DB, then in each library I would have to specify DBusername, DBpassword etc. Is that ok?

[abiusx]

you should only use database manager to retrive active connection, and not even the connection, just use the default connection's SQL function via database manager.

The test code (or the actual usage code) will create the connection and set one as default.

[rash805115]

ok...so I make a connection in the test files.

But in the library, I use the DatabaseManager::connect(DatabaseConfig Object) to bring and use this connection.

So instead of passing the actual connection, I have to pass the DatabaseConfig Object. Am I correct ?

On Thu, Jul 4, 2013 at 2:52 PM, AbiusX notifications@github.com wrote:

you should only use database manager to retrive active connection, and not even the connection, just use the default connection's SQL function via database manager.

The test code (or the actual usage code) will create the connection and set one as default.

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/13#issuecomment-20489792 .

Regards, Rahul Chaudhary Ph - 412-519-9634

[abiusx]

Nop, You use DatabaseManager::defaultConnection()->SQL(...) or simply create an alias for that in DatabaseManager, and do DatabaseManager::SQL(...), or even simpler create a function in phpsec namespace SQL, which does that with DatabaseManager, and you do SQL(...)

[rash805115]

lets discuss 2 and 4:

In 2: To get the hash of a string, I need the string. This is what I am using the password for. The password isn't getting store anywhere. I am not sure what you mean by taking password. The user will enter password in their end, the password reaches server, gets hashed and then stored. But to calculate hash, I do need this string. Is there any other methods in use that I am not aware of ?

In 4: If I remove the get functions, I would have to make userID, hashedPassword and dynamicSalt public. Is that ok ?

[abiusx]

1. So you basically need the actual password only for CREATE NEW USER and VALIDATE CREDENTIALS. Nothing else.
2. if they need to be public, it is. if not, you're using them somewhere wrong.

[rash805115]

1) yes...when user creates a new account, they send their password, it gets hashed and stored. If a returning user comes, he gives password, it gets hashed, then checked with the old hash, then allowed access.

2) Well they can be public. I was just worried that the hash seems like sensitive data and so does dynamicSalt. Why take chances by making them public.

[abiusx]

1. A library is not a use-case scenario. You should implement developer use-cases, not user use-cases.
2. public, protected, private has nothing to do with security.

[rash805115]

Abbas, I am really sorry but this might irritate you...I have still not understood some of the enhancements you suggested in this library.

Lets go through them one by one again.

First I want to discuss this one: You said I should create a user management class that will contain add, delete etc functionality. But what is the need. Lets say I create a "addUser" function.So this function will also create a new user (the same function is already provided in the user class). Same goes for delete and all other functions. Why will the developers need a user management. One case would if if they want to define roles and stuff to the user. But for this, the functions would be entirely different. It would be like createRole, and addUserToRole etc. etc. Is that what you were referring to ?

[abiusx]

I'm not irritated, I'm starting to have fun :D

Have this example: what if you moved the createUser function into the DatabaseManager class. It defeinitely does something with the database, and developers could use that easily. So why not do it?

---

Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Tir 14, 1392, at 8:32 AM, rash805115 notifications@github.com wrote:

Abbas, I am really sorry but this might irritate you...I have still not understood some of the enhancements you suggested in this library.

Lets go through them one by one again.

First I want to discuss this one: You said I should create a user management class that will contain add, delete etc functionality. But what is the need. Lets say I create a "addUser" function.So this function will also create a new user (the same function is already provided in the user class). Same goes for delete and all other functions. Why will the developers need a user management. One case would if if they want to define roles and stuff to the user. But for this, the functions would be entirely different. It would be like createRole, and addUserToRole etc. etc. Is that what you were referring to ?

— Reply to this email directly or view it on GitHub.

[rash805115]

so you want me to create some functions such as create, delete etc and put it inside userManagement Class and put that class with databaseManager class so that developers can use them easily. What if they are only interested in databaseManager or userManagement ? Wouldn't then these functions would be redundant ?

Also there are several other classes such as session and adv. password mgt that does changes to DB. Should I also move common functions from these classes to the databaseManager class ?

On Fri, Jul 5, 2013 at 5:07 AM, AbiusX notifications@github.com wrote:

I'm not irritated, I'm starting to have fun :D

Have this example: what if you moved the createUser function into the DatabaseManager class. It defeinitely does something with the database, and developers could use that easily. So why not do it?

---

Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Tir 14, 1392, at 8:32 AM, rash805115 notifications@github.com wrote:

Abbas, I am really sorry but this might irritate you...I have still not understood some of the enhancements you suggested in this library.

Lets go through them one by one again.

First I want to discuss this one: You said I should create a user management class that will contain add, delete etc functionality. But what is the need. Lets say I create a "addUser" function.So this function will also create a new user (the same function is already provided in the user class). Same goes for delete and all other functions. Why will the developers need a user management. One case would if if they want to define roles and stuff to the user. But for this, the functions would be entirely different. It would be like createRole, and addUserToRole etc. etc. Is that what you were referring to ?

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/13#issuecomment-20508019 .

Regards, Rahul Chaudhary Ph - 412-519-9634

[abiusx]

I was talking about a scenario which works, btu is a very bad idea! I didn't want you to do it ;D

---

Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Tir 14, 1392, at 10:09 PM, rash805115 notifications@github.com wrote:

so you want me to create some functions such as create, delete etc and put it inside userManagement Class and put that class with databaseManager class so that developers can use them easily. What if they are only interested in databaseManager or userManagement ? Wouldn't then these functions would be redundant ?

Also there are several other classes such as session and adv. password mgt that does changes to DB. Should I also move common functions from these classes to the databaseManager class ?

On Fri, Jul 5, 2013 at 5:07 AM, AbiusX notifications@github.com wrote:

I'm not irritated, I'm starting to have fun :D

Have this example: what if you moved the createUser function into the DatabaseManager class. It defeinitely does something with the database, and developers could use that easily. So why not do it?

---

Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Tir 14, 1392, at 8:32 AM, rash805115 notifications@github.com wrote:

Abbas, I am really sorry but this might irritate you...I have still not understood some of the enhancements you suggested in this library.

Lets go through them one by one again.

First I want to discuss this one: You said I should create a user management class that will contain add, delete etc functionality. But what is the need. Lets say I create a "addUser" function.So this function will also create a new user (the same function is already provided in the user class). Same goes for delete and all other functions. Why will the developers need a user management. One case would if if they want to define roles and stuff to the user. But for this, the functions would be entirely different. It would be like createRole, and addUserToRole etc. etc. Is that what you were referring to ?

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/13#issuecomment-20508019 .

Regards, Rahul Chaudhary Ph - 412-519-9634 — Reply to this email directly or view it on GitHub.

[MysterAitch]

With respect to the getters/setters, I suggest that if a value should be publicly get-able but _not_ publicly set-able then I suggest a getter is necessary. This becomes more important when writing library code for other developers - if it _can_ be misused by being directly accessible, then it _will_ be misused.

As a specific example, code elsewhere may wish to know the user's username but there is no valid reason to edit the user's username. In the case that this were wanted, creating a new User() object with the new username would be warranted.

Further to this, I cannot see any reason why a password hash or dynamic salt would be needed elsewhere - either you are giving the password/user manager a hash to compare to the stored one (in which case the stored one doesn't need to be "seen" publicly) or you are creating a new password.

In both of these cases the data is being given _to_ the password/user manager that has private access to the variable, thus the variable does not need to be publicly accessible and no getter is necessary.

If the current architecture does not support this, then it suggests refactoring is necessary (logically, should a User extend a BasicPasswordManagment? should AdvancedPasswordManagement extend BasicPasswordManagement? maybe User() should contain only the core implementation and an ApplicationSpecificUser have an ApplicationSpecificUser::passwordManager?).

[MysterAitch]

Sidenote: @abiusx , I am guilty of this by suggesting a refactor in the previous post, but it is getting rather confusing to keep track of which problems have been solved, which ones remain and which ones are being discussed. Perhaps create issues in seperate threads, link to them in the top post here and then strikethrough once the linked issue is closed?

[abiusx]

Hello, Both points are perfectly valid and I have pointed them out many times, but the team is just getting up. We would appreciate you helping with those. Thanks

---

Notice: This message is digitally signed, its source and integrity are verifiable. If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Tir 15, 1392, at 2:37 AM, howelrtc notifications@github.com wrote:

Sidenote: @abiusx , I am guilty of this by suggesting a refactor in the previous post, but it is getting rather confusing to keep track of which problems have been solved, which ones remain and which ones are being discussed. Perhaps create issues in seperate threads, link to them in the top post here and then strikethrough once the linked issue is closed?

— Reply to this email directly or view it on GitHub.

[MysterAitch]

I am more than happy to help out :) I shall have something to share tomorrow afternoon.

[MysterAitch]

What is the best way of sharing?

I have committed work-in-progress inside my fork at: https://github.com/howelrtc/phpsec/commit/15db553d26f9bdbf0dd14552520ae98a9190516d

[abiusx]

feel free to commit to the main branch, the project is not released yet. you can also use the mailing list.