Plume
/
Plume
8
16
Vork 22
Code Kwesties 174 Pull-aanvragen 8 Publicaties 8 Wiki Activiteit

Labels Mijlpalen

API Authentication #285

Samengevoegd
elegaanz heeft 13 commits samengevoegd van api-auth naar master 6 jaren geleden
Discussie 0 Commits 13 Bestanden gewijzigd 23
elegaanz reageerde 6 jaren geleden (Gemigreerd van 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 beoordeeld 6 jaren geleden
src/main.rs
trinity-1686a reageerde 6 jaren geleden
Eigenaar

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 (Gemigreerd van github.com) beoordeeld 6 jaren geleden
igalic (Gemigreerd van github.com) heeft een reactie achtergelaten

👀

👀
plume-models/src/apps.rs
@ -0,0 +1,77 @@
use canapi::{Error, Provider};
igalic (Gemigreerd van github.com) reageerde 6 jaren geleden

should i be watching the canapi repo as well?

should i be watching the canapi repo as well?
elegaanz (Gemigreerd van github.com) beoordeeld 6 jaren geleden
plume-models/src/apps.rs
@ -0,0 +1,77 @@
use canapi::{Error, Provider};
elegaanz (Gemigreerd van github.com) reageerde 6 jaren geleden

As you want

As you want
trinity-1686a beoordeeld 6 jaren geleden
trinity-1686a heeft een reactie achtergelaten
Eigenaar

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 reageerde 6 jaren geleden
Eigenaar

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 reageerde 6 jaren geleden
Eigenaar

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 reageerde 6 jaren geleden
Eigenaar

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

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 reageerde 6 jaren geleden
Eigenaar

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

both error message ("Wrong password" and "Unknown user") should probably be merged, for similar reasons as #170
elegaanz (Gemigreerd van github.com) beoordeeld 6 jaren geleden
docs/API.md
elegaanz (Gemigreerd van github.com) reageerde 6 jaren geleden

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 (Gemigreerd van github.com) beoordeeld 6 jaren geleden
plume-api/src/apps.rs
@ -0,0 +1,13 @@
use canapi::Endpoint;
#[derive(Clone, Default, Serialize, Deserialize)]
pub struct AppEndpoint {
elegaanz (Gemigreerd van github.com) reageerde 6 jaren geleden

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 (Gemigreerd van github.com) beoordeeld 6 jaren geleden
src/api/authorization.rs
elegaanz (Gemigreerd van github.com) reageerde 6 jaren geleden

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 (Gemigreerd van github.com) beoordeeld 6 jaren geleden
src/api/authorization.rs
elegaanz (Gemigreerd van github.com) reageerde 6 jaren geleden

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

As you can see, these two Options will actually always be None, never Some(A) or Some(S).
trinity-1686a beoordeeld 6 jaren geleden
src/api/authorization.rs
trinity-1686a reageerde 6 jaren geleden
Eigenaar

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 beoordeeld 6 jaren geleden
trinity-1686a heeft een reactie achtergelaten
Eigenaar

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

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 reageerde 6 jaren geleden
Eigenaar

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 reageerde 6 jaren geleden
Eigenaar

Same goes here (about variable naming)

Same goes here (about variable naming)
src/api/posts.rs
trinity-1686a reageerde 6 jaren geleden
Eigenaar

Same here (about access without tokens)

Same here (about access without tokens)
trinity-1686a reageerde 6 jaren geleden
Eigenaar

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)

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 heeft deze veranderingen 6 jaren geleden goedgekeurd

Reviewers

trinity-1686a heeft deze veranderingen 6 jaren geleden goedgekeurd
De pull request is samengevoegd als e26a150164.
Je kunt ook command line instructies bekijken.

Stap 1:

Vanuit het project, check een branch uit en test de veranderingen.
git checkout -b api-auth master
git pull origin api-auth

Stap 2:

Voeg de wijzigingen samen en update ze op Forgejo.
git checkout master
git merge --no-ff api-auth
git push origin master
Log in om deel te nemen aan deze discussie.
Reviewers
Beoordeling aanvragen
Geen beoordelaars
trinity-1686a
Labels
Labels toepassen
Verwijder labels   
A: API

Related to the REST API   
A: Backend

Code running on the server   
A: Federation

Stuff related to Federation   
A: Front-End

Related to the front-end   
A: I18N

Translations, and related code   
A: Meta

More about project management or code than the project itself   
A: Security
   
Build

The building, or installation process of Plume   
C: Bug

Something isn't working   
C: Discussion

We need to talk   
C: Enhancement

New feature or request   
C: Feature

This is a new feature   
Compatibility

Compatibility with different browsers, readers and OS   
Dependency

Related to an external package that Plume uses   
Design

UI/UX related issues and PRs   
Documentation
   
Good first issue

Good for newcomers   
Help welcome

Extra attention is needed   
Mobile

Issues affecting only mobile UX   
Rendering

How elements're rendered out for the end user   
S: Blocked

Something else needs to be fixed first   
S: Duplicate

This issue or pull request already exists   
S: Incomplete

This PR is not complete yet   
S: Instance specific

Issues concern a limited number of instances   
S: Invalid

This doesn't seem right   
S: Needs Voting/Discussion

Need to be discussed by the community (on Loomio)   
S: Ready for review

This PR is ready to be reviewed   
Suggestion

Proposed ideas worth considering   
S: Voted on Loomio

This is issue has been created after a vote on Loomio   
S: Wontfix

This will not be worked on
Geen label
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
Mijlpaal
Stel mijlpaal in
Verwijder mijlpaal
Geen items
Geen mijlpaal
Toegewezen aan
Gebruikers toewijzen
Verwijder toegewezen aan
Niet toegewezen
2 deelnemers
Notificaties
Vervaldatum
De deadline is ongeldig of buiten bereik. Gebruik het formaat 'jjjj-mm-dd'.

Geen vervaldatum ingesteld.

Afhankelijkheden

Geen afhankelijkheden ingesteld.

Referentie: Plume/Plume#285
Schrijf Voorbeeld
Laden…
Annuleren
Opslaan
Er is nog geen inhoud.
Verwijder branch 'api-auth'

Het verwijderen van een branch is permanent. Het KAN NIET ongedaan gemaakt worden. Wil je toch doorgaan?

Nee
Ja