The Wayback Machine - https://web.archive.org/web/20191117130521/https://github.com/tootsuite/mastodon/pull/7929
Skip to content

/mastodon

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
+292 −102

Conversation

@Gargron
Copy link
Member

Gargron commented Jul 2, 2018 •
edited

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 force-pushed the feature-detailed-scopes branch 2 times, most recently Jul 2, 2018
@Gargron
Add more granular OAuth scopes
Loading status checks…
77d8f87
@Gargron Gargron force-pushed the feature-detailed-scopes branch to 77d8f87 Jul 2, 2018
Gargron added 2 commits Jul 2, 2018
@Gargron
Add human-readable descriptions of the new scopes
Loading status checks…
80d5d79
@Gargron
Ensure new scopes look good on the app UI
Loading status checks…
297db5d
@ThisIsMissEm

This comment has been minimized.

Sign in to view
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
View changes
@ThisIsMissEm

This comment has been minimized.

Sign in to view
Copy link
Contributor

ThisIsMissEm commented Jul 3, 2018

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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Contributor

ThisIsMissEm commented Jul 3, 2018

@Gargron
Add tests
Loading status checks…
474e709
@Gargron Gargron force-pushed the feature-detailed-scopes branch to 474e709 Jul 5, 2018
@valerauko
valerauko reviewed Jul 5, 2018
View changes
app/controllers/api/v1/follows_controller.rb Outdated
@@ -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' }

This comment has been minimized.

Sign in to view
Copy link
@valerauko

valerauko Jul 5, 2018

Contributor

Is the :'write:follows:create' intentional? Is there a third tier too?
app/controllers/api/v1/lists_controller.rb
@@ -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]

This comment has been minimized.

Sign in to view
Copy link
@valerauko

valerauko Jul 5, 2018

Contributor

Previously the colons were aligned too (there are a few others like this)
config/initializers/doorkeeper.rb
:'write:notifications',
:'write:reports',
:'write:statuses',
:read,

This comment has been minimized.

Sign in to view
Copy link
@valerauko

valerauko Jul 5, 2018

Contributor

Does this need to be added to optional_scopes too?
spec/controllers/api/v1/domain_blocks_controller_spec.rb

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|

This comment has been minimized.

Sign in to view
Copy link
@valerauko

valerauko Jul 5, 2018

Contributor

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 added 2 commits Jul 5, 2018
@Gargron
Group scopes in screen and color-code dangerous ones
Loading status checks…
bee7bce
@Gargron
Fix wrong extra scope
Loading status checks…
8fbfebc
@Gargron Gargron merged commit 1f6ed4f into master Jul 5, 2018
9 checks passed
9 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details
@Gargron Gargron deleted the feature-detailed-scopes branch Jul 5, 2018
lawremipsum added a commit to lawremipsum/mspsocial-mastodon that referenced this pull request Jul 7, 2018
@Gargron @lawremipsum
Add more granular OAuth scopes (tootsuite#7929)
2da3246 
* 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
@zxvfxwing zxvfxwing referenced this pull request Jul 18, 2018
Open
@pwoolcoc pwoolcoc referenced this pull request Aug 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valerauko valerauko

@ykzts ykzts

Assignees
No one assigned
Labels
api
Projects
None yet
Milestone
No milestone
4 participants
@Gargron @ThisIsMissEm @ykzts @valerauko
You can’t perform that action at this time.