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

fix the login log entry #36443

Merged
merged 1 commit into from
Feb 15, 2023
Merged

fix the login log entry #36443

merged 1 commit into from
Feb 15, 2023

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Jan 30, 2023

fix #23063

@szaimen szaimen added bug 2. developing Work in progress labels Jan 30, 2023
@szaimen szaimen added this to the Nextcloud 26 milestone Jan 30, 2023
@blizzz blizzz mentioned this pull request Feb 1, 2023
@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 11, 2023
@szaimen szaimen marked this pull request as ready for review February 11, 2023 13:24
@szaimen szaimen requested review from ChristophWurst, a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team February 11, 2023 13:24
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

👍

@blizzz
Copy link
Member

blizzz commented Feb 13, 2023

Just to be on the safe side: this does not conflict with any passwordless login things?

@szaimen
Copy link
Contributor Author

szaimen commented Feb 13, 2023

Just to be on the safe side: this does not conflict with any passwordless login things?

Why should it? Isnt that using a different endpoint? And if so ir should have failed before?

Also since @ChristophWurst already approved I think it should be fine.

@szaimen szaimen requested review from come-nc and kesselb February 15, 2023 12:44
@kesselb
Copy link
Contributor

kesselb commented Feb 15, 2023

The fix looks wrong to me.

{
   "method":"POST",
   "url":"/login",
   "message":"Argument 1 passed to OC\\Core\\Controller\\LoginController::tryLogin() must be of the type string, null given, called in /var/www/html/lib/private/AppFramework/Http/Dispatcher.php on line 217",
}

The error is triggered by post request but the form data / inputs for username and password are missing.
A default value for username and password will not address the actual issue.

I believe the actual issue is something like #36286.

@szaimen
Copy link
Contributor Author

szaimen commented Feb 15, 2023

@kesselb I rather think this is someone trying to log in without providing a username. However I don't think that this should be logged as error. So changing the level of the error would be fine by me as well but is this even possible technically?

@kesselb
Copy link
Contributor

kesselb commented Feb 15, 2023

Username and Password are required fields. You cannot submit the login form without them.

This patch is a workaround to hide another error 🙈

@szaimen
Copy link
Contributor Author

szaimen commented Feb 15, 2023

This patch is a workaround to hide another error 🙈

Since you seem to be sure about this I'm going to close this attempt.

@szaimen szaimen closed this Feb 15, 2023
@szaimen szaimen deleted the fix/23063/fix-login-log-entry branch February 15, 2023 16:24
@szaimen
Copy link
Contributor Author

szaimen commented Feb 15, 2023

Too bad. I hoped to get rid of these logs that spam the logs on my test instance:
image

@szaimen
Copy link
Contributor Author

szaimen commented Feb 15, 2023

I mean this was fine to log as loglevel warning or info but it seems rather overkill as error...

@kesselb
Copy link
Contributor

kesselb commented Feb 15, 2023

Mind to have a look at the webserver's access log? Maybe it tells us more about the origin of the request.

@szaimen
Copy link
Contributor Author

szaimen commented Feb 15, 2023

The webservers access log is not kept on my test server so I cannot tell you anything except it seems to be always the same external ip-address (not mine) and a Windows device likely with chrome browser: remoteAddr":"137.184.182.xxx", "user":"--","app":"index","method":"POST","url":"/login","message":"OC\\Core\\Controller\\LoginController::tryLogin(): Argument #1 ($user) must be of type string, null given, called in /var/www/html/lib/private/AppFramework/Http/Dispatcher.php on line 225 in file '/var/www/html/core/Controller/LoginController.php' line 298","userAgent":"Mozilla/5.0 (Windows NT 6.4; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2225.0 Safari/537.36"

@kesselb kesselb restored the fix/23063/fix-login-log-entry branch February 15, 2023 16:51
@kesselb kesselb reopened this Feb 15, 2023
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

👍

@szaimen
Copy link
Contributor Author

szaimen commented Feb 15, 2023

Thanks Daniel!

@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 15, 2023
@szaimen szaimen merged commit a747be3 into master Feb 15, 2023
@szaimen szaimen deleted the fix/23063/fix-login-log-entry branch February 15, 2023 17:14
@szaimen
Copy link
Contributor Author

szaimen commented Feb 15, 2023

Backport to stable25 here: #36672

@blizzz blizzz mentioned this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
4 participants