Added some endpoints, cleaned up some details to API documentation

duckpunch commented 8 years ago

In this PR:

Cleaned up the schema somewhat so that it was more likely to pass prmd verify.
- At present, it doesn't verify all the way because I was doing an FK link with black_id and white_id. prmd is pretty opinionated about this.
Added a few endpoints. I omitted a few endpoints because they seemed duplicative.
- POST /api/v1/games Report a game result.
- Added
- GET /api/v1/games/ Get a game result by ID
- Added
- GET /api/v1/games//sgf Get a game's SGF file by ID
- Skipped. Seems duplicative with the other GET.
- GET /api/v1/players/ Get a player by ID
- Added
- GET /api/v1/players?token= Get a player by their secret token.
- Skipped. Seems duplicative with player GET.
API docs looks like this now
- I'm in favor of something that resembles {API/spec/contract}-driven-development. In other words, we work out the details of API semantics before implementing them.
I tried to rebase and failed :(
- If this bugs you, I can try to fix it before the merge.

duckpunch commented 8 years ago

updated docs

amj commented 8 years ago

LGTM?

artasparks commented 8 years ago

LGTM. I don't have a strong opinion about the auth stuff now that I understand what's going on. I have a slight preference for including the tokens in the post body, but I see the point if you want to make this API a protocol (that's the intent, right?)

brilee commented 8 years ago

There's 3 options for token location: post body; queryparams; and custom headers. Of the three, the custom headers is the most "correct" way to do it; queryparams are the most popular way to do it; and post body I think is just not right - REST asks that you submit as the body of a request a representation of the object. Auth is not part of the object so it should come in through a different channel.

Let's stick with queryparams - that's how it's currently implemented, and it's what people will be most familiar with. See details of current implementation here: https://github.com/duckpunch/online-ratings/blob/322a6fcbcaa304c2ffdc8e33862700dc6af3a254/web/app/api_1_0/game_resource.py#L32

So a request would look like POST /api/v1/games?server_tok=blah&w_tok=blah&b_tok=blah {... game body...}

The name of the params should be expanded to "server_token", "white_token", and so on.

duckpunch commented 8 years ago

queryparams are the most popular way to do it

Can you link some examples? I'm not sure I've seen that before.

Conveniently, this popped up on hacker news.

A bunch of them use Basic Auth.

OAuth is also pretty popular.

and post body I think is just not right

That's fair.

duckpunch commented 8 years ago

and post body I think is just not right

I'm slowly refreshing my memory on this. I think the reason for using the POST body is because our case is a bit strange. We have 3 parties trying to "authenticate" - the server and the two players. I think I was assuming that the actual entity we care to authenticate is the server since it is the one actually integrating with OR. As such, we can use whatever auth (Basic/OAuth) for the server, and then pass the tokens as parameters.

They seem closer to data in this case than auth tokens.

duckpunch commented 8 years ago

Were you inspired by any particular API design or article? I'd be interested to see a similar use case where the integrating entity (the server) posts on the behalf of yet another party (the players).

brilee commented 8 years ago

Not inspired by any particular API. I've just had to integrate with a few in the past. The ones that are less developer-oriented tend to put ?api_key=blahblahblah in the queryparams. I can see why they do that; if you're one of the many html/css web designers who thinks jquery and javascript are synonymous, then it'd be quite a hassle to explain what HTTP headers are, and how you can customize them. OTOH most of the APIs in that HN thread use HTTP headers to do auth.

In this case we could probably assume our integrators at least know what http headers are and can customize them to use our API. I'd be up for switching to headers - stuff like Authorization: server_token, X-Auth-black-token: black_token, etc.

duckpunch commented 8 years ago

What do you think about the second comment I had? I think it's my favorite. More detailed version:

Authentication occurs between the server and OR via Basic auth, perhaps by base64 hashing "$server_name:$api_token". This looks to be pretty standard based on the examples I previously linked. In most libraries, this means user=$server_name and password=$api_token and conveniences are provided to do so.
Black/White player tokens are provided in the body because they aren't the main parties.

This way keeps things pretty standard. I'm not sure I buy the argument that we should speculate on what's easiest based on assuming the consumer is less skilled. I think that makes things more confusing, looks bad to the more sophisticated consumer, and more prone to bike shedding because the semantics aren't standard (though on the bike shedding point, that's kind of what this has become).

As a meta point, while I think this is a pretty important point, feel free to exercise your veto power.

brilee commented 8 years ago

I don't think I buy that the player tokens are closer to the data. The criteria for whether it goes in the body params, to me, is whether you can submit that field as part of a POST/PUT/GET request, and get it updated/returned/whatever. And the tokens do not fall into those categories. So those tokens should still go in the headers. As for basic auth, I realize that I was misusing the Authenticate header in my suggestion - it's intended for a very specific pattern of use that I was not doing. So I think it's probably best if all three tokens are stuffed in custom headers X-Auth-(server|black|white)-token to avoid confusion.

duckpunch commented 8 years ago

As for basic auth, I realize that I was misusing the Authenticate header in my suggestion - it's intended for a very specific pattern of use that I was not doing.

Do you mean Authorization? (The unfortunately misnamed header - you're right that it should be called Authenticate or some conjugation of it - See also, referer)

If we set aside black/white tokens, I think this use case becomes a lot simpler. That is, the server connects to OR and should authenticate. Basic/OAuth/whatever standard would work perfectly in this case.

Instead of thinking of black/white tokens as auth tokens, would you have a different opinion if they were thought of as IDs? Alternatively, would it possibly be simpler to just pass the online handle on that server?

e.g. The server could post something like this:

POST /games
Authorization: Basic <hash of server_id:api_token>

{
  "black": "TheCaptain",
  "white": "duckpunch",
  ...

In order for this game to "count", both black and white player should have "enabled" online play for that server under that handle.

brilee commented 8 years ago

We've already discussed dropping the player tokens - we want to allow players to have control over revoking their tokens even if the servers continue submitting games on their behalf. We shouldn't just drop them because we can't find a neat place for them. Custom headers are a perfectly okay place to put them.

duckpunch commented 8 years ago

we want to allow players to have control over revoking their tokens even if the servers continue submitting games on their behalf

The players would continue to have control since whether a server can submit a game on their behalf is still stored in OR.

We shouldn't just drop them because we can't find a neat place for them.

You're right if we lose features. I'm suggesting that we should drop them if we don't need them because we can maintain feature parity.

Custom headers are a perfectly okay place to put them.

Sure, but there are more standard, less confusing places to put them.

Anyway, it sounds like you're pretty set in keeping it in the headers, so I'll drop the issue.

Let me know what else you need here to land this PR. As I mentioned, I just made up some of the fields based on the tables which I'm guessing is pretty wrong.

duckpunch commented 8 years ago

I addressed your comments. LMK if there's more to be done before landing.

duckpunch commented 8 years ago

Don't forget to deploy to gh pages.

mkdocs gh-deploy --clean

It shows up at usgo.github.io/online-ratings.

brilee commented 8 years ago

K. How does authentication work? Is it uploaded to usgo.github.io by virtue of the fact that I'm part of the USGO organization? If so, could you do it?

Brian

On Sun, Jul 24, 2016 at 3:18 PM, duckpunch notifications@github.com wrote:

Don't forget to deploy to gh pages http://duckpunch.github.io/online-ratings/#deploying-to-gh-pages.

mkdocs gh-deploy --clean

It shows up at usgo.github.io/online-ratings.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/usgo/online-ratings/pull/27#issuecomment-234796336, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQlFp7tPZIsyL23Am3RM64uaPQUkQxZks5qY7p-gaJpZM4IpgqX .

duckpunch commented 8 years ago

Auth is a normal git push. The mechanism github uses is just that it serves a branch called gh-pages if you have it. Here are the instructions. mkdocs does that boilerplate work for you such that it's a single command. I don't think I have push access to the usgo repo.

In any case, it'd be good to spread the knowledge on doc deployment.

brilee commented 8 years ago

I just gave you push access (I think.) Can you give it a whirl and see if you can push the compiled docs?

usgo / online-ratings

Added some endpoints, cleaned up some details to API documentation #27