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

Add Support for Persisting Audit Events #551

Merged
merged 14 commits into from Oct 7, 2018

Conversation

Projects
None yet
3 participants
@cfilby
Copy link
Contributor

cfilby commented Oct 3, 2018

Issue: PR for #522. Add basic event storage schema and functionality.

Cooper Filby
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 3, 2018

Codecov Report

Merging #551 into master will decrease coverage by 0.18%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
- Coverage   74.55%   74.37%   -0.18%     
==========================================
  Files          97       99       +2     
  Lines        6397     6439      +42     
==========================================
+ Hits         4769     4789      +20     
- Misses       1286     1306      +20     
- Partials      342      344       +2
Impacted Files Coverage Δ
app/models/infra.go 0% <ø> (ø) ⬆️
app/storage/inmemory/event.go 0% <0%> (ø)
app/storage/postgres/event.go 80% <80%> (ø)
app/pkg/web/util/webutil.go 96% <0%> (-0.15%) ⬇️
app/pkg/web/context.go 88.43% <0%> (-0.09%) ⬇️
main.go 0% <0%> (ø) ⬆️
app/pkg/email/smtp/smtp.go 16.15% <0%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7d5b93...d03d00f. Read the comment docs.

cfilby and others added some commits Oct 3, 2018

@cfilby cfilby changed the title [WIP] Add Support for Persisting Audit Events Add Support for Persisting Audit Events Oct 4, 2018

@cfilby

This comment has been minimized.

Copy link
Contributor

cfilby commented Oct 4, 2018

I believe this is ready for an initial review. This PR does the following:

  • Creates a new Events migration with the following fields: id, tenant_id, name, created_at.
  • Creates an Events interface with two methods, Add and GetByID (named to be consistent with the other Add storage interface methods).
  • Adds inmemory/postgres implementations for event storage with tests for the postgres implementation.

The two questions I had were:

  • Is it worth embedding the base interface? At the moment SetCurrentUser won't have any real effect. I ended up doing it since I figured it'd make things upstream easier if they were depending on the Base interface for common functionality.

  • Would you recommend creating a separate dbModel for Event? Or given the simplicity of the model is it fine to reuse it at both layers?

As an aside, is there a script somewhere to generate new empty migration files? I tried looking around a bit and just ended up generating the filename manually this time around.

Let me know if you see anything that I can change or improve!

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 5, 2018

Hey!

  1. The base interface is easy to implement and will fit better into current structure, so I’d recommend to implement it like you did.

  2. No need for the dbModel. My only suggestion would be to remove the TenantID from the model. Saving/querying the tenant_id should be done inside the storage struct (like you did), but we don’t need to expose that as a field.

  3. You forgot the ClientIP field 😀 You can get that from the web.Context struct. I’m now wondering if we should inject this as a dependency on the EventStorage constructor. If we do that, we can keep the Add method with a simple signature, just the name of the event as a parameter.

  4. We don’t have a migration generator, I usually just create a new file manually

Cooper Filby added some commits Oct 5, 2018

@cfilby

This comment has been minimized.

Copy link
Contributor

cfilby commented Oct 5, 2018

  1. Cool, that's what I was figuring.

  2. Fair enough, I went ahead and removed that from the Event model.

  3. Whoops! That's what I get for coding late at night - sorry about that. I've added the clientIP INET field to the existing migration. Right now this is just an argument to the new event function.

I think whether or not to inject the context depends on the overall lifecycle of the storage objects. If you'd expect to create new storage objects with each request or if you wanted cancellation, then it'd probably make sense. I haven't used with Contexts much yet, but to me passing the Context directly to the storage does expose it to a lot of high level implementation details, whereas just passing the clientIP keeps the storage interface simple and ignorant of the web layer above it. I'm good with either approach though, just let me know.

  1. Ah okay, just wanted to make sure I wasn't missing anything!

As an aside, it looks like the WrapRequest method manually extracts the IP from the forwarding header. The net package has a similar function that might be worth looking at depending on your needs.

Thanks!

Cooper Filby
@cfilby

This comment has been minimized.

Copy link
Contributor

cfilby commented Oct 5, 2018

Sorry, did another commit to try rerunning the CircleCI tests. It looks like it was failing due to timeout issues or something.

cfilby and others added some commits Oct 6, 2018

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 6, 2018

Are you talking about this line here?

strings.Split(request.RemoteAddr, ":")[0]

I never though about using SplitHostPort, but I guess it'd fit well. If you want to give it a try, just use a separate PR.

Now that we're talking about Client IP, I also noticed that if we can't parse the IP, we default it to "N/A". If this happens, we won't be able to insert into events table, because client_ip is inet type. This will most likely fail the request with a 500 response.

I think we have two options here:

  1. We remove the "N/A" default value and allow null into client_ip field. The Add function would then have to check whether client_ip is empty or not;

  2. Change client_ip to string;

Any thoughts?

@cfilby

This comment has been minimized.

Copy link
Contributor

cfilby commented Oct 6, 2018

Sure! I can go ahead and do that. Looking at the source code it seems like it may catch more edge cases (although it has the potential to return an error). Is the expectation that Fider will always run behind a reverse proxy? Right now it looks like the clientIP is pulled from the X-Forwarded-For header, so if you run it locally the client IP looks like it will always be null. While making that PR I could go ahead and have it fallback to the RemoteAddr property of the request if the X-Forwarded-For header is empty.

Do you mostly see the events table being used to prevent system abuse, or do you also envision it as a general purpose log? If you're trying to prevent abuse then you may want to leave the ClientIP mandatory and reject the request if the ClientIP can't be parsed. If we wanted to make ClientIP optional it seems like we'd want to keep INET field if possible for the stricter validation and drop the NOT NULL clause. My guess is that I could try to create a dbModel for events that uses a NullString as the datatype to make the IP optional seeing how we're currently just passing Postgres a string with the IP anyway.

I'm good with any of the approaches above though (Mandatory INET, Optional INET, Optional String), so let me know what you think and I'll go ahead and make those changes.

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 6, 2018

We already parse from RemoteAddr, see:

clientIP := request.Header.Get("X-Forwarded-For")
if clientIP == "" {
clientIP = strings.Split(request.RemoteAddr, ":")[0]
if clientIP == "" {
clientIP = "N/A"
}
} else {
clientIP = strings.Split(clientIP, ",")[0]
}

It'll be a general purpose event log, which can then be used for multiple ends. So let's make it an Optional INET. The goal is to always save the Client IP, but if for any reason it's not available, so be it, save as NULL but don't fail the request.

I also think we can remove the default "N/A" when ClientIP is empty (line 53 above). That doesn't add any value, just make everything harder.

@cfilby

This comment has been minimized.

Copy link
Contributor

cfilby commented Oct 7, 2018

Haha, whoops, definitely overlooked that one (embarrassingly, I added some debugging statements before the block with RemoteAddr but didn't look in the block XD).

The clientIP field should now be nullable and I've removed N/A as the default case.

One thing I noticed is that some of the other models are using NullableString for the retrieval, but not for the insert. I think you may want to use those for the insert as well if you want empty string values to be NULL in the database - my empty ClientIP string test failed on insert due to passing "" instead of NULL. That said, most of those fields that are marked as Nullable seem like they're validated by the application code to prevent empty strings and whatnot - just wanted to mention it in case there were some fields you wanted to be converted to null in the DB.

Let me know if there's anything else I can tweak! Once this is merged I'll go ahead and look into the SplitHostPort method PR.

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 7, 2018

When the database was created, I decided to use nullable varchar for certain fields in the database. But in 99% of the our use cases, a nil string and an empty string have the exact same meaning. I then realised that, because Go doesn't have nil strings, it's easier to treat the database fields just like any other Go string, without null values.

AFAIR, We use NullableString because some queries might not return all fields and dbx package doesn't convert null strings to empty strings.

@goenning goenning merged commit c7efc92 into getfider:master Oct 7, 2018

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: push Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test-server Your tests passed on CircleCI!
Details
ci/circleci: test-ui Your tests passed on CircleCI!
Details
@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 7, 2018

Thanks for the PR! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment