Skip to content

Sign Up User, Username and Password Can Be Empty #3606

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

Closed
trylovetom opened this issue Mar 7, 2017 · 8 comments
Closed

Sign Up User, Username and Password Can Be Empty #3606

trylovetom opened this issue Mar 7, 2017 · 8 comments

Comments

@trylovetom
Copy link
Contributor

We use GitHub Issues for bugs.

If you have a non-bug question, ask on Stack Overflow or Server Fault:

You may also search through existing issues before opening a new one: https://github.com/ParsePlatform/Parse-Server/issues?utf8=%E2%9C%93&q=is%3Aissue

--- Please use this template. If you don't use this template, your issue may be closed without comment. ---

Issue Description

When attempting to signup a user with empty username and password, I think that can get error which is username and password are required. But it still created user and session.

Steps to reproduce

Post /parse/users
{ "username": "", "password": "" }

Expected Results

like this
{ "code": 201, "error": "username and password is required." }
or
{ "code": 201, "error": "username is required." }
or
{ "code": 201, "error": "password is required." }

Actual Outcome

{
"objectId": "xehXxh1MDt",
"createdAt": "2017-03-07T14:43:02.468Z",
"username": "QTt1bmB1ojpw7krJ7SaK8rBkP",
"sessionToken": "r:41e117452a9279cbddb70d434264aaac"
}

Environment Setup

  • Server

    • parse-server version (Be specific! Don't say 'latest'.) : 2.3.6
    • Operating System: OSX
    • Hardware: MacBook Pro (Retina, 13-inch, Early 2015)
    • Localhost or remote server? Localhost
  • Database

    • MongoDB version: v3.4.0
    • Storage engine:
    • Hardware: Same as above
    • Localhost or remote server? Localhost

Logs/Trace

Include all relevant logs. You can turn on additional logging by configuring VERBOSE=1 in your environment.

@imwiss
Copy link
Contributor

imwiss commented Mar 17, 2017

I think this is a valid point and that it should prevent us from creating users with empty passwords and / or usernames.

I'm currently taking a look at this and attempting my first PR on Parse. I can reproduce the bug using the REST API, and I found where the changes need to be done.

@imwiss
Copy link
Contributor

imwiss commented Mar 17, 2017

Actually, after further investigation, I feel like this was the intended behaviour.

For starters, to avoid users passing an empty password, you should set the passwordPolicy in the config. This will make sure all passwords respect the rules you defined.

Secondly, I see in the code that the username is set to a random secure string if left blank. Based on that, I am assuming that this use case was accounted for and is working as-designed.

  if (!this.data.username) {
    if (!this.query) {
      this.data.username = cryptoUtils.randomString(25);
      this.responseShouldHaveUsername = true;
    }
    return Promise.resolve();
  }

I'll wait for confirmation on this from the maintainers before proceeding any further.

@flovilmart
Copy link
Contributor

The randomly generated username is intended IIRC for non password based logins (i.e.: facebook, oauth...)

It seems that we don't make the distinction between a missing key and an invalid key.

Not sure we should let empty usernames/passwords in, if only this is passed along, however we still need the automatically generated username for the case discussed above.

@imwiss
Copy link
Contributor

imwiss commented Mar 17, 2017

Great, thanks for the update. I don't think we should let empty usernames/passwords either. This use case is handled when using the JS SDK, but not the REST API. I'll try to base myself off of what was done in the JS SDK. I'll keep you posted.

Merci @flovilmart

@imwiss
Copy link
Contributor

imwiss commented Mar 17, 2017

Just created a PR for this: #3650

@flovilmart
Copy link
Contributor

And just merged! Thanks for the PR!

@imwiss
Copy link
Contributor

imwiss commented Mar 18, 2017

Awesome! Thanks for the quick merge!

@trylovetom
Copy link
Contributor Author

Thank A Lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants