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

Add more granular OAuth scopes #7929

Merged
merged 6 commits into from Jul 5, 2018
Merged

Add more granular OAuth scopes #7929

merged 6 commits into from Jul 5, 2018

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Jul 2, 2018

Fix #7121

  • write
    • write:accounts
      • PUT /api/v1/accounts/verify_credentials
      • POST /api/v1/statuses/:id/pin
      • POST /api/v1/statuses/:id/unpin
    • write:blocks
      • POST /api/v1/accounts/:id/block
      • POST /api/v1/accounts/:id/unblock
      • POST|DELETE /api/v1/domain_blocks
    • write:favourites
      • POST /api/v1/statuses/:id/favourite
      • POST /api/v1/statuses/:id/unfavourite
    • write:filters
      • POST /api/v1/filters
      • PUT|DELETE /api/v1/filters/:id
    • write:follows
      • POST /api/v1/accounts/:id/follow
      • POST /api/v1/accounts/:id/unfollow
      • POST /api/v1/follows
      • POST /api/v1/follow_requests/:id/authorize
      • POST /api/v1/follow_requests/:id/reject
    • write:lists
      • POST|DELETE /api/v1/lists/:id/accounts
      • POST /api/v1/lists
      • PUT|DELETE /api/v1/lists/:id
    • write:media
      • POST /api/v1/media
      • PUT /api/v1/media/:id
    • write:mutes
      • POST /api/v1/statuses/:id/mute
      • POST /api/v1/statuses/:id/unmute
      • POST /api/v1/accounts/:id/mute
      • POST /api/v1/accounts/:id/unmute
    • write:notifications
      • POST /api/v1/notifications/clear
      • POST /api/v1/notifications/:id/dismiss
    • write:reports
      • POST /api/v1/reports
    • write:statuses
      • POST /api/v1/statuses/:id/reblog
      • POST /api/v1/statuses/:id/unreblog
      • POST /api/v1/statuses
      • DELETE /api/v1/statuses/:id
  • read
    • read:accounts
      • GET /api/v1/accounts/verify_credentials
      • GET /api/v1/accounts/:id/followers
      • GET /api/v1/accounts/:id/following
      • GET /api/v1/accounts/search
      • GET /api/v1/statuses/:id/favourited_by
      • GET /api/v1/statuses/:id/reblogged_by
      • GET /api/v1/accounts/:id
    • read:blocks
      • GET /api/v1/blocks
      • GET /api/v1/domain_blocks
    • read:favourites
      • GET /api/v1/favourites
    • read:filters
      • GET /api/v1/filters
      • GET /api/v1/filters/:id
    • read:follows
      • GET /api/v1/accounts/relationships
      • GET /api/v1/follow_requests
    • read:lists
      • GET /api/v1/accounts/:id/lists
      • GET /api/v1/lists/:id/accounts
      • GET /api/v1/lists
      • GET /api/v1/lists/:id
    • read:mutes
      • GET /api/v1/mutes
    • read:notifications
      • GET /api/v1/notifications
      • GET /api/v1/notifications/:id
    • read:reports
      • GET /api/v1/reports
    • read:search
      • GET /api/v1/search
      • GET /api/v2/search
    • read:statuses
      • GET /api/v1/accounts/:id/statuses
      • GET /api/v1/timelines/direct
      • GET /api/v1/timelines/home
      • GET /api/v1/timelines/list/:id
      • GET /api/v1/statuses/:id
      • GET /api/v1/statuses/:id/context
      • GET /api/v1/statuses/:id/card
  • follow (legacy)
    • read:blocks
    • read:follows
    • read:mutes
    • write:blocks
    • write:follows
    • write:mutes
  • push

All UIs remain the same before, except the developer form for creating new apps, which I have adjusted:

image

@Gargron Gargron added api REST API, Streaming API, Web Push API work in progress Not to be merged, currently being worked on labels Jul 2, 2018
@Gargron Gargron force-pushed the feature-detailed-scopes branch 2 times, most recently from c835fed to 487b5e9 Compare Jul 2, 2018
@Gargron Gargron force-pushed the feature-detailed-scopes branch from 487b5e9 to 77d8f87 Compare Jul 2, 2018
@Gargron Gargron removed the work in progress Not to be merged, currently being worked on label Jul 2, 2018
@ThisIsMissEm
Copy link
Contributor

ThisIsMissEm commented Jul 3, 2018

I agree on the scopes, but not the design. We should group scopes by “privacy” level. Read public profile, Read Public toots, Etc. it should be grouped by what people do.

If I use an app, yes, I want to know they can do each thing, but I also what that high level overview.

A list like this isn’t useful to non technical users. We must make that simple for people to understand. What does each thing mean? Maybe displau it grouped by topic?

Favourites:   (read y/n) (write y/n)
Toots:           (read y/n) (write y/n)

ykzts
ykzts approved these changes Jul 3, 2018
@ThisIsMissEm
Copy link
Contributor

ThisIsMissEm commented Jul 3, 2018

That is, it should be grouped by type, not access level; they’re different vertices.

@ThisIsMissEm
Copy link
Contributor

ThisIsMissEm commented Jul 3, 2018

So, basically, you need to transpose this into a grid: field / read / write

For the top levels, they should just auto-check all below. Check all / read, abd it checks everything below it in read

@ThisIsMissEm
Copy link
Contributor

ThisIsMissEm commented Jul 3, 2018

The current design is too developer focused; from a user perspective I want to know who and what can do what things.

@Gargron
Copy link
Member Author

Gargron commented Jul 3, 2018

@ThisIsMissEm The screenshot is from the development "create app" UI. The authorize page is untouched, and simply lists the human description of the requested scopes as before.

@Gargron
Copy link
Member Author

Gargron commented Jul 3, 2018

I'm hoping it's an incremental improvement and will allow for more improvements on top of it in the future. Most importantly there is still no way to get different statuses depending on scope, it's all or nothing.

@ThisIsMissEm
Copy link
Contributor

ThisIsMissEm commented Jul 3, 2018

@Gargron okay, so, I’d take things that are generic (read/write/follow) and put those in a separate section, to discourage their use; I think “push” could also be split up into finer grain items, but it isn’t critical for now.

Then the remainder can be groups by object like shown above, rather than a flat list. Currently the list looks overwhelming and jumbled, but by adding grouping we can make it much clearer for developers & help encourage them to make better choices

@Gargron
Copy link
Member Author

Gargron commented Jul 3, 2018

Maybe I could assign color codes to each scope, and the top-level ones could be dangerous red or something, additionally to re-grouping them. Personally I don't know if client apps should get the top-level or all of the specific ones. What if a new feature comes out, like filters? Then all apps would have to re-register and re-authenticate their users...

I'm probably not going to add anything more to this PR though, because 37 changed files is on the high end of what I expect other contributors to read and review.

@ThisIsMissEm
Copy link
Contributor

ThisIsMissEm commented Jul 3, 2018

@Gargron Gargron force-pushed the feature-detailed-scopes branch from e13748a to 474e709 Compare Jul 5, 2018
@@ -1,7 +1,7 @@
# frozen_string_literal: true

class Api::V1::FollowsController < Api::BaseController
before_action -> { doorkeeper_authorize! :follow }
before_action -> { doorkeeper_authorize! :follow, :'write:follows', :'write:follows:create' }
Copy link
Contributor

@valerauko valerauko Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the :'write:follows:create' intentional? Is there a third tier too?

@@ -1,8 +1,8 @@
# frozen_string_literal: true

class Api::V1::ListsController < Api::BaseController
before_action -> { doorkeeper_authorize! :read }, only: [:index, :show]
before_action -> { doorkeeper_authorize! :write }, except: [:index, :show]
before_action -> { doorkeeper_authorize! :read, :'read:lists' }, only: [:index, :show]
Copy link
Contributor

@valerauko valerauko Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the colons were aligned too (there are a few others like this)

:'write:notifications',
:'write:reports',
:'write:statuses',
:read,
Copy link
Contributor

@valerauko valerauko Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be added to optional_scopes too?


before do
user.account.block_domain!('example.com')
allow(controller).to receive(:doorkeeper_token) { token }
end

shared_examples 'forbidden for wrong scope' do |wrong_scope|
Copy link
Contributor

@valerauko valerauko Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say this should be moved to some shared place once other controller specs use it too (blocks and accounts already have the same though)

@Gargron Gargron merged commit 1f6ed4f into master Jul 5, 2018
@Gargron Gargron deleted the feature-detailed-scopes branch Jul 5, 2018
lawremipsum pushed a commit to lawremipsum/mspsocial-mastodon that referenced this pull request Jul 7, 2018
* Add more granular OAuth scopes

* Add human-readable descriptions of the new scopes

* Ensure new scopes look good on the app UI

* Add tests

* Group scopes in screen and color-code dangerous ones

* Fix wrong extra scope
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api REST API, Streaming API, Web Push API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants