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

Login with invalid username or email displays 'Internal Server Error' message #1808

Closed
saralavanip opened this issue Oct 24, 2016 · 24 comments
Closed
Assignees
Labels

Comments

@saralavanip
Copy link
Contributor

saralavanip commented Oct 24, 2016

Steps:

  1. Visit https://apinf.io
  2. Click on 'Login'
  3. Enter non-registered username or email address
  4. Enter valid password
  5. Click on 'Sign In' button

Expected Result

Notify user with error message 'Invalid email address'

Actual Result

Displays 'Internal Server' error

Findings

  • This scenario was also tested on https://nightly.apinf.io and the result is: displays error message 'Login forbidden' (login unsuccessful )as expected.

Browser

  1. Mozilla Firefox 48.0
    (Mozilla Firefox for Ubuntu Canonical – 1.0 )
  2. Google Chrome 52.0.2743.116

Operating System

Ubuntu 16.0 LTS

Screenshot

selection_040
selection_041

@saralavanip saralavanip changed the title Login with invalid email id displays 'Internal Error' message Login with invalid email id displays 'Internal Server Error' message Oct 24, 2016
@bajiat
Copy link
Contributor

bajiat commented Oct 26, 2016

@brylie Can you check if anything has changed in validate (?) login code after release 0.33?

@brylie
Copy link
Contributor

brylie commented Oct 26, 2016

There is a login_verify function that runs on the server when submitting the form. As of today, the most recent change to that file was on October 13th.

It is possible that recent changes to that file, or some other user login related function/configuration has resolved this issue in nightly.

@bajiat bajiat added the backlog label Oct 26, 2016
@bajiat
Copy link
Contributor

bajiat commented Oct 28, 2016

@jykae Please check this one after apinf.io is redeployed and settings have been changed

@jykae
Copy link
Contributor

jykae commented Oct 31, 2016

@bajiat Still results "Internal server error" in apinf.io

nayttokuva 2016-10-31 kello 13 33 34

@brylie
Copy link
Contributor

brylie commented Oct 31, 2016

I think this is by design. If we submit an invalid email address, the security method may be designed to throw a 500 error to the client.

See

@jykae
Copy link
Contributor

jykae commented Oct 31, 2016

@brylie yep, error is ok, but we should find a way to catch error and show nicer error message for user :) And interesting part is why nightly gives "Login forbidden" that is a bit better message.

@brylie
Copy link
Contributor

brylie commented Oct 31, 2016

Also, the login form allows people to login with a username, so we can't always expect an email address.

@jykae
Copy link
Contributor

jykae commented Oct 31, 2016

@brylie Right, but it behaves same way if username does not exist on apinf.io

@brylie
Copy link
Contributor

brylie commented Oct 31, 2016

Where does the error originate? What method is checking that the user exists?

@brylie
Copy link
Contributor

brylie commented Oct 31, 2016

And what is the difference between our local envoronment, where we see 'Login forbidden' and the deployment, where we see 'Internal server error'?

@jykae
Copy link
Contributor

jykae commented Oct 31, 2016

@brylie Don't know, trying to look at it.. in progress

@brylie
Copy link
Contributor

brylie commented Oct 31, 2016

I am testing locally with meteor --production to see if there are any console errors.

@brylie brylie changed the title Login with invalid email id displays 'Internal Server Error' message Login with invalid username or email displays 'Internal Server Error' message Oct 31, 2016
@jykae
Copy link
Contributor

jykae commented Oct 31, 2016

This does not look too good, https://github.com/apinf/platform/blob/develop/users/server/accounts_hooks.js#L38
but it has worked somehow in the past :D

@brylie
Copy link
Contributor

brylie commented Oct 31, 2016

Hm. That seems to be left over from our boilerplate. Is it still necessary, or can we remove it?

@brylie
Copy link
Contributor

brylie commented Oct 31, 2016

Also, how can we access the server error log? It would probably be useful to get an error message.

@jykae
Copy link
Contributor

jykae commented Oct 31, 2016

@brylie I asked @shaliko to look server logs

@brylie
Copy link
Contributor

brylie commented Oct 31, 2016

Kool beans.

@jykae
Copy link
Contributor

jykae commented Oct 31, 2016

@brylie onLogin hook seems to be unnecessary, tested locally by commenting out, tried to register/login both with Github. It should wipe away undefined user._id error found in server logs.

@brylie
Copy link
Contributor

brylie commented Oct 31, 2016

Good work! Lets clean up that hook. 💣 💥 😎

@jykae jykae mentioned this issue Oct 31, 2016
@jykae
Copy link
Contributor

jykae commented Nov 2, 2016

nayttokuva 2016-11-02 kello 12 00 14

Reason for this according above apinf.io logs:
https://github.com/apinf/platform/blob/0.33.2/users/server/login_verify.js#L15

Current develop, we are checking that: (would explain difference between nightly vs. apinf.io)
https://github.com/apinf/platform/blob/develop/users/server/login_verify.js#L20

@jykae
Copy link
Contributor

jykae commented Nov 2, 2016

@bajiat @brylie maybe we can close this as solved?

@brylie
Copy link
Contributor

brylie commented Nov 2, 2016

@jykae we need to verify that it works properly in apinf.io.

@bajiat mentioned testing it on staging.apinf.io and then deploying the next release on apinf.io.

@jykae jykae added ready and removed in progress labels Nov 2, 2016
@jykae
Copy link
Contributor

jykae commented Nov 11, 2016

@bajiat @brylie Tested new release on staging. Verified to work as expected. Closing.
nayttokuva 2016-11-11 kello 13 03 43

@jykae jykae closed this as completed Nov 11, 2016
@jykae jykae removed the ready label Nov 11, 2016
@bajiat
Copy link
Contributor

bajiat commented Nov 11, 2016

Also apinf.io looks ok after the new release was deployed. Showing Login forbidden instead of Internal server error

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

No branches or pull requests

4 participants