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

Log successful and failed login attempts #2347

Merged
merged 1 commit into from
Jun 23, 2016
Merged

Log successful and failed login attempts #2347

merged 1 commit into from
Jun 23, 2016

Conversation

fabaff
Copy link
Member

@fabaff fabaff commented Jun 21, 2016

Description:
This is basically the same feature which was introduced with #1558. Instead of only showing:

16-06-21 21:31:35 INFO (WSGI-server) [homeassistant.components.http] 127.0.0.1 - - [21/Jun/2016 21:31:35] "GET /api/bootstrap HTTP/1.1" 200 14754 0.001991

and

16-06-21 21:51:35 INFO (WSGI-server) [homeassistant.components.http] 127.0.0.1 - - [21/Jun/2016 21:51:35] "GET /api/bootstrap HTTP/1.1" 401 186 0.001866

There will be a human-readable log entry with the source additionally to the 200/401 HTTP message.

16-06-21 21:31:35 INFO (WSGI-server) [homeassistant.components.http] Successful login from 127.0.0.1

or

16-06-21 21:51:35 WARNING (WSGI-server) [homeassistant.components.http] Login attempt with an invalid password from 127.0.0.1

Example entry for configuration.yaml (if applicable):

http:

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

authenticated = True

elif hmac.compare_digest(request.args.get(DATA_API_PASSWORD, ''),
self.hass.wsgi.api_password):
_LOGGER.info('Successful login from %s', request.remote_addr)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of repeating the same log message three times with the risk of them getting out of sync, let's add it before the if self.requires_auth line:

if authenticated:
    _LOGGER.info(…)
elif self.requires_auth:
    _LOGGER.warning(…)
    raise Unauthorized()

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks

@balloob
Copy link
Member

balloob commented Jun 22, 2016

Looks good. Can be merged when linting error addressed 🐬

@fabaff fabaff merged commit 3349bdc into home-assistant:dev Jun 23, 2016
@fabaff fabaff deleted the http-login branch June 23, 2016 10:34
@balloob
Copy link
Member

balloob commented Jun 29, 2016

I only realize now that this adds a lot of logging. I wonder if we should reduce the visibility of the logging? Although, that might also not be useful. I don't know.

@philhawthorne
Copy link
Contributor

I wonder if it would be better to add a config option so this could be turned off?

@fabaff
Copy link
Member Author

fabaff commented Jun 29, 2016

Perhaps we should get rid for the additional log entries for 200 responses. One would still be able to spot easily failed login attempts and the entry for 200 responses only need a little more parsing if those details matters.

@kellerza
Copy link
Member

Another option would be to default the http component logging to a higher level, like warning. To be honest I keep mine at critical

@philhawthorne to turn off/down you can try this

logger:
  logs:
    homeassistant.components.http: critical

@balloob
Copy link
Member

balloob commented Jun 29, 2016

The logging trick works but we shouldn't be broken (or spamming) by
default.

On Wed, Jun 29, 2016, 07:41 Johann Kellerman [email protected]
wrote:

Another option would be to default the http component logging to a higher
level, like warning. To be honest I keep mine at critical

@philhawthorne https://github.com/philhawthorne to turn off/down you
can try this

logger:
logs:
homeassistant.components.http: critical


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#2347 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABYJ2vGwhW8xy_DW4RJuNt4CGHh3hhccks5qQoQigaJpZM4I7HkD
.

@balloob
Copy link
Member

balloob commented Jun 29, 2016

New CherryPy wsgi server doesn't print any url so we could print the url
retrieved and include if we authenticated successfully

On Wed, Jun 29, 2016, 09:07 Paulus Schoutsen [email protected]
wrote:

The logging trick works but we shouldn't be broken (or spamming) by
default.

On Wed, Jun 29, 2016, 07:41 Johann Kellerman [email protected]
wrote:

Another option would be to default the http component logging to a higher
level, like warning. To be honest I keep mine at critical

@philhawthorne https://github.com/philhawthorne to turn off/down you
can try this

logger:
logs:
homeassistant.components.http: critical


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#2347 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABYJ2vGwhW8xy_DW4RJuNt4CGHh3hhccks5qQoQigaJpZM4I7HkD
.

@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants