Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Development Part 2 (API Routes/Capabilities) #6289

Open
KentShikama opened this issue Aug 3, 2015 · 46 comments

Comments

Projects
None yet
@KentShikama
Copy link
Contributor

commented Aug 3, 2015

This issue has been created to keep track of the current API routes/capabilities progress. PRs for each route should be submitted individually.

Namespace

All API routes for our initial API release will be namespaced under /api/v0/. For example, if the current URL path to resource is /posts/1, the corresponding API route should be /api/v0/posts/1.

Steps to Creating an API route

  1. Refactor the current controller of the resource that you are trying to implement the API route for. This will probably involve extracting a Service class (see post_service.rb) and its corresponding tests (see posts_controller_spec.rb and post_service_spec.rb). You may want to submit a PR at this point to the develop branch.

  2. Add the route in routes.rb.

  api_version(module: "Api::V0", path: {value: "api/v0"}, default: true) do
    resources :posts, only: %i(show create destroy)
    # Add the resource route here
  end
  1. Write some tests for the API controller route under spec/integration/api/ that you will write in step 4 (see spec/integration/api/posts_controller_spec.rb).

  2. Add the corresponding controller under app/controllers/api/v0/ (for example, see app/controllers/api/v0/posts_controller.rb). Ideally it will only involve calling the service class you created in step 1 and adding in the required scope at the top. For now either the route will require the read scope or the read and write scopes.

List of Remaining API routes

  • Read a single post (Needs polishing)
  • Delete a single post (Needs polishing)
  • Post status messages (See https://kentshikama.com/posts/863) (Needs polishing)
  • Post comments (Needs polishing)
  • Delete comments (Needs polishing)
  • Like posts (Needs polishing)
  • Reshare posts
  • Read the stream (Needs polishing)
  • Read conversations
  • Write conversations
  • Upload photos through posts
  • Search for users
  • Search for tags
  • View a profile
  • Retrieve user photos
  • Retrieve generic pod information/stats (name, version, etc)
  • View notifications
  • Mark notifications as read
  • "Extended" profile

Less Priority

  • Post your location
  • Post a poll
  • Manage user ignores
  • Manage aspects (create, delete, maintain users)
  • Manage followed tags


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

You guys are on fire! <3 Go go go!

@jaywink jaywink added the api label Aug 3, 2015

@jhass

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

I do think we should design more scopes around this than read & write. And probably use an authorization gem to manage them, like pundit or cancancan.

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

Could we push that as an enhancement for the next iteration? It would be nice to keep the scopes simple to begin with so we get a working API in and then iterate on experiences.

@jhass

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

Giving an application access to something is not easily undone, if we make the changes too heavy between API versions, applications will just stay behind on the old versions and we'll have a harder time to phase them out.

@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2015

I suppose for starters adding a delete scope.

@jhass

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

That still feels way too broad for me. I can easily think of almost a dozen

  • Base permissions: read basic info as demanded by OIDC, read single public post, tag search, user search
  • posts:public:read: get the streams only containing the public posts, read interactions of public posts
  • posts:public:write: create public posts, create interactions of public posts, posts to services (includes reading which services are available of course, (publisher_metadata route?))
  • posts:limited: same as posts:public:* for limited posts, if you trust the app enough to retrieve your private data, you should also trust it enough to create private data in your name, read aspect names, read followed tags
  • conversations: read & write access to conversations
  • profiles: read private profile data of others you can read
  • contacts: create aspects, read/add/remove aspect memberships, also enables mentions (since that requires reading aspect memberships)
  • profile:read, profile:write: full read/write access to your own profile
  • followed_tags: write access to the list of followed tags, could perhaps be condensed into something else

And that's just from a quick brainstorm

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

Personally I feel going for a lot of scopes is just over engineering. Apps can still just ask for all of them.

IMHO, simple is beautiful. What is the real problem with read and read/write?

@jhass

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

We can hardly call ourselves privacy aware if we don't at least give users some control. Not completely wide open broad scopes (read: I still even consider the scopes in my suggestion quite broad) will also allow us to explain the user what they're giving the app access to on the authorization step.

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

More choices is not always better for privacy. Look at Android
permissions, it's a mess. Look at Facebook - there are so many normal
users don't understand half of them. The end result -> they just click "ok".

I'm still convinced we should keep things simple rather than complex
(simple can be more than just 2 scopes of course). Other opinions?

On 03.08.2015 12:03, Jonne Haß wrote:

We can hardly call ourselves privacy aware if we don't at least give
users some control. Not completely wide open broad scopes (read: I
still even consider the scopes in my suggestion quite broad) will also
allow us to explain the user what they're giving the app access to on
the authorization step.


Reply to this email directly or view it on GitHub
#6289 (comment).

Br,
Jason Robinson
mail@jasonrobinson.me
https://jasonrobinson.me

@jhass

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

For those that blindly click accept it does not matter if they do it to 9 boxes or just two or three. Offering just two or three however takes away the choice from those (even if they're few) that do not.

@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2015

I'm going to abstain from voicing my opinion too hard here. I would probably side with keeping it simple: openid (basic profile), extended (profile), read, write, destroy - and maybe even not allowing/releasing the destroy routes (and perhaps conversations and extended profile) until we know for the sure the authorization infrastructure is secure. However, my main motive is to roll this API out as fast as possible so I might have had voted/expressed differently if I wasn't actually involved in this API development.

@AugierLe42e

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

There's a third option. IMHO, having scopes is useless as soon as the application asks them itself. Like @jaywink says, nothing prevents the application to claim all scopes like most of Android apps do on Android. But there's an alternative as implemented in CyanogenMod: the application asks for a set of permissions but the user is able to manually deny each one in the settings menu of CM.

This is, IMHO, the best option. We shouldn't leave only two options to the user. What I propose is the following: the application claims a set of scopes. When the user autorises the application, he autorises them all. But we can add in the applications page switch to disable each scope seperatly without revoking the application.

@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2015

@AugierLe42e I'm just wondering if the added complexity of enabling the disabling of each scope for each application on the user applications setting is worth it. An application should only ask the permissions they need. I wouldn't download an application that asks for more. The tricky case is when an application can have a variable amount of permissions. For example, take an application that collects a stream of posts from various sources (one of them being Diaspora). Maybe it has an add-on feature in which it can also post to your Diaspora stream if you enable it (but only 1% of the users use it). The problem with the Google Play market is that such an application would probably ask for both read and write permissions, even though the write permission is only needed by 1% of the users. This is because the Google Play market doesn't allow variable permissions AFAIK. Fortunately with OIDC, the application can present a list of checkboxes next to a list of corresponding scope names/explanations to the user right before they initiate the OIDC flow (the application can describe here why they are needing the permission in question). If an application needs more permissions (maybe the user decided to use the add-on feature after all), the application could just go through the OIDC flow again with the additional scope (after revoking the current authorization as explained below). In other words, I want to say that a responsible application shouldn't require the revoking of certain scopes with our current model.

I just realized with the penultimate sentence in the last paragraph that we need to add a way for applications to revoke their own permissions for a particular user (which should be just creating one additional controller route). Otherwise, they will get a validation error on the uniqueness of the (user, application) pair when reinitializing the OIDC flow again.

Should we open a Loomio post for this discussion?

@AugierLe42e

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

An application should only ask the permissions they need.

I agree. But obviously you can't trust them to do so. This is the same problem that Android: the application asks you a huge pile of authorization to dispay screensavers. And if you don't want, then you can't use the app. I think we should add a bit more flexibility in the process.

Fortunately with OIDC, the application can present a list of checkboxes next to a list of corresponding scope names/explanations to the user right before they initiate the OIDC flow

Yea, but again, you shouldn't trust the applications. Developers won't use this feature (mainly becaus they're lazy, some of them because they're malicious

Should we open a Loomio post for this discussion?

I think so.

@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

mainly becaus they're lazy

Yeah I feel like we shouldn't further encourage the laziness though. I mean otherwise might as well grant all scopes for all applications from the start and then do as you propose and have each scope disabled by the user. I'm hoping developers who use the API for Diaspora, which is all about being privacy-aware, will care enough to use as few permissions as possible. And even if the developers are initially "lazy", hopefully the privacy conscious Diaspora user base can pressure the developers into actually using this feature by not using the application. Perhaps a bit naive.

I think so.

I will do.

I'm actually starting to think that it might be better to break the scopes down by category, e.g., posts, conversations and then separate the category by read/write only if it makes a lot of difference. But anyways, I suppose the biggest thing I'm looking for is a consensus within the community. Vacation is almost over for me. And I can always change my personal pod to have the settings I would like.

@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

@AugierLe42e

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2015

I'm actually starting to think that it might be better to break the scopes down by category, e.g., posts, conversations and then separate the category by read/write only if it makes a lot of difference.

Categories are what I had in mind initially.

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2015

But anyways, I suppose the biggest thing I'm looking for is a consensus within the community.

Well, there doesn't seem to be that many comments going around, only us here. I don't want to hold you back from working on the API - please do so if you can and want to. Could the routes be worked on without defining the scopes too strictly at this point? If not, I think go with a compromise between complex and simple - then we can vote on loomio to accept that :)

The categories sounds like a good compromise.

@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2015

@jaywink Thanks for trying to come to a consensus. If it is just a matter of adding in more scopes to replace the current read/write it shouldn't be problematic at all. @AugierLe42e 's idea might involve some further changes to the authorization infrastructure, so if we are going with that route, we might want to discuss it before we get too far into the API routes. But yeah for now I'll just continue with read/write.

@jhass I'm now taking a look at pundit and cancan, and at first glance I feel like a lot of their functionalities are not so applicable or overlapping with Nov's openid-connect gem. Pundit's scopes and cancan's abilities seem to be based around the idea that a user is a specific category (e.g., "admin", "collaborator") and that determines whether or not a user has access but we don't actually allocate permissions based on the user "category". Pundit and cancan can handle unauthorized access but Nov already has ways to handle unauthorized access errors under rack/oauth2/server/resource/error.rb in his gem. Whether we use the gems or not, we still will have to rescue the exception that are thrown by unauthorized access and provide appropriate error messages. Cancan has a "Lock It Down" feature which "ensures authorization happens on every action in your application" but since we aren't using the password flow, we can just require the openid scope in the base controller for the API routes. It seems like both gems might be more fit for refactoring the admin routes/controllers we have.

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Aug 9, 2015

I didn't have a look at the code so I have no idea if this sounds doable but I saw you talked about the (horrible) way Android is dealing with permissions. There is another approach than checkboxes during installation: ask for permission on demand. That's the way Firefox OS (and every web browsers) goes and it is awesome. For example, the user can attach his location to the post he writes. That means the app needs to access the position of the user. With Android, that means the app will have to ask for the permission to access user position during the installation process, and does that even if the user is never going to use this feature. So the app dev has the location of every user, and that's a fail. With Firefox OS, the app (website) will ask the permission only when it needs it, that means when the user taps on the location icon. So, only users who will do it will share their location, and they can directly see when the permission is used, so they can take a better decision.

I don't know if we can do that here, but that would be way better than the Android way.

@AugierLe42e

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2015

I didn't have a look at the code so I have no idea if this sounds doable but I saw you talked about the (horrible) way Android is dealing with permissions. There is another approach than checkboxes during installation: ask for permission on demand.

That's what I was saying: CyanogenMod does this too. Here is how it looks:

screenshot_2015-08-09-23-30-13

I, too, like this way of managing things because, even if the user have granted permissions, he is not binded to this acceptance. He can manage every permission without revoking eveything.

I don't think this it is a big deal. IMHO, it is just adding a boolean column to the authorisation model next to the scope saying denied or not.

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2015

Again, let's keep it simple please. A certain amount of scopes that application asks for and then app has those permissions. Apps will very likely do automatic stuff so always asking for permissions on demand is not an option.

@jhass

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

Let's postpone user revokable scopes until we have the base API in a release and some real world apps using and thus can evaluate if it's needed and how it would affect real world applications.

@AugierLe42e

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2015

Sounds good to me.

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

Apps will very likely do automatic stuff so always asking for permissions on demand is not an option.

There is a check with "remember my choice". That's the way web browsers go, see when you attach your location in diaspora* for example ;) (the pop up in the browser, etc).

I'm not working on this so I let you guys organize your work the way you want to, but it seems to me that the chosen approach is something fundamental so it should be decided at the beginning.

@jhass jhass added the feature label Aug 10, 2015

@loziniak

This comment has been minimized.

Copy link

commented Nov 10, 2015

Hello. I would add "Read specific tag stream". Do you plan such feature now or would it be a future thing?

@jaywink jaywink referenced this issue Nov 19, 2015

Closed

Diaspora api #4554

@jhass jhass referenced this issue Jul 3, 2016

Closed

Define scopes #3

@vanitasvitae

This comment has been minimized.

Copy link

commented Sep 9, 2016

Are the ticked API parts already available on Diaspora? Or do you plan to release the api all at once, once its finished?

@AugierLe42e

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

The ticked part are present in 0.6.

@svbergerem

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

@AugierLe42e Could you tell me where I can find the code for the checked routes? I wasn't able to find it for any of these.

@AugierLe42e

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

Oh, my bad, that's part 2. @KentShikama hasn't submitted the PR yet but they are present on his api branch. Sorry for the mistake.

@frankrousseau

This comment has been minimized.

Copy link

commented Mar 9, 2017

Hello Diaspora community. I would like to know if you still need help. I don't have much time but I think I can work on a controller or two (maybe the conversation controllers). I can also work on the polishing of already developed controllers.

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

Hi @frankrousseau, yes we still need help on this feature. Maybe you can have a look at @KentShikama work (as linked in the comment just above) as he doesn't seem to be much active at the moment. Please read our wiki page to help you start on the right foot. You can set up a dev environment by following the install guide (you can select development instead of production on this page). There are also various docker images outside here but none of them are officially supported at the moment.

However, even if the codebase has been refactored a lot during the last years, it is still huge, so maybe you want to start with a newcomer issue which should be easier to solve than the API which is a big work?

@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

Hey @frankrousseau ! The API routes involve a lot of refactoring and thus understanding of the current codebase; tell me if you need help.

@frankrousseau

This comment has been minimized.

Copy link

commented Mar 18, 2017

I will have a look at it. But if it requires a lot of refactoring, I don't think I will have enough time. I will update you when I will make some progress.

@frankrousseau

This comment has been minimized.

Copy link

commented Mar 21, 2017

I have one more question @KentShikama. Which branch/repo is the best to start working on the API?

@Flaburgan I will look at newcomers tickets but I'm more interested in the API (I want a CLI client for Diaspora).

@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2017

@frankrousseau As @AugierLe42e mentioned, I would at least take a look at what I tried on my API branch https://github.com/KentShikama/diaspora/tree/api . Perhaps there have already been some refactorings done on the develop branch since but.

@frankrousseau

This comment has been minimized.

Copy link

commented Mar 23, 2017

Thx @KentShikama. So the first step will be to rebase your branch :)

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Apr 9, 2017

Hey @frankrousseau any news on this? Don't hesitate if you have questions ;)

@frankrousseau

This comment has been minimized.

Copy link

commented Apr 10, 2017

Hello @Flaburgan, I'm working on it but I make slow progress. I was busy with other things. I started from the branch of @KentShikama. I did a rebase. Now my main concern is to understand how to write tests. I started something for conversations but it's still a work in progress.

@frankrousseau

This comment has been minimized.

Copy link

commented Apr 13, 2017

I have a few questions:

  • How does the conversation model work? 
  • Do you have to create a conversation then add a message on it?
  • Do I have to create a conversation visibility object too?
  • What does the guid field mean?
@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

@frankrousseau I'm not exactly sure about the first three bullets but they can be deduced through tracing through the current conversations (non-API) endpoints.

guid stands for globally unique id. It is required because simple id's aren't unique across pods.

This was referenced May 21, 2017

@frankrousseau

This comment has been minimized.

Copy link

commented May 21, 2017

@KentShikama @Flaburgan I opened a pull request for this, because I think it's better to discuss around code.

I added the conversation controllers. But I'm new to Ruby, Ruby on Rails, Rspec and the Diaspora codebase. So there are probably flaws.

Can you tell me what you think about it and what do you want to do next?

@KentShikama

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2017

@frankrousseau Yeah you'll have to make some changes (including to the routes I attempted) to make it compliant with the spec. This will probably involve diving into some of the helpers to change the structure of the json. The tricky part is that you can't just wipe out the current structure for the front-end (non-api).

@Flaburgan

This comment has been minimized.

Copy link
Member

commented May 22, 2017

The tricky part is that you can't just wipe out the current structure for the front-end (non-api).

That's a good question actually, I guess the long term goal is to make the front-end use the API? @svbergerem @denschub

@denschub

This comment has been minimized.

Copy link
Member

commented May 22, 2017

Well, that's a "that would be pretty epic to have" kind of scenario, and that will happen when we rewrite our frontend using more sane technologies. But eh, that's something that's not going to happen anytime soon, so totally out of the scope if the discussion here.

Also yeah, sorry that this stuff is incompatible, but we agreed on the structure in the API docs and the current frontend is in a state where changes are not that easy. So, we basically have to maintain two presenters for each model. But that's not bad, given we have to think about versioning the API in the future anyway, that's probably a good thing. And in the end, there should be no logic inside the presenters, so that shouldn't be a big deal.

@amraboelela

This comment has been minimized.

Copy link

commented Jan 3, 2018

Hi Guys, I am new here. However, I am an iOS developer and would like to help. Is there any existing iOS client code? Or should I create one from scratch, to test the current APIs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.