/mastodon

Application prefs section #2758

Merged
merged 26 commits into from Aug 22, 2017

Conversation

Reviewers

@Gargron Gargron

@akihikodaki akihikodaki

@beatrix-bitrot beatrix-bitrot

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
5 participants
@muffinista @beatrix-bitrot @nightpool @Gargron @akihikodaki
@muffinista
Contributor

muffinista commented May 3, 2017

This adds a page to the preferences section to manage credentials for user-created applications.

The goal here is to make it easier for users to manage scripts that require API access. This could be clients, scripts that do some sort of data management, bots, etc. While it's obviously possible to get credentials for a script right now, it's a bit challenging and involved. With this code, you can create an app in the preferences section, and get a token right there. You can also regenerate your token if needed.

These changes also address an issue that exists in the code right now, which is that there is no way to edit the name or website of an app right now.
config/initializers/doorkeeper.rb
@@ -45,7 +45,7 @@
# Optional parameter :confirmation => true (default false) if you want to enforce ownership of
# a registered application
# Note: you must also run the rails g doorkeeper:application_owner generator to provide the necessary support
- # enable_application_owner :confirmation => true
+ enable_application_owner

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot May 7, 2017

Collaborator

does this have any effects on applications that exist already before these changes are applied?
@beatrix-bitrot

beatrix-bitrot May 7, 2017

Collaborator

does this have any effects on applications that exist already before these changes are applied?

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista May 7, 2017

Contributor

I don't believe so, and I'm running it in production with no reported issues.
@muffinista

muffinista May 7, 2017

Contributor

I don't believe so, and I'm running it in production with no reported issues.

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot May 10, 2017

Collaborator

that was my only concern really. there are some merge conflicts so i'm going to mark this as requesting changes
@beatrix-bitrot

beatrix-bitrot May 10, 2017

Collaborator

that was my only concern really. there are some merge conflicts so i'm going to mark this as requesting changes

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

I'm not sure even why owner is required. Could you explain the reason?
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

I'm not sure even why owner is required. Could you explain the reason?

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista Jun 25, 2017

Contributor

Yes, it sets the application owner to the current user if that data is available. Without that, we can't associate apps with owners.
@muffinista

muffinista Jun 25, 2017

Contributor

Yes, it sets the application owner to the current user if that data is available. Without that, we can't associate apps with owners.

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 26, 2017

Collaborator

I mean I doubt associating apps with owners is necessary. Isn't tracking access tokens with resource owners enough?
@akihikodaki

akihikodaki Jun 26, 2017

Collaborator

I mean I doubt associating apps with owners is necessary. Isn't tracking access tokens with resource owners enough?

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista Jun 26, 2017

Contributor

Maybe I'm missing something. The whole point of this PR is to associate applications with their creator so they can be managed and displayed in the settings section of that user. While tokens have an associated user, its very difficult to use that link to actually do any management of the app without some brittle hacks.
@muffinista

muffinista Jun 26, 2017

Contributor

Maybe I'm missing something. The whole point of this PR is to associate applications with their creator so they can be managed and displayed in the settings section of that user. While tokens have an associated user, its very difficult to use that link to actually do any management of the app without some brittle hacks.

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 26, 2017

Collaborator

I see. It looks good to me to add owner.
@akihikodaki

akihikodaki Jun 26, 2017

Collaborator

I see. It looks good to me to add owner.
config/initializers/doorkeeper.rb
@@ -45,7 +45,7 @@
# Optional parameter :confirmation => true (default false) if you want to enforce ownership of
# a registered application
# Note: you must also run the rails g doorkeeper:application_owner generator to provide the necessary support
- # enable_application_owner :confirmation => true
+ enable_application_owner

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot May 10, 2017

Collaborator

that was my only concern really. there are some merge conflicts so i'm going to mark this as requesting changes
@beatrix-bitrot

beatrix-bitrot May 10, 2017

Collaborator

that was my only concern really. there are some merge conflicts so i'm going to mark this as requesting changes
@muffinista

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista May 10, 2017

Contributor

OK, conflicts should be resolved, thanks!
Contributor

muffinista commented May 10, 2017

OK, conflicts should be resolved, thanks!
@beatrix-bitrot

beatrix-bitrot reviewed May 10, 2017
View changes

the CI checks are failing ):

@muffinista
Fix embarrassing typo 
I lost an `end` statement while fixing a merge conflict.
Loading status checks…
a4bacbd
@muffinista

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista May 10, 2017

Contributor

So sorry about that! I'm travelling and made a typo using the github inline editor. CI is passing now.
Contributor

muffinista commented May 10, 2017

So sorry about that! I'm travelling and made a typo using the github inline editor. CI is passing now.
@beatrix-bitrot

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot May 11, 2017

Collaborator

No worries. Also sorry to ask but could you add a screenshot or two? I'm trying to take a little more care with my reviews and so I'd either want to run your changes on one of my servers before approving or at least see how it looks in some screenshots.
Collaborator

beatrix-bitrot commented May 11, 2017

No worries. Also sorry to ask but could you add a screenshot or two? I'm trying to take a little more care with my reviews and so I'd either want to run your changes on one of my servers before approving or at least see how it looks in some screenshots.
@muffinista

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista May 11, 2017

Contributor

No problem at all. First, the code is running on botsin.space if you want to create an account.

Here's the app list:

screenshot 2017-05-10 20 41 16

Here's the Create New App page:

screenshot 2017-05-10 20 42 02

The app page, with tokens:
screenshot 2017-05-10 20 42 57
Contributor

muffinista commented May 11, 2017

No problem at all. First, the code is running on botsin.space if you want to create an account.

Here's the app list:

screenshot 2017-05-10 20 41 16

Here's the Create New App page:

screenshot 2017-05-10 20 42 02

The app page, with tokens:
screenshot 2017-05-10 20 42 57

muffinista added some commits May 23, 2017

@muffinista
Merge branch 'master' into application-prefs-section
Loading status checks…
02ea5a8
@muffinista
Merge branch 'master' into application-prefs-section
Loading status checks…
e875319
@muffinista
Merge branch 'master' into application-prefs-section
Loading status checks…
575181b
@muffinista
Add code for creating/managing apps to settings section 
- Add specs for app changes
- Add locale strings. Add 'my apps' to nav
- Add Client ID/Secret to App page. Add some visual separation
- Fix some bugs/warnings
f3bd8cd
@muffinista
Merge v1.4.1 into application-prefs-section
Loading status checks…
691f888
@muffinista
Merge branch 'master' into application-prefs-section
Loading status checks…
1bdca00

@akihikodaki akihikodaki added the enhancement label Jun 3, 2017

@muffinista
Merge branch 'master' of https://github.com/tootsuite/mastodon into a… 
…pplication-prefs-section
Loading status checks…
abf9710

@beatrix-bitrot beatrix-bitrot requested a review from akihikodaki Jun 18, 2017

@akihikodaki

akihikodaki requested changes Jun 19, 2017
View changes

It may be good to have a notice to tell users to keep tokens secret to the application page.
Mastodon has a variety of users and some of them are not familiar with them.

@nightpool

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Jun 19, 2017

Collaborator

Is there a reason the redirect URI is a textarea instead of just an input? the styling looks odd.
Collaborator

nightpool commented Jun 19, 2017

Is there a reason the redirect URI is a textarea instead of just an input? the styling looks odd.
@akihikodaki

akihikodaki requested changes Jun 19, 2017
View changes

For reuqests of additional tests; pending examples would be sufficient to merge this change, but actual examples will be appreciated.

Thank you for your contribution. This feature is essential.

spec/controllers/settings/applications_controller_spec.rb
+ describe 'GET #show' do
+ it 'returns http success' do
+ get :show, params: { id: app.id }
+ expect(response).to have_http_status(:success)

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

Spec that it acutally shows the application. (testing assigns(:application) is enough.)
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

Spec that it acutally shows the application. (testing assigns(:application) is enough.)
spec/controllers/settings/applications_controller_spec.rb
+ end
+
+ describe 'POST #create' do
+ describe 'success' do

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator
  • Test if it actually creates an application.
  • Use context instead of describe.
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator
  • Test if it actually creates an application.
  • Use context instead of describe.
spec/controllers/settings/applications_controller_spec.rb
+ end
+
+ describe 'PUT #update' do
+ describe 'success' do

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator
  • Test if it actually updates an application.
  • Use context instead of describe.
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator
  • Test if it actually updates an application.
  • Use context instead of describe.
spec/controllers/settings/applications_controller_spec.rb
+ end
+ end
+
+ describe 'PUT #update' do

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

It is not PUT but PATCH.
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

It is not PUT but PATCH.
spec/controllers/settings/applications_controller_spec.rb
+ end
+
+ it 'should create new token' do
+ expect( user.token_for_app(app) ).to_not eql(token)

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

Do not add spaces in parentheses.
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

Do not add spaces in parentheses.
spec/models/user_spec.rb
+ end
+
+ it 'is nil if user does not own app' do
+ app.owner = nil

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

Use update! instead of a combination of an assignment and save!.
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

Use update! instead of a combination of an assignment and save!.
config/initializers/doorkeeper.rb
@@ -45,7 +45,7 @@
# Optional parameter :confirmation => true (default false) if you want to enforce ownership of
# a registered application
# Note: you must also run the rails g doorkeeper:application_owner generator to provide the necessary support
- # enable_application_owner :confirmation => true
+ enable_application_owner

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

I'm not sure even why owner is required. Could you explain the reason?
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

I'm not sure even why owner is required. Could you explain the reason?

@akihikodaki akihikodaki added the security label Jun 19, 2017

@muffinista

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista Jun 26, 2017

Contributor

Thanks for the feedback! I think I've addressed most of the concerns raised here. I've updated the specs, added a note about not sharing API keys, and made a few other changes. I didn't change the textarea for the redirect URL for while I agree with @nightpool that the textarea looks a little weird here (especially with the default redirect), it makes it a lot easier to see URLs that are longer. I would be fine with changing it to a normal input though.
Contributor

muffinista commented Jun 26, 2017

Thanks for the feedback! I think I've addressed most of the concerns raised here. I've updated the specs, added a note about not sharing API keys, and made a few other changes. I didn't change the textarea for the redirect URL for while I agree with @nightpool that the textarea looks a little weird here (especially with the default redirect), it makes it a lot easier to see URLs that are longer. I would be fine with changing it to a normal input though.
@akihikodaki

akihikodaki requested changes Jun 27, 2017
View changes

I request some minor changes. This change will be ready to merge after they are addressed.

spec/controllers/settings/applications_controller_spec.rb
+ end
+
+ it 'returns 404 if you dont own app' do
+ app.owner = nil

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 27, 2017

Collaborator

Use update! here, too.
@akihikodaki

akihikodaki Jun 27, 2017

Collaborator

Use update! here, too.
spec/controllers/settings/applications_controller_spec.rb
+
+ describe 'GET #index' do
+ let(:other_app) { Fabricate(:application) }
+ before {

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 27, 2017

Collaborator

I think let! is what you expect, isn't it?
@akihikodaki

akihikodaki Jun 27, 2017

Collaborator

I think let! is what you expect, isn't it?
spec/controllers/settings/applications_controller_spec.rb
+ end
+
+ it 'removes the app' do
+ expect(Doorkeeper::Application.find_by(id: app.id)).to be nil

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 27, 2017

Collaborator

This is trivial, but be nil is used here while the other part uses be_nil. Anyway we do not have standard about that as far as I know, so it does not blocking merging.
@akihikodaki

akihikodaki Jun 27, 2017

Collaborator

This is trivial, but be nil is used here while the other part uses be_nil. Anyway we do not have standard about that as far as I know, so it does not blocking merging.

muffinista added some commits Jun 27, 2017

@muffinista
Tweak spec a bit
Loading status checks…
5fecc29
@muffinista
Cleanup fixture creation by using let!
Loading status checks…
83d69ec
@muffinista

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista Aug 16, 2017

Contributor

@beatrix-bitrot @akihikodaki pinging to check if/when this might be merged, and if anything else is needed to help with that? thanks!
Contributor

muffinista commented Aug 16, 2017

@beatrix-bitrot @akihikodaki pinging to check if/when this might be merged, and if anything else is needed to help with that? thanks!

@beatrix-bitrot beatrix-bitrot dismissed their stale review Aug 16, 2017

requested changes were addressed

@beatrix-bitrot

This comment has been minimized.

Show comment
Hide comment
@beatrix-bitrot

beatrix-bitrot Aug 16, 2017

Collaborator

@Gargron please merge, request changes, or close this~
Collaborator

beatrix-bitrot commented Aug 16, 2017

@Gargron please merge, request changes, or close this~
db/migrate/20170427011934_re_add_owner_to_application.rb
+class ReAddOwnerToApplication < ActiveRecord::Migration[5.0]
+ def change
+ add_column :oauth_applications, :owner_id, :integer, null: true
+ add_column :oauth_applications, :owner_type, :string, null: true

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Aug 21, 2017

Member

Wait, does this have to be polymorphic? We're never gonna attach the application to anything other than User. If this could be only owner_id with a foreign key ensuring integrity with the users table, that'd be far better.
@Gargron

Gargron Aug 21, 2017

Member

Wait, does this have to be polymorphic? We're never gonna attach the application to anything other than User. If this could be only owner_id with a foreign key ensuring integrity with the users table, that'd be far better.

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista Aug 22, 2017

Contributor

Adding the foreign key is no problem, it should be in the PR now.

I'm reluctant to remove the polymorphic association because it's part of Doorkeeper and the underlying code expects it, and removing it could have some weird side-effects. At the very least, removing owner_type breaks a bunch of specs I wrote which rely on the Application fabricator and while I can probably figure out a way around that issue, it seems like it would introduce a lot of potential fragility.
@muffinista

muffinista Aug 22, 2017 •
edited
Edited 1 time
  • @muffinista muffinista edited Aug 22, 2017 (most recent)

Contributor

Adding the foreign key is no problem, it should be in the PR now.

I'm reluctant to remove the polymorphic association because it's part of Doorkeeper and the underlying code expects it, and removing it could have some weird side-effects. At the very least, removing owner_type breaks a bunch of specs I wrote which rely on the Application fabricator and while I can probably figure out a way around that issue, it seems like it would introduce a lot of potential fragility.

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Aug 22, 2017

Member

Alright, that will do for now, since owner_type is essentially redundant.
@Gargron

Gargron Aug 22, 2017

Member

Alright, that will do for now, since owner_type is essentially redundant.

This comment has been minimized.

Show comment
Hide comment
@muffinista

muffinista Aug 22, 2017

Contributor

Thank you!
@muffinista

muffinista Aug 22, 2017

Contributor

Thank you!

@Gargron Gargron merged commit 871c0d2 into tootsuite:master Aug 22, 2017
2 checks passed

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

lindwurm added a commit to lindwurm/mastodon that referenced this pull request Aug 22, 2017

@muffinista @lindwurm
Application prefs section (#2758) 
* Add code for creating/managing apps to settings section

* Add specs for app changes

* Fix controller spec

* Fix view file I pasted over by mistake

* Add locale strings. Add 'my apps' to nav

* Add Client ID/Secret to App page. Add some visual separation

* Fix rubocop warnings

* Fix embarrassing typo

I lost an `end` statement while fixing a merge conflict.

* Add code for creating/managing apps to settings section

- Add specs for app changes
- Add locale strings. Add 'my apps' to nav
- Add Client ID/Secret to App page. Add some visual separation
- Fix some bugs/warnings

* Update to match code standards

* Trigger notification

* Add warning about not sharing API secrets

* Tweak spec a bit

* Cleanup fixture creation by using let!

* Remove unused key

* Add foreign key for application<->user
06b786b

ykzts added a commit to ykzts/mastodon that referenced this pull request Aug 22, 2017

@ykzts
Add Japanese translations for #2758, #4506, #4521, #4600 and #4664
cc4e385

@ykzts ykzts referenced this pull request Aug 22, 2017

Merged

Add Japanese translations for #2758, #4506, #4521, #4600 and #4664 #4665

Gargron added a commit that referenced this pull request Aug 23, 2017

@ykzts @Gargron
Add Japanese translations for #2758, #4506, #4521, #4600 and #4664 (#… 
…4665)

* Add Japanese translations for #2758, #4506, #4521, #4600 and #4664

* Do not translate Inbox URL and Outbox URL

* Remove "あなたの"

* Remove "あなたの"
8d23667

lindwurm added a commit to lindwurm/mastodon that referenced this pull request Aug 27, 2017

@ykzts @lindwurm
Add Japanese translations for #2758, #4506, #4521, #4600 and #4664 (#… 
…4665)

* Add Japanese translations for #2758, #4506, #4521, #4600 and #4664

* Do not translate Inbox URL and Outbox URL

* Remove "あなたの"

* Remove "あなたの"
406e982

YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017

@muffinista @YaQ00
Application prefs section (#2758) 
* Add code for creating/managing apps to settings section

* Add specs for app changes

* Fix controller spec

* Fix view file I pasted over by mistake

* Add locale strings. Add 'my apps' to nav

* Add Client ID/Secret to App page. Add some visual separation

* Fix rubocop warnings

* Fix embarrassing typo

I lost an `end` statement while fixing a merge conflict.

* Add code for creating/managing apps to settings section

- Add specs for app changes
- Add locale strings. Add 'my apps' to nav
- Add Client ID/Secret to App page. Add some visual separation
- Fix some bugs/warnings

* Update to match code standards

* Trigger notification

* Add warning about not sharing API secrets

* Tweak spec a bit

* Cleanup fixture creation by using let!

* Remove unused key

* Add foreign key for application<->user
d51a77f

YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017

@ykzts @YaQ00
Add Japanese translations for #2758, #4506, #4521, #4600 and #4664 (#… 
…4665)

* Add Japanese translations for #2758, #4506, #4521, #4600 and #4664

* Do not translate Inbox URL and Outbox URL

* Remove "あなたの"

* Remove "あなたの"
6c114be

39e added a commit to Arukas/mastodon that referenced this pull request Sep 8, 2017

@muffinista @39e
Application prefs section (#2758) 
* Add code for creating/managing apps to settings section

* Add specs for app changes

* Fix controller spec

* Fix view file I pasted over by mistake

* Add locale strings. Add 'my apps' to nav

* Add Client ID/Secret to App page. Add some visual separation

* Fix rubocop warnings

* Fix embarrassing typo

I lost an `end` statement while fixing a merge conflict.

* Add code for creating/managing apps to settings section

- Add specs for app changes
- Add locale strings. Add 'my apps' to nav
- Add Client ID/Secret to App page. Add some visual separation
- Fix some bugs/warnings

* Update to match code standards

* Trigger notification

* Add warning about not sharing API secrets

* Tweak spec a bit

* Cleanup fixture creation by using let!

* Remove unused key

* Add foreign key for application<->user
Unverified
This user has not uploaded their public key yet.
GPG key ID: 05D78A80FBBF61D4 Learn about signing commits
7209f1b

39e added a commit to Arukas/mastodon that referenced this pull request Sep 8, 2017

@ykzts @39e
Add Japanese translations for #2758, #4506, #4521, #4600 and #4664 (#… 
…4665)

* Add Japanese translations for #2758, #4506, #4521, #4600 and #4664

* Do not translate Inbox URL and Outbox URL

* Remove "あなたの"

* Remove "あなたの"
Unverified
This user has not uploaded their public key yet.
GPG key ID: 05D78A80FBBF61D4 Learn about signing commits
80d2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment