Failed authentication are now properly logged #2334

daviian · 2017-08-18T17:50:59Z

Targets #2301

Failed authentication now returns 401 status code.
Done by introducing a new RenderFormWithErr function in context to allow passing of a statuscode

Tested in following browsers:

Latest version of FF, Chrome and Safari Mac
IE 11
Chrome for Android Latest
Microsoft Edge 12

lafriks · 2017-08-20T01:12:11Z

LGTM

lunny · 2017-08-20T03:07:31Z

It seems CI status is disappeared?

lafriks · 2017-08-20T06:45:16Z

@lunny yes it was there until recent force push when he fixed my suggested in discord changes :) Don't know how Drone status got lost

daviian · 2017-08-20T19:27:08Z

integrations/signin_test.go

+		{username: "user1@example.com", password: "wrongPassword", errorMessage: i18n.Tr("en", "form.username_password_incorrect")},
+	}
+
+	prepareTestEnv(t)


Shouldn't this be in the testLoginFailed function because each call represents a seperate test case!?

if test-case would change data that need to be reset than yes, otherwise it will only make tests run slower.

ethantkoenig

I'm not sure returning a 401 on failed login is the correct thing to do; see https://stackoverflow.com/questions/8273757/should-a-failed-login-attempt-result-in-a-http-401-response

I've investigated several public-facing sites (Google, Amazon, Instagram, Github, Gitlab), and none of them returned 401 on failed login.

ethantkoenig · 2017-08-20T19:47:46Z

models/fixtures/user.yml

@@ -218,3 +218,33 @@
  avatar_email: user13@example.com
  num_repos: 3
  is_active: true
+


Is there a reason we can't just use existing user fixtures? user16 isn't used anywhere, and it seems we could use any existing user in place of user15 in TestSignin.

Didn't want to intervene in other test cases because of duplicate email address.
But if that's not the case of course I can remove user16.

Oh, I missed that user15 and user16 had the same email addresses; sorry about that.

Instead of introducing two new fixtures for testing the duplicate email case, could we instead do something like:

func TestDuplicateEmailFailedLogin(t *testing.T) { prepareTestEnv(t) user := models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) // add new user with user2's email user.Name = "testuser" user.LowerName = strings.ToLower(user.Name) user.ID = 0 // will be auto-populated on insert models.AssertSuccessfulInsert(t, user) // TODO try to login with user.Email, should fail with duplicate email }

It's not really scalable for us to add new test fixtures for every corner case we need to test.

If others feel the same way I will change that ;-)

ethantkoenig · 2017-08-20T19:48:53Z

modules/context/context.go

-	ctx.Data["Flash"] = ctx.Flash
-	ctx.HTML(200, tpl)
+	ctx.Flash.Error(msg, true)
+	ctx.HTML(status, tpl)


Why did you remove the ctx.Data["Flash"] = ctx.Flash line?

@ethantkoenig ctx.Flash.Error(...) sets ctx.Data["Flash"]

@lafriks @ethantkoenig Only if second parameter is true

daviian · 2017-08-20T20:11:55Z

The main reason for 401 status code was to enable easier fail2ban configuration.
An alternative would be to only log the failed authentication attempt in an easy to distinguish way into the gitea log if that pleases you more ;-)

lafriks · 2017-08-20T20:27:43Z

I'm now thinking that entry to gitea log would be better than changing response status code

ethantkoenig · 2017-08-20T20:29:11Z

@daviian @lafriks I would prefer a log message as well

daviian · 2017-08-20T20:31:05Z

@lafriks @ethantkoenig Then it's settled. I will change PR.

daviian · 2017-08-22T21:20:45Z

@lafriks @ethantkoenig Which log level would you say is appropriate? Or is there a way to log independent from the set log level?

ethantkoenig · 2017-08-22T22:57:28Z

@daviian I would say "Info", because I think it makes sense to reserve "Warning" and "Error" for actionable entries

lafriks · 2017-08-23T05:49:33Z

@ethantkoenig but than fail2ban can be only used by enabling only at least info log level that is not best option. And in case of fail2ban usage failed authorization is actionable entry

daviian · 2017-08-23T06:04:54Z

@ethantkoenig @lafriks I was also struggling with this, but I think users who want to use fail2ban have to set the necessary log level either way. And I agree that Info is the best choice.

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>

lafriks · 2017-08-23T06:17:53Z

Why did you remove integration tests? These were useful anyway

daviian · 2017-08-23T06:20:04Z

I thought that they are not part of the PR if it is only logging. But I can add them again.

lafriks · 2017-08-23T06:24:47Z

You could at least submit them in other PR then, we need tests :)

daviian · 2017-08-23T06:55:17Z

And regarding the log level. I am open for discussion ;-) It's not cast in stone yet ;-)

lafriks · 2017-08-23T07:20:14Z

@ethantkoenig need your approval

bkcsoft · 2017-08-24T03:31:22Z

Changed the title to reflect the change 🙂

ethantkoenig

LGTM

ethantkoenig · 2017-08-24T05:26:11Z

Let's try again LGTM

daviian force-pushed the auth-failed-statuscode branch from 24bcd80 to 1d6f917 Compare August 18, 2017 17:54

lunny added the type/enhancement An improvement of existing functionality label Aug 19, 2017

lunny added this to the 1.x.x milestone Aug 19, 2017

daviian force-pushed the auth-failed-statuscode branch 4 times, most recently from 53a657d to 6852aab Compare August 20, 2017 00:59

tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Aug 20, 2017

daviian commented Aug 20, 2017

View reviewed changes

ethantkoenig suggested changes Aug 20, 2017

View reviewed changes

Log failed authentication attempts with remote address for fail2ban

fc83835

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>

daviian force-pushed the auth-failed-statuscode branch from 95c7640 to fc83835 Compare August 23, 2017 06:14

bkcsoft changed the title ~~Failed authentication now returns 401 status code~~ Failed authentication are now properly logged Aug 24, 2017

ethantkoenig approved these changes Aug 24, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 24, 2017

lafriks merged commit 174255e into go-gitea:master Aug 24, 2017

lafriks modified the milestones: 1.2.0, 1.x.x, 1.3.0 Aug 24, 2017

daviian mentioned this pull request Aug 24, 2017

HTTP 401 on failed login #2301

Closed

7 tasks

daviian deleted the auth-failed-statuscode branch August 24, 2017 09:42

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Failed authentication are now properly logged #2334

Failed authentication are now properly logged #2334

daviian commented Aug 18, 2017

lafriks commented Aug 20, 2017

lunny commented Aug 20, 2017

lafriks commented Aug 20, 2017

daviian Aug 20, 2017

lafriks Aug 20, 2017

ethantkoenig left a comment •

edited

Loading

ethantkoenig Aug 20, 2017

daviian Aug 20, 2017 •

edited

Loading

ethantkoenig Aug 20, 2017

daviian Aug 20, 2017

ethantkoenig Aug 20, 2017

lafriks Aug 20, 2017

daviian Aug 20, 2017

daviian commented Aug 20, 2017 •

edited

Loading

lafriks commented Aug 20, 2017

ethantkoenig commented Aug 20, 2017

daviian commented Aug 20, 2017

daviian commented Aug 22, 2017

ethantkoenig commented Aug 22, 2017

lafriks commented Aug 23, 2017

daviian commented Aug 23, 2017 •

edited

Loading

lafriks commented Aug 23, 2017 •

edited

Loading

daviian commented Aug 23, 2017

lafriks commented Aug 23, 2017

daviian commented Aug 23, 2017

lafriks commented Aug 23, 2017

bkcsoft commented Aug 24, 2017

ethantkoenig left a comment

ethantkoenig commented Aug 24, 2017

Failed authentication are now properly logged #2334

Failed authentication are now properly logged #2334

Conversation

daviian commented Aug 18, 2017

lafriks commented Aug 20, 2017

lunny commented Aug 20, 2017

lafriks commented Aug 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviian Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviian commented Aug 20, 2017 • edited Loading

lafriks commented Aug 20, 2017

ethantkoenig commented Aug 20, 2017

daviian commented Aug 20, 2017

daviian commented Aug 22, 2017

ethantkoenig commented Aug 22, 2017

lafriks commented Aug 23, 2017

daviian commented Aug 23, 2017 • edited Loading

lafriks commented Aug 23, 2017 • edited Loading

daviian commented Aug 23, 2017

lafriks commented Aug 23, 2017

daviian commented Aug 23, 2017

lafriks commented Aug 23, 2017

bkcsoft commented Aug 24, 2017

ethantkoenig left a comment

Choose a reason for hiding this comment

ethantkoenig commented Aug 24, 2017

ethantkoenig left a comment •

edited

Loading

daviian Aug 20, 2017 •

edited

Loading

daviian commented Aug 20, 2017 •

edited

Loading

daviian commented Aug 23, 2017 •

edited

Loading

lafriks commented Aug 23, 2017 •

edited

Loading