Store IP address for every post

[tobyzerner]

This is pretty essential information that will be useful to many extensions.

• Create a migration to add ip_address column to posts table (use php flarum generate:migration)
• Set its value when a post is created (probably in CommentPost::reply())

[oldskool]

@tobscure What would be the cleanest way to obtain the user's IP address?

I would say to get it from the getServerParams() result of the Psr\Http\Message\ServerRequestInterface, but a quick search doesn't seem to reveal any classes implementing that interface? Any hints would be greatly appreciated 😄

[tobyzerner]

@oldskool Thanks for looking into this. You're correct that we should get it from ServerRequestInterface. I think we need to get the value in Flarum\Api\Controller\CreatePostController and pass it into the PostReply command in the data array. Then the PostReplyHandler can pass it into CommentPost::reply.

[franzliedke]

We're using the ServerRequest class from zend-diactoros as implementation of that interface. Doesn't matter, though... yes, you should be able to determine the IP address from the server params.

We need to decide whether it's worth supporting clients behind reverse proxies, like we did in FluxBB or like Symfony's HttpFoundation does it.

[oldskool]

This is what I currently have in mind: https://github.com/flarum/core/compare/master...oldskool:ip-logging?expand=1

Any comments/suggestions?

[tobyzerner]

Mostly looks good, thanks! Unfortunately you'll need to rebase on top of the master branch as there have recently been some significant changes to the Request/Action side of things. Specifically:

• Api/Actions/Posts/CreateAction is now Api/Controllers/CreatePostController
• Controllers now get passed an instance of Psr\Http\Message\ServerRequestInterface directly. Api/JsonApiRequest is dead.

Sorry about that!

[oldskool]

@tobscure No problem at all, I'm used to rebasing pretty much every other PR I make as I'm often working in active development environments 😉

[franzliedke]

The only thing I noticed: we probably don't want to throw an exception, but rather supply a default value that will be used if the server param does not exist.

[oldskool]

@franzliedke I specifically put in that exception because you're doing a specific call getServerParam. When you want to do something with that and it doesn't exist, it should throw an exception IMO. (Otherwise you always have to put if (!empty($request->getServerParam('x')) kind of checks in your code). In this case, when the REMOTE_ADDR server param does not exist, something very funky is going on and I would really want to know about that in the form of an exception and not just a null result. Another option would be to rename it to getServerParamOrNull or getServerParamIfExists or something like that, so the name implies that the result could be null.

Edit: Something else that comes to mind is a seperate "hasser" like hasServerParam, that way you can check that before doing something with it. (Although that kinda looks like the !empty constuction).

[franzliedke]

How about getServerParam($key, $default = null)? That way, you can call ->getServerParam('CLIENT_IP_or_whatever', '127.0.0.1').

Note, though, that the JsonApiAction class is gone, so you'll need to add that method somewhere else... I'm thinking about writing a helper class that makes things like these easier when working with PSR-7-style HTTP requests.

[oldskool]

@franzliedke Yeah, that could work. I was thinking about a Helper class yesterday as well, feels like this kind of action is going to be needed more often, so a Helper class seems useful. I'll look into this some more this weekend.

[franzliedke]

Let's build a package. PSR-7 stuff is super useful, but the APIs sometimes require more boilerplate than necessary...

[tobyzerner]

@franzliedke Sounds good, I'd love to contribute :)

@oldskool Would you be able to do the rebased PR this week? Wanna try and get beta 3 out the door. (In lieu of the PSR-7 helper package, using array_get($request->getServerParams(), 'REMOTE_HOST') in the controller is fine for now.)

[franzliedke]

Though don't forget the default parameter: array_get($request->getServerParams(), 'REMOTE_HOST', '127.0.0.1')...

[franzliedke]

@oldskool Will you have time to work on this in the next days? If not, no worries, but could we take your code to build upon it? :)

[oldskool]

@franzliedke I'm working on it right now. I hope to get the PR ready today or tomorrow.

[oldskool]

I'm stuck with testing this, I finally got my dev environment up and running again after merging all edits and running a db update (config -> settings), but now I have another issue with composer. See #605

Any simple way to fix this? I can't test my edits like this.

[oldskool]

I'm still having some problems with my dev environment, I think I have to start over from scratch with Vagrant. This is my latest WIP: https://github.com/flarum/core/compare/flarum:master...oldskool:ip-logging?expand=1

But I haven't been able to verify/test if this fully works because of the issues. I'll try to get them resolved ASAP, but feel free to continue on the above code changes if it needs to be implemented sooner.

[tobyzerner]

Merged! #615 / 73c44ad

Thanks very much @oldskool :)

[poVoq]

@askvortsov1 @luceos

This seems a bit pre-GDPR mind-set. Any chance to improve this to be optional or time-limited? IPs are personal data and it seems a bit excessive to store them with every post indefinitely in the database.

At least for our use, I would like to disable IP logging completely (outside of what the webserver does in the rotated logs).

[davwheat]

You're right that IPs are personal data, but it's a fairly common think to want to log.

You could set up a cron job to automatically delete IPs on posts older than a week, for example, or create an extension that strips IPs from the data being added to the database entirely.

[poVoq]

Yes I could probably do that, but first of all this seems like a work-around to something that isn't really needed in the first place (the GDPR is quite strict in what is needed functionally and what not) and secondly, even if we store the IP only for short time we would still need to ask for user consent regarding it in our privacy policy. At least that is my IANAL understanding as the personal data isn't only processed for technical reasons like in the case of short term web-server logging.

[katosdev]

There are a lot of contributing factors with regards to IP addresses and their personal identification of a user.
The EU courts ruled that IP addresses are personal information “in some cases” - IP addresses are usually dynamically assigned for home users and reused between customers and therefore do not directly identify a single user.

That said, we have spoken on a GDPR extension discussion with regards to anonymising an IP address by obfuscating the last few octets of the address.