API Authentication #285

trinity-1686a · 2018-10-21T16:28:07Z

elegaanz hat

2018-10-21 16:28:07 +00:00

kommentiert (Migriert von github.com)

App model
API endpoint to register an app
ApiToken model
OAuth2 endpoint to get a token, from app and user credentials
make ApiToken a request guard

Fixes #275

- [x] `App` model - [x] API endpoint to register an app - [x] `ApiToken` model - [x] OAuth2 endpoint to get a token, from app and user credentials - [x] make `ApiToken` a request guard Fixes #275

❤️ 2

trinity-1686a hat 2018-10-21 18:54:15 +00:00 gereviewt

src/main.rs

trinity-1686a hat

2018-10-21 18:54:15 +00:00

kommentiert

Besitzer

I think actually the whole /api can be authorized, if I remember well it's denoted by /api/<path..>

I think actually the whole `/api` can be authorized, if I remember well it's denoted by `/api/<path..>`

igalic (Migriert von github.com) hat 2018-10-22 08:35:57 +00:00 gereviewt

igalic (Migriert von github.com) hat einen Kommentar hinterlassen

👀

plume-models/src/apps.rs

					
				@ -0,0 +1,77 @@

				use canapi::{Error, Provider};

igalic (Migriert von github.com) hat

2018-10-22 08:31:27 +00:00

kommentiert

should i be watching the canapi repo as well?

elegaanz (Migriert von github.com) hat 2018-10-22 14:11:30 +00:00 gereviewt

plume-models/src/apps.rs

					
				@ -0,0 +1,77 @@

				use canapi::{Error, Provider};

elegaanz (Migriert von github.com) hat

2018-10-22 14:11:30 +00:00

kommentiert

As you want

trinity-1686a hat 2018-10-22 19:55:39 +00:00 gereviewt

trinity-1686a hat einen Kommentar hinterlassen

Besitzer

I think it could help with readability to have a special type with some parameters, doing by itself something similar to ApiToken.can(), and which could be used as a request guard (so we could do something like fn read_post( [...] , _authorized: Authorization<Read, Post>) and it would deny request without tests to do by ourself)

I think it could help with readability to have a special type with some parameters, doing by itself something similar to ApiToken.can(), and which could be used as a request guard (so we could do something like `fn read_post( [...] , _authorized: Authorization<Read, Post>)` and it would deny request without tests to do by ourself)

docs/API.md

trinity-1686a hat

2018-10-22 19:27:41 +00:00

kommentiert

Besitzer

Just "use apps" is maybe a bit unclear, maybe you should use the full path, and also tell how to use it (Do a post with such data, and such other is optional and....)

token. To do so, use the `/api/v1/apps` API (accessible without a token) to create

Just "use apps" is maybe a bit unclear, maybe you should use the full path, and also tell how to use it (Do a post with such data, and such other is optional and....) ```suggestion token. To do so, use the `/api/v1/apps` API (accessible without a token) to create ```

plume-api/src/apps.rs

trinity-1686a hat

2018-10-22 19:39:42 +00:00

kommentiert

Besitzer

name seems to be required by 36297101f2/plume-models/src/apps.rs (L47)
So I think it should not be an Option

name seems to be required by https://github.com/Plume-org/Plume/blob/36297101f261f473fd03d86a22f46379d125e002/plume-models/src/apps.rs#L47 So I think it should not be an Option

plume-api/src/apps.rs

					
				@ -0,0 +1,13 @@

				use canapi::Endpoint;

				#[derive(Clone, Default, Serialize, Deserialize)]

				pub struct AppEndpoint {

trinity-1686a hat

2018-10-22 19:39:31 +00:00

kommentiert

Besitzer

This feel strange to be at the same time data received from Post (with id, client_id and client_secret ignored, as they must be generated by the server) and data returned by the api (with those same field used, and most likely different than what was originally posted if they where). It should either be 2 different struct or at least a struct with FromForm custom-implemented to ensure that

src/api/mod.rs

trinity-1686a hat

2018-10-22 19:47:14 +00:00

kommentiert

Besitzer

both error message ("Wrong password" and "Unknown user") should probably be merged, for similar reasons as #170

elegaanz (Migriert von github.com) hat 2018-10-22 19:57:15 +00:00 gereviewt

docs/API.md

elegaanz (Migriert von github.com) hat

2018-10-22 19:57:13 +00:00

kommentiert

The request should be documented with Swagger (but it is broken for the moment 😢)

The request should be documented with Swagger (but it is broken for the moment :cry:)

elegaanz (Migriert von github.com) hat 2018-10-22 20:01:38 +00:00 gereviewt

plume-api/src/apps.rs

					
				@ -0,0 +1,13 @@

				use canapi::Endpoint;

				#[derive(Clone, Default, Serialize, Deserialize)]

				pub struct AppEndpoint {

elegaanz (Migriert von github.com) hat

2018-10-22 20:01:38 +00:00

kommentiert

We can't use FromForm in plume-api, or we would loose all the benefits of canapi.

But I think I may add a Server/Client/Both wrapper type to specify when a field is required and make it easier to check if something has been forgotten.

Or maybe canapi is just a bad idea and we should drop it... 🤔

We can't use `FromForm` in plume-api, or we would loose all the benefits of canapi. But I think I may add a `Server`/`Client`/`Both` wrapper type to specify when a field is required and make it easier to check if something has been forgotten. Or maybe canapi is just a bad idea and we should drop it... :thinking:

elegaanz (Migriert von github.com) hat 2018-10-23 11:19:07 +00:00 gereviewt

src/api/authorization.rs

elegaanz (Migriert von github.com) hat

2018-10-23 11:19:06 +00:00

kommentiert

I don't know if there is a better way to define this type. If I don't use A and S in its definition, it refuses to build.

I don't know if there is a better way to define this type. If I don't use `A` and `S` in its definition, it refuses to build.

elegaanz (Migriert von github.com) hat 2018-10-23 11:19:58 +00:00 gereviewt

src/api/authorization.rs

elegaanz (Migriert von github.com) hat

2018-10-23 11:19:58 +00:00

kommentiert

As you can see, these two Options will actually always be None, never Some(A) or Some(S).

trinity-1686a hat 2018-10-23 19:29:12 +00:00 gereviewt

src/api/authorization.rs

trinity-1686a hat

2018-10-23 19:29:11 +00:00

kommentiert

Besitzer

You can use PhantomData https://doc.rust-lang.org/beta/std/marker/struct.PhantomData.html, it'll probably do the trick
Simple example : https://gist.github.com/rust-play/9e51f5b8bb3a915a99d958f5ea982f1a

You can use PhantomData https://doc.rust-lang.org/beta/std/marker/struct.PhantomData.html, it'll probably do the trick Simple example : https://gist.github.com/rust-play/9e51f5b8bb3a915a99d958f5ea982f1a

trinity-1686a hat 2018-10-27 21:13:02 +00:00 gereviewt

trinity-1686a hat einen Kommentar hinterlassen

Besitzer

There are just a few quick things that should be changed or discussed, and this will be good to go

plume-models/src/api_tokens.rs

					
				@ -0,0 +56,4 @@

				    pub fn can_read(&self, scope: &'static str) -> bool {

				        self.can("read", scope)

				    }

trinity-1686a hat

2018-10-27 21:01:25 +00:00

kommentiert

Besitzer

This is confusing because can take a what set to "read" and a scope set to what. what should probably renamed scope, or something else should be renamed in can

This is confusing because `can` take a `what` set to "read" and a `scope` set to `what`. `what` should probably renamed `scope`, or something else should be renamed in `can`

plume-models/src/api_tokens.rs

					
				@ -0,0 +60,4 @@

				    pub fn can_write(&self, scope: &'static str) -> bool {

				        self.can("write", scope)

				    }

trinity-1686a hat

2018-10-27 21:01:34 +00:00

kommentiert

Besitzer

Same goes here (about variable naming)

src/api/posts.rs

trinity-1686a hat

2018-10-27 21:11:44 +00:00

kommentiert

Besitzer

Same here (about access without tokens)

trinity-1686a hat

2018-10-27 21:11:46 +00:00

kommentiert

Besitzer

this kind of endpoint can probably be called without tokens, at least as long as the post is published (and require a valid authorization, from a user having access to the post if it's not)

trinity-1686a hat die Änderungen 2018-10-30 15:44:15 +00:00 genehmigt

Anmelden, um an der Diskussion teilzunehmen.

Keine Reviewer

Kein Meilenstein

Kein Projekt

Niemand zuständig

2 Beteiligte

Nachrichten

Fällig am

Das Fälligkeitsdatum ist ungültig oder außerhalb des zulässigen Bereichs. Bitte verwende das Format „jjjj-mm-tt“.

Kein Fälligkeitsdatum gesetzt.

Abhängigkeiten

Keine Abhängigkeiten gesetzt.

Referenz: Plume/Plume#285

				`@ -0,0 +1,77 @@`
				`use canapi::{Error, Provider};`

				`@ -0,0 +1,77 @@`
				`use canapi::{Error, Provider};`

Zeilen
Spalten