API shouldn't allow unauthenticated users to do many things

[cgolubi1]

Right now, the API allows unauthenticated users to do a number of tasks, such as list buttons, and perhaps also create games. That won't fly once we take down the firewall. The API should only allow unauthenticated users to run the API calls:

• login
• createUser
• verifyUser (this is borderline, but there'd be some work to make the UI friendlier if we didn't allow it, because new users would be required to (1) login, then (2) paste their verify string, whereas right now (1) paste verify string, then (2) login, also works. Leave it as is for now, maybe revisit later)

All other responder calls should check whether the user is logged in before doing anything else, and fail if not.

[irilyth]

One side effect of this is that it won't be possible to share links to story tournaments with people who aren't players, the Wayback Machine won't be able to archive pages, etc. That sounds fine for the beta, but it might be nice to allow unauthenticated access to more things at some point.

(I can make a separate ticket for that if it sounds good, or not if it doesn't.)

[cgolubi1]

That's not actually a side effect of this, since the wayback machine would be scraping the UI, which already won't show games to people who aren't logged in. I think it's fine to make a separate ticket for it.

[AdmiralJota]

As I mentioned in #627, I don't think this test should be left up to the individual methods, since it's too easy to forget it someplace (compare #630), especially as further functionality gets implemented.

I'd recommend doing this test when deciding which task is being requested, and to not even call the method unless either the user is authenticated or the task is one of the specifically whitelisted ones mentioned above.

[cgolubi1]

Yeah, agreed --- the architectural idea is that responder/ApiResponder is a thin wrapper around each function, which decides whether the arguments are okay and only then talks to BMInterface (or api_core in the case of login/logout) to do the actual function. So part of this ticket is to verify that that's true --- at any rate, my plan is not to leave it up to the individual functions, but to have a central part of ApiResponder nix the request unless the requested function allows unauth.

[cgolubi1]

I took a first cut at this, and now i'm trying to decide what i think: in particular, i'm thinking about what code an unauthenticated user can access, and whether we should do more to reduce that amount:

• src/api/responder itself: trivial
• src/api/ApiResponder: ~300 LOC. in principle, now most of that code is not available to the un-logged-in user.
  • What's directly available:
  • __construct(): has some includes, see below
  • process_request(): the landing function
  • verify_function_access(): the function i just wrote to gatekeep other functions, hopefully okay. :>)
  • get_interface_response_createUser(): thin wrapper around $interface->create_user()
  • get_interface_response_verifyUser(): thin wrapper around $interface->verify_user()
  • get_interface_response_loadPlayerName(): uses an api_core function to check the session
  • get_interface_response_login(): uses an api_core function to attempt login
  • What's included:
  • BMInterface: blech, huge. Now we don't include it until after we check that the user is allowed to access the specified function, but in fact create_user and verify_user actually need to write to the database and do real BMInterface things
  • api_core: needed to check/create logins and sessions
  • bootstrap: hmm, probably only needed for things that need interface, so maybe we could move this include later, but OTOH you need bootstrap to get to BMInterface, and you need BMInterface to create a user, so what are you gonna do?
• src/api/dummy_responder and src/api/DummyApiResponder: the latter is quite long because of all the embedded test game data, but, by design, doesn't include any other classes (not even the bootstrapper) or access any other systems, so it really should be pretty self-contained.

So, the big issue here is that the enormous 1600-line BMInterface class is in some sense "available" to the unauthenticated user, via createUser() and verifyUser(). At a glance, those two functions are pretty self-contained (they do some database stuff and invoke send_email_verification(), which nothing else uses and which invokes BMEmail stuff). So it's tempting to think about moving those two functions from BMInterface to a new class, BMInterfaceNewuser. I'm torn between: A. That would be quite easy to do and would reduce by a lot the amount of code in classes the unauthenticated user can access B. That would be silly and paranoid. The bootstrap mechanism means that if you can execute arbitrary code, you can execute any of our code, so what the heck kind of vulnerability would let you at other code inside the class you're in, but not other code in other places? (But, back on the first hand, it does seem plausible that such a thing might exist.)

Anyone got a vote? If i don't move BMInterface functions around, i'm done with this, and Jenkins is http://jenkins.buttonweavers.com:8080/job/buttonmen-cgolubi1/180/ (for my future reference). If i do move BMInterface functions, then i want to do that on this ticket.

[cgolubi1]

Enh, as is often the case with these things, it takes longer to think about it than to do it: i went ahead and split off the BMInterface newuser functions.