[WIP] Webapp feature for public data export #11996

whoodes · 2019-03-27T00:45:57Z

What is this PR for

Fixes: #11930, which adds a public only realm export feature for the webapp.

Demo of tentative UI:

Commits

Previous commits

(merged): This commit serves as the first step in supporting "public export" as a
webapp feature. The refactoring was done as a means to allow calling
the export logic from elsewhere in the codebase.
(merged): This allows for removal of the local tarball upon a successful s3 export.
(merged): An endpoint was created in zerver/views. Basic rate-limiting was
implemented using RealmAuditLog. The event is then published to the deferred_work queue processor to prevent the export process from being killed after 60s. Finally, upon completion of the export the realm admin(s) are notified via send_event.
(Plus a handful of clean up commits).

Complete (re-posting of Tim's comment below):

We need to add LOCAL_UPLOADS_DIR support to the --upload-to-s3 option. First, a commit should rename it to --upload since it's no longer going to be just S3. And then a commit that changes export_realm_wrapper to support upload via the LOCAL_UPLOADS_DIR backend (likely, this will be just a shutil.copyfile call with the right construction), and then we can remove the AssertionError and fill out the test for the LOCAL_UPLOADS_DIR backend.
There are several TODOs in test_realm_export.py that need to be completed.
We probably should arrange to store the ID of the RealmAuditLog row created in public_only_realm_export in the event sent to the deferred_work queue, and then have the queue processor add the public_url to the RealmAuditLog event in the extra_data field. This will enable support for deleting the uploaded exports later once they've been accessed.

zerver/views/public_export.py

hackerkid · 2019-03-27T19:25:19Z

@whoodes I went through the code quickly. I have commented how to use queue system. Let me know in CZO if there is something that you did't understand. Also posted a bunch of minor comments. We need to add tests for this as well once you are done so that we know this is working correctly as expected.

zerver/lib/export_realm.py

timabbott · 2019-04-05T22:04:07Z

@whoodes thanks for working on this! I have a few proposed changes:

Let's move the function you put in zerver/lib/export_realm.py in the existing zerver/lib/export.py. And I think the name isn't ideal; how about export_realm_wrapper ? It's a bit tricky to name this given the existing do_export_realm, but I think that's clear enough.
With that change, I think the first two commits are safe to merge.
For the third commit, add @require_realm_admin on the view function, and test that it's only usable by organization administrators.
I'll have more suggestions, but this is a good start.

zerver/views/public_export.py

zerver/models.py

zerver/worker/queue_processors.py

timabbott · 2019-04-05T22:08:13Z

Testing for the admin notification event.

This should be tested in test_events.py similar to our other tests, though it'll be a simple one (since the event is a no-op).

whoodes · 2019-04-07T10:40:25Z

Thanks for reviewing @timabbott, I addressed the current batch of comments. I made some additional updates based upon working on the frontend piece of this as well. A couple questions:

Do we want to send a json_error if the s3 backend isn't configured?
I changed the admins to be notified via a PM from notification bot, curious as to your thoughts on this approach (or how you would prefer notifying admins).

Looking forward to it.

whoodes · 2019-04-07T18:07:23Z

(Just rebased from master.)

timabbott · 2019-04-12T17:56:55Z

I merged the first couple commits, since they looked complete. Can you post the precise threading error traceback you got? I'll need that to help figure out what's going on.

Do we want to send a json_error if the s3 backend isn't configured?

json_error is for situations which are a user error (HTTP code 4xx), not for system configuration problems (5xx). I think what we need to do instead is extend the upload logic to support using the "local file uploads" backend as well (where we effectively just copy the file into an appropriate path under LOCAL_UPLOADS_DIR).

zerver/tests/test_realm_export.py

zerver/views/public_export.py

zproject/urls.py

timabbott · 2019-08-10T00:58:20Z

zerver/lib/export.py

@@ -1709,7 +1709,7 @@ def get_realm_exports_serialized(user: UserProfile) -> List[Dict[str, Any]]:
    for export in all_exports:
        exports_dict[export.id] = dict(
            id=export.id,
-            event_time=export.event_time.ctime(),
+            export_time=ujson.dumps(export.event_time),


I don't understand -- why do we need JSON to send a timestamp?

I was looking at this incorrectly. I realize now that using dumps was just a weird way of converting the datetime object to a timestamp. Fixed.

timabbott · 2019-08-10T00:58:57Z

zerver/lib/actions.py

+    # However, mypy throws: "AnyStr" of "loads" cannot be "Optional[str]`
+    # if we don't check the None case.
+    export_extra_data = export.extra_data
+    if export_extra_data is not None:


I would do assert export_extra_data is not None then, not add a useless and confusing if statement.

Ahh of course, that should have been obvious. Fixed.

timabbott · 2019-08-10T00:59:56Z

zerver/tests/test_openapi.py

@@ -223,6 +223,7 @@ class OpenAPIArgumentsTest(ZulipTestCase):
        # test because of the variable name mismatch. So really, it's more of a buggy endpoint.
        '/user_groups/{group_id}',  # Equivalent of what's in urls.py
        '/user_groups/{user_group_id}',  # What's in the OpenAPI docs
+        '/export/realm/{export_id}',


Move this earlier., it's not related to the use_groups block that has comments.

In particular, should be next to /export/realm.

Woops, silly mistake. Fixed.

timabbott · 2019-08-10T01:00:52Z

zerver/views/realm_export.py

+                                           realm=user.realm,
+                                           event_type="realm_exported")
+    except RealmAuditLog.DoesNotExist:
+        return json_error(_("The export doesn't exist"))


"Invalid data export ID" would be a more standard way to write this.

timabbott · 2019-08-10T01:01:28Z

zerver/views/realm_export.py

+        return json_error(_("The export doesn't exist"))
+
+    if ujson.loads(export.extra_data).get('deleted_timestamp') is not None:
+        return json_error(_("Export already deleted"))


Split the JSON load on a separate line, and do an if 'deleted_timestamp' in export_data.

timabbott · 2019-08-10T01:01:38Z

zerver/views/realm_export.py

+@require_realm_admin
+def delete_realm_export(request: HttpRequest, user: UserProfile, export_id: int) -> HttpResponse:
+    try:
+        export = RealmAuditLog.objects.get(id=export_id,


Call this local variable audit_log_entry.

Done, also updated the tests to use this naming.

The time of the event was incorrectly being sent as a datetime object.

We don't want to add a `deleted_timestamp` key until the export is actually deleted.

Leftover from prior debugging.

* Added comments. * Refactored `export_object` to `audit_log_entry`. * Cleaned up white space. * Removed uses of `getattr`.

This commit serves as the frontend piece for the "public export" webapp feature. Fixes: zulip#11930

whoodes · 2019-08-12T18:26:03Z

@timabbott I addressed the batch of comments. I also split up some clean up commits that should be individually mergeable. Namely:

Fixing the timestamp thing.
Changing the extra_data key in export_dict to have the more consistent, and less ambiguous name export_data.
A general clean up of test_realm_export

I also removed deleted_timestamp from being added in queue_processors. Instead the key, value pair is only added upon deletion.

timabbott · 2019-08-13T01:23:37Z

I merged all the backend commits. I then added 8ac8247 and the commit before it to make sure we don't accidentally trigger a DoS (limits are probably conservative) before this has gotten more heavy use. @rishig you should look at the error message I set for "organization too large"; I think it should be good.

Thanks for making this happen @whoodes! @rishig we should now be able to update the documentation for data exports to say one can do "public export" self-service. Not sure if we also need something on /features or what.

I'll open a few technical follow-up issues.

timabbott · 2019-08-13T01:34:12Z

#12992 is the main follow-up issue for this. We also have some technical infrastructure work to do.

andersk · 2019-08-13T01:37:50Z

Also your limit commit failed CI.

whoodes · 2019-08-13T02:21:13Z

Added tests for that in #12993

zulipbot added the size: XL label Mar 27, 2019

hackerkid suggested changes Mar 27, 2019

View reviewed changes

hackerkid reviewed Mar 27, 2019

View reviewed changes

zerver/lib/export_realm.py Outdated Show resolved Hide resolved

whoodes force-pushed the webapp-public-data-export branch 5 times, most recently from 02d9cdc to 578a625 Compare April 2, 2019 00:48

timabbott reviewed Apr 5, 2019

View reviewed changes

zerver/views/public_export.py Outdated Show resolved Hide resolved

timabbott reviewed Apr 5, 2019

View reviewed changes

zerver/models.py Outdated Show resolved Hide resolved

timabbott reviewed Apr 5, 2019

View reviewed changes

zerver/worker/queue_processors.py Outdated Show resolved Hide resolved

timabbott reviewed Apr 5, 2019

View reviewed changes

zerver/worker/queue_processors.py Outdated Show resolved Hide resolved

whoodes force-pushed the webapp-public-data-export branch from 578a625 to bd1ae6e Compare April 7, 2019 10:04

whoodes force-pushed the webapp-public-data-export branch from bd1ae6e to 17064cf Compare April 7, 2019 18:06

whoodes force-pushed the webapp-public-data-export branch from 17064cf to b923294 Compare April 10, 2019 08:39

This comment has been minimized.

Sign in to view

whoodes force-pushed the webapp-public-data-export branch 2 times, most recently from e04c01c to 430bfcc Compare April 12, 2019 10:08