Opened 7 years ago

Closed 7 years ago

#927 closed defect (fixed)

Clean up Federation

Reported by: Jessica Tallon Owned by:
Priority: critical Milestone: 0.7.0
Component: programming Keywords:
Cc: Parent Tickets:

Description (last modified by Elrond)

Elrond found some issues over his review of the federation code recently merged. These need fixing before 0.7.0.

Garbage Collection

  • This task should be moved to submit
  • This task uses the wrong operator in garbage collection (greater than rather than less then)
  • The test sets entry.uploaded not entry.created ergo doesn't do anything.
  • Logging is wrong.
  • Use spaces not tabs! (WTF jess).

Feed Endpoint

  • The comment is wrong (should say list all media not the users media).

Whoami Endpoint

  • Doesn't handle the case where I might not be logged in

MediaComment.unserialize

  • Needs to do more checking that all the data I want to look up in the dictionary exists to avoid KeyErrors.

Mediagoblin INI/Config file

  • Should remove garbage_collection setting to spec file and remove it from mediagoblin.ini as it won't be commonly set.

json_error

  • On the idea of:
    def json_error(error_str, status=400):
        return json_response({"error": error_str}, status=status)
    
  • I am not sure, where to put this, either in the general response place, where json_response is also, or maybe only where it's being used in the federation code. If I have to vote, I'd likely vote for the former.

media_type/image: api_* upload utils

  • Those staticmethod functions are really a specialized variant of submit/lib.py:submit_media().
  • As only the pump.io/federation code uses it, please move this into federation/lib.py
  • In the long run, this should be refactored together with submit/lib.py to provide a consistent upload api for everybody, again.

views.py security

  • oauth authenticates user A; the URL contains user B; And for some cases, there is an already existing object O, owned by user C.
  • I guess, that all three users must be the same. If not, please explain in detail why (comments in code!)
  • Testing for A == B is already in the code.
  • Testing for C == A is not yet there.

view.py style

  • There are a bunch of
    obj = Klass.query.filter(...)
    if obj is None:
      return error
    obj = obj[0]
    
    in there. The if will never fire, as filter() always returns an unfinished query object. Please replace by
    obj = ...filter(...).first()
    if obj is None:
      return error
    

If any more issues are found please add them to this ticket. This is a rather large issue but most of the tasks in them are so small it doesn't warrant splitting them up.

Subtickets

Change History (12)

comment:1 Changed 7 years ago by Elrond

Description: modified (diff)

Add json_error idea.

comment:2 Changed 7 years ago by Elrond

Description: modified (diff)

Fix Cut'n'paste issue. Add placement comment.

comment:3 Changed 7 years ago by Jessica Tallon

Owner: set to Jessica Tallon
Status: newin_progress

comment:4 Changed 7 years ago by Jessica Tallon

I believe all of the current ticket has been done now. I have put the code on a federation branch on the mediagoblin repo so Elrond can review. Let me know if there are any issues with those changes.

comment:5 Changed 7 years ago by Elrond

Description: modified (diff)

comment:6 Changed 7 years ago by Elrond

A bunch of places, where you replaced the arguments to json_response with the arguments for json_error, but didn't replace the actuall function call:

 def whoami(request):
+        return json_response(error, status=401)
@@ -151,46 +149,39 @@ def feed(request):
+            return json_response("Unknown object type '{0}'.".format(object_type))
@@ -93,9 +90,9 @@ def uploads(request):
+    return json_response("Not yet implemented", 501)

Did you change the error code on purpose?

@@ -69,8 +67,7 @@ def uploads(request):
-        return json_response({"error": error}, status=404)
+        return json_error("No such 'user' with id '{0}'".format(user), 400)
@@ -34,10 +34,8 @@ def profile(request, raw=False):
-        return json_response({"error": error}, status=404)
+        return json_error("No such 'user' with id '{0}'".format(user), 400)

Where did mediagoblin/federation/task.py go?

Rest looks good.

comment:7 Changed 7 years ago by Jessica Tallon

I have fixed the json_errors you mentioned above. the task has gone to mediagoblin/submit as suggested earlier ("This task should be moved to submit").

comment:8 Changed 7 years ago by Jessica Tallon

Owner: Jessica Tallon deleted
Resolution: fixed
Status: in_progressclosed

This has now been pushed to master in commit 5e5d445. If you find any other issues feel free to re-open this issue.

comment:9 Changed 7 years ago by Elrond

Description: modified (diff)
Priority: majorcritical
Resolution: fixed
Status: closedaccepted

Re-Open for more items in views.py. Especially more security. See description.

comment:10 Changed 7 years ago by Jessica Tallon

Owner: set to Jessica Tallon
Status: acceptedin_progress

comment:11 Changed 7 years ago by Christopher Allan Webber

Owner: Jessica Tallon deleted
Status: in_progressreview

Jessica has done most of this, now I need to review it!

comment:12 Changed 7 years ago by Jessica Tallon

Resolution: fixed
Status: reviewclosed
Note: See TracTickets for help on using tickets.