Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process add'l data on registration #911

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

apps-caraga
Copy link
Contributor

Added procedure to process additional data sent during registration.
Other changes:

  • Minor edit to readme to improve readability of the Firebase setup procedure.
  • Additional properties for dbAuth for checking max/min length of username as well as allowable characters (thru regex pattern)

NB. Currently, the code throws an error on duplicate entry on columns that are intended to be unique.

Updated registration endpoint for dbAuth to accomodate other posted registration data. 
Also added some new dbAuth properties
1. usernameMinLength : specify minimum length of username (5)
2. usernameMaxLength : specify maximum length of username (40)
3. usernamePattern : specify regex pattern for usernames ('/^[A-Za-z0-9]+$/') // defaults to alphanumeric chars only
Formatted section on setting up JWT authentication to make the list more readable
Added the dbAuth properties for checking minimum and maximum length of username as well as allowed characters.
@mevdschee mevdschee self-requested a review September 17, 2022 06:19
@mevdschee mevdschee self-assigned this Sep 17, 2022
@mevdschee
Copy link
Owner

Is it ready to merge? Did you accidentally close it?

@mevdschee mevdschee reopened this Sep 17, 2022
@apps-caraga
Copy link
Contributor Author

Is it ready to merge? Did you accidentally close it?

It still causes fatal error on duplicate keys exception so I closed it to review.

Basically, since we are processing additional data, it is possible that the extra data are defined to be unique in the users table. A common example would be an email address. I think we can query the users table if an email address is already registered, but if there 2,3 or more unique fields in the table (not common but possible), we'll also need to query 2,3, or more times.

Since we're processing additional data during registration, we need to check if these data were defined in db to be unique. 
For example, email addresses are usually used just once in an application. We can query the database to check if the new email address is not yet registered, but, in some cases, we may more than 2 or 3 or more unique fields (not common, but possible), hence we would also need to query 2,3 or more times. 

As a TEMPORARY WORKAROUND, we'll just attempt to register the new user and wait for the db to throw a DUPLICATE KEY EXCEPTION.
@apps-caraga
Copy link
Contributor Author

@mevdschee, Good day! I've added try{}catch around the createSingle invocation when registring new user so I can suppress the PDOException on duplicate key It's not a very elegant solution but that's all I can think of for now.
I hope you can review and improve on it.

@chattago2002
Copy link

Are there any news about it? Can this improvement be merged in the main branch?

My problem is that I use "apiKeyDbAuth mode" so after the user sends username and password to "/register", every following request is blocked because of the "user api_key" is missing (I need to send many fields including "api_key"). This improvement can be useful to bypass the problem. Or are there other solutions for my problem?

@apps-caraga
Copy link
Contributor Author

𝙸 𝚞𝚜𝚎𝚍 𝚝𝚑𝚎 𝚖𝚘𝚍𝚒𝚏𝚒𝚎𝚍 𝚌𝚘𝚍𝚎 𝚎𝚟𝚎𝚗 𝚝𝚑𝚘𝚞𝚐𝚑 𝚒𝚝'𝚜 𝚗𝚘𝚝 𝚢𝚎𝚝 𝚖𝚎𝚛𝚐𝚎𝚍 𝚝𝚘 𝚖𝚊𝚒𝚗. 𝙰𝚗𝚢𝚠𝚊𝚢, 𝚒𝚏 𝚢𝚘𝚞 𝚍𝚘 𝚝𝚘𝚘, 𝚢𝚘𝚞 𝚗𝚎𝚎𝚍 𝚝𝚘 𝚔𝚎𝚎𝚙 𝚝𝚑𝚊𝚝 𝚒𝚗 𝚖𝚒𝚗𝚍 𝚒𝚗 𝚌𝚊𝚜𝚎 𝚢𝚘𝚞 𝚜𝚘𝚖𝚎𝚑𝚘𝚠 𝚞𝚙𝚍𝚊𝚝𝚎 𝚢𝚘𝚞𝚛 𝚊𝚙𝚙 𝚊𝚗𝚍 𝚝𝚑𝚎𝚗 𝚝𝚑𝚎 𝚖𝚘𝚍𝚒𝚏𝚒𝚎𝚍 𝚌𝚘𝚍𝚎 𝚐𝚎𝚝𝚜 𝚘𝚟𝚎𝚛𝚠𝚛𝚒𝚝𝚎𝚗.

𝙰𝚗𝚘𝚝𝚑𝚎𝚛 𝚠𝚊𝚢 𝚠𝚒𝚝𝚑𝚘𝚞𝚝 𝚞𝚜𝚒𝚗𝚐 𝚝𝚑𝚎 𝚖𝚘𝚍𝚒𝚏𝚒𝚎𝚍 𝚌𝚘𝚍𝚎 𝚒𝚜 𝚝𝚘 𝚜𝚝𝚘𝚛𝚎 𝚝𝚑𝚎 𝚘𝚝𝚑𝚎𝚛 𝚞𝚜𝚎𝚛 𝚍𝚎𝚝𝚊𝚒𝚕𝚜 𝚒𝚗 𝚊𝚗𝚘𝚝𝚑𝚎𝚛 𝚝𝚊𝚋𝚕𝚎 𝚊𝚗𝚍 𝚖𝚊𝚔𝚎 𝚒𝚝 𝚊 𝚖𝚞𝚕𝚝𝚒-𝚜𝚝𝚎𝚙 𝚙𝚛𝚘𝚌𝚎𝚜𝚜;

  • 𝚁𝚎𝚐𝚒𝚜𝚝𝚎𝚛 𝚓𝚞𝚜𝚝 𝚝𝚑𝚎 𝚞𝚜𝚎𝚛𝚗𝚊𝚖𝚎 & 𝚙𝚊𝚜𝚜𝚠𝚘𝚛𝚍,
  • 𝙶𝚎𝚝 𝚝𝚑𝚎 𝚘𝚝𝚑𝚎𝚛 𝚞𝚜𝚎𝚛 𝚏𝚒𝚎𝚕𝚍𝚜 𝚘𝚛 𝚒𝚗𝚏𝚘 𝚊𝚗𝚍 𝚜𝚊𝚟𝚎 𝚝𝚘 𝚊𝚗𝚘𝚝𝚑𝚎𝚛 𝚝𝚊𝚋𝚕𝚎, 𝚕𝚒𝚗𝚔𝚎𝚍 𝚝𝚘 𝚞𝚜𝚎𝚛 𝚝𝚊𝚋𝚕𝚎 𝚋𝚢 𝚏𝚘𝚛𝚎𝚒𝚐𝚗 𝚔𝚎𝚢.

𝙰𝚜 𝚏𝚘𝚛 𝚋𝚕𝚘𝚌𝚔𝚎𝚍 𝚛𝚎𝚚𝚞𝚎𝚜𝚝𝚜, 𝙸 𝚜𝚞𝚙𝚙𝚘𝚜𝚎𝚍 𝚒𝚝'𝚜 𝚌𝚊𝚞𝚜𝚎𝚍 𝚋𝚢 𝚝𝚑𝚎 𝚖𝚒𝚜𝚜𝚒𝚗𝚐 𝚑𝚎𝚊𝚍𝚎𝚛 (𝚡-𝚊𝚙𝚒-𝚔𝚎𝚢).
𝙸 𝚑𝚊𝚟𝚎𝚗'𝚝 𝚜𝚞𝚌𝚌𝚎𝚜𝚜𝚏𝚞𝚕𝚕𝚢 𝚝𝚛𝚒𝚎𝚍 𝚞𝚜𝚒𝚗𝚐 𝚝𝚑𝚎 𝚍𝚋𝙰𝚞𝚝𝚑 𝚊𝚗𝚍 𝚊𝚙𝚒𝙺𝚎𝚢𝙳𝚋𝙰𝚞𝚝𝚑 𝚝𝚘𝚐𝚎𝚝𝚑𝚎𝚛. 𝙼𝚊𝚢𝚋𝚎 @mevdschee 𝚌𝚊𝚗 𝚌𝚘𝚗𝚏𝚒𝚛𝚖 𝚒𝚏 𝚝𝚑𝚎𝚜𝚎 𝚝𝚠𝚘 𝚖𝚒𝚍𝚍𝚕𝚎𝚠𝚊𝚛𝚎𝚜 𝚌𝚊𝚗 𝚋𝚎 𝚞𝚜𝚎𝚍 𝚒𝚗 𝚝𝚑𝚎 𝚜𝚊𝚖𝚎 𝚏𝚒𝚕𝚎.

𝙰𝚜 𝚊 𝚠𝚘𝚛𝚔𝚊𝚛𝚘𝚞𝚗𝚍, 𝚢𝚘𝚞 𝚌𝚊𝚗 𝚜𝚎𝚝𝚞𝚙 𝚝𝚠𝚘 𝚟𝚎𝚛𝚜𝚒𝚘𝚗𝚜 𝚘𝚏 𝚝𝚑𝚎 𝚊𝚙𝚒.𝚙𝚑𝚙, 𝚘𝚗𝚎 𝚌𝚘𝚗𝚏𝚒𝚐𝚞𝚛𝚎𝚍 𝚠𝚒𝚝𝚑 𝚍𝚋𝙰𝚞𝚝𝚑 - 𝚝𝚑𝚒𝚜 𝚠𝚒𝚕𝚕 𝚑𝚊𝚗𝚍𝚕𝚎 𝚞𝚜𝚞𝚊𝚕 𝚛𝚎𝚐𝚒𝚜𝚝𝚛𝚊𝚝𝚒𝚘𝚗 𝚊𝚗𝚍 𝚝𝚑𝚎 𝚘𝚝𝚑𝚎𝚛 𝚒𝚜 𝚏𝚘𝚛 𝚊𝚙𝚒𝙺𝚎𝚢𝙳𝚋𝙰𝚞𝚝𝚑 𝚏𝚘𝚛 𝚞𝚜𝚎 𝚋𝚢 𝚢𝚘𝚞𝚛 𝚏𝚛𝚘𝚗𝚝𝚎𝚗𝚍

@mevdschee
Copy link
Owner

mevdschee commented Jan 22, 2023

𝙸 𝚑𝚊𝚟𝚎𝚗'𝚝 𝚜𝚞𝚌𝚌𝚎𝚜𝚜𝚏𝚞𝚕𝚕𝚢 𝚝𝚛𝚒𝚎𝚍 𝚞𝚜𝚒𝚗𝚐 𝚝𝚑𝚎 𝚍𝚋𝙰𝚞𝚝𝚑 𝚊𝚗𝚍 𝚊𝚙𝚒𝙺𝚎𝚢𝙳𝚋𝙰𝚞𝚝𝚑 𝚝𝚘𝚐𝚎𝚝𝚑𝚎𝚛. 𝙼𝚊𝚢𝚋𝚎 @mevdschee 𝚌𝚊𝚗 𝚌𝚘𝚗𝚏𝚒𝚛𝚖 𝚒𝚏 𝚝𝚑𝚎𝚜𝚎 𝚝𝚠𝚘 𝚖𝚒𝚍𝚍𝚕𝚎𝚠𝚊𝚛𝚎𝚜 𝚌𝚊𝚗 𝚋𝚎 𝚞𝚜𝚎𝚍 𝚒𝚗 𝚝𝚑𝚎 𝚜𝚊𝚖𝚎 𝚏𝚒𝚕𝚎.

You may get it to work, as I don't know of a reason it shouldn't work. Feel free to try it and open an issue if you get stuck.

𝙰𝚗𝚘𝚝𝚑𝚎𝚛 𝚠𝚊𝚢 𝚠𝚒𝚝𝚑𝚘𝚞𝚝 𝚞𝚜𝚒𝚗𝚐 𝚝𝚑𝚎 𝚖𝚘𝚍𝚒𝚏𝚒𝚎𝚍 𝚌𝚘𝚍𝚎 𝚒𝚜 𝚝𝚘 𝚜𝚝𝚘𝚛𝚎 𝚝𝚑𝚎 𝚘𝚝𝚑𝚎𝚛 𝚞𝚜𝚎𝚛 𝚍𝚎𝚝𝚊𝚒𝚕𝚜 𝚒𝚗 𝚊𝚗𝚘𝚝𝚑𝚎𝚛 𝚝𝚊𝚋𝚕𝚎 𝚊𝚗𝚍 𝚖𝚊𝚔𝚎 𝚒𝚝 𝚊 𝚖𝚞𝚕𝚝𝚒-𝚜𝚝𝚎𝚙 𝚙𝚛𝚘𝚌𝚎𝚜𝚜;

If you make the fields nullable and add values to them later in the multi-step process then you don't even need multiple tables.

Are there any news about it? Can this improvement be merged in the main branch?

I'll look into merging this code.

apps-caraga and others added 5 commits February 27, 2023 17:11
Changes
$_SESSION['user']['updatedAt'] - set to time when the session was created/updated
dbAuth.refreshSession - number of minutes after which the session data is refreshed when the /me end-point is called
Option to update session data via /me end-point
Copy link
Owner

@mevdschee mevdschee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the review notes.

@@ -71,6 +71,15 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
$usernameColumnName = $this->getProperty('usernameColumn', 'username');
$usernameColumn = $table->getColumn($usernameColumnName);
$passwordColumnName = $this->getProperty('passwordColumn', 'password');
$usernamePattern = $this->getProperty('usernamePattern','/^[A-Za-z0-9]+$/'); // specify regex pattern for username, defaults to alphanumeric characters
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this should default to non-white-space printable characters in any language (Chinese characters included).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be sufficient as the regex pattern? Haven't done much checking with other languages,but basically, we'll be checking if it's alphanumeric and also if it's a printable char in Unicode.

'/^[[:alnum:][:print:]]+$/u'

@@ -71,6 +71,15 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
$usernameColumnName = $this->getProperty('usernameColumn', 'username');
$usernameColumn = $table->getColumn($usernameColumnName);
$passwordColumnName = $this->getProperty('passwordColumn', 'password');
$usernamePattern = $this->getProperty('usernamePattern','/^[A-Za-z0-9]+$/'); // specify regex pattern for username, defaults to alphanumeric characters
$usernameMinLength = (int)$this->getProperty('usernameMinLength',5);
$usernameMaxLength = (int)$this->getProperty('usernameMaxLength',30);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel 30 is too low for an email address for instance. I would default to 255.

}else if($key === $passwordColumnName){
$data[$passwordColumnName] = password_hash($password, PASSWORD_DEFAULT);
}else{
$data[$key] = filter_var($value, FILTER_VALIDATE_EMAIL) ? $value : filter_var($value,FILTER_SANITIZE_ENCODED);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks internationalization (Chinese characters for instance).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure yet how to resolve this. Any issue if we simply just
$data[$key] = htmlspecialchars($value); ?

src/Tqdev/PhpCrudApi/Middleware/DbAuthMiddleware.php Outdated Show resolved Hide resolved
@apps-caraga
Copy link
Contributor Author

Here's the updated DbAuthMiddleware.

apps-caraga@c64d996

Changes
$usernamePattern - defaults  to  /^\p{L}+$/u , visible characters, no punctuation or numbers, unicode mode
$usernameMaxLength - defaults to 255
changed validation of other inputs from filter_validate()  to htmlspecialchars()
fixed typos missing and extra $
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants