Application prefs section by muffinista · Pull Request #2758 · mastodon/mastodon

muffinista · 2017-05-03T16:41:04Z

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.

beatrix-bitrot · 2017-05-07T22:29:42Z

  # 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


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

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

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

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

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

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

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.

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

beatrix-bitrot · 2017-05-10T03:11:00Z

  # 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


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

muffinista · 2017-05-10T21:01:35Z

OK, conflicts should be resolved, thanks!

beatrix-bitrot

the CI checks are failing ):

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

muffinista · 2017-05-10T22:43:24Z

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

beatrix-bitrot · 2017-05-11T00:16:25Z

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 · 2017-05-11T03:43:55Z

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

Here's the app list:

Here's the Create New App page:

The app page, with tokens:

- 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

…pplication-prefs-section

akihikodaki

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 · 2017-06-19T02:46:21Z

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

akihikodaki

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.

akihikodaki · 2017-06-19T02:35:14Z

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


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

akihikodaki · 2017-06-19T02:39:08Z

+  end
+
+  describe 'POST #create' do
+    describe 'success' do


Test if it actually creates an application.

Use context instead of describe.

akihikodaki · 2017-06-19T02:45:15Z

+  end
+
+  describe 'PUT #update' do
+    describe 'success' do


Test if it actually updates an application.

Use context instead of describe.

akihikodaki · 2017-06-19T02:45:37Z

+    end
+  end
+
+  describe 'PUT #update' do


It is not PUT but PATCH.

akihikodaki · 2017-06-19T02:47:58Z

+    end
+
+    it 'should create new token' do
+      expect( user.token_for_app(app) ).to_not eql(token)


Do not add spaces in parentheses.

akihikodaki · 2017-06-19T02:49:06Z

+    end
+
+    it 'is nil if user does not own app' do
+      app.owner = nil


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

akihikodaki · 2017-06-19T02:58:25Z

  # 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


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

…todon into application-prefs-section

muffinista · 2017-06-26T18:54:51Z

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

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

akihikodaki · 2017-06-27T03:07:20Z

+    end
+
+    it 'returns 404 if you dont own app' do
+      app.owner = nil


Use update! here, too.

akihikodaki · 2017-06-27T03:07:51Z

+
+  describe 'GET #index' do
+    let(:other_app) { Fabricate(:application) }
+    before {


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

akihikodaki · 2017-06-27T03:17:05Z

+    end
+
+    it 'removes the app' do
+      expect(Doorkeeper::Application.find_by(id: app.id)).to be nil


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 · 2017-08-16T17:05:33Z

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

requested changes were addressed

beatrix-bitrot · 2017-08-16T17:45:32Z

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

Gargron · 2017-08-21T20:12:01Z

+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


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.

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.

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

* 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

…4665) * Add Japanese translations for #2758, #4506, #4521, #4600 and #4664 * Do not translate Inbox URL and Outbox URL * Remove "あなたの" * Remove "あなたの"

…4521, mastodon#4600 and mastodon#4664 (mastodon#4665) * Add Japanese translations for mastodon#2758, mastodon#4506, mastodon#4521, mastodon#4600 and mastodon#4664 * Do not translate Inbox URL and Outbox URL * Remove "あなたの" * Remove "あなたの"

…upstream Merge upstream changes up to 2c7eed1

muffinista added 7 commits May 3, 2017 11:17

Add code for creating/managing apps to settings section

777fa04

Add specs for app changes

b7d9abe

Fix controller spec

c492d4d

Fix view file I pasted over by mistake

872f84b

Add locale strings. Add 'my apps' to nav

5f48b41

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

613f76c

Fix rubocop warnings

a089078

beatrix-bitrot reviewed May 7, 2017

View reviewed changes

beatrix-bitrot suggested changes May 10, 2017

View reviewed changes

Merge branch 'master' into application-prefs-section

e14679e

beatrix-bitrot approved these changes May 10, 2017

View reviewed changes

beatrix-bitrot previously requested changes May 10, 2017

View reviewed changes

Fix embarrassing typo

a4bacbd

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

muffinista and others added 6 commits May 23, 2017 13:38

Merge branch 'master' into application-prefs-section

02ea5a8

Merge branch 'master' into application-prefs-section

e875319

Merge branch 'master' into application-prefs-section

575181b

Add code for creating/managing apps to settings section

f3bd8cd

- 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

Merge v1.4.1 into application-prefs-section

691f888

Merge branch 'master' into application-prefs-section

1bdca00

akihikodaki added the enhancement label Jun 3, 2017

Merge branch 'master' of https://github.com/tootsuite/mastodon into a…

abf9710

…pplication-prefs-section

beatrix-bitrot requested a review from akihikodaki June 18, 2017 15:07

akihikodaki suggested changes Jun 19, 2017

View reviewed changes

akihikodaki added the security Security issues and fixes, vulnerabilities label Jun 19, 2017

muffinista and others added 5 commits June 26, 2017 11:02

Update to match code standards

32e1ca6

Trigger notification

e0a9914

Merge branch 'master' into application-prefs-section

0e09da1

Add warning about not sharing API secrets

112bea3

Merge branch 'application-prefs-section' of github.com:muffinista/mas…

fdbee78

…todon into application-prefs-section

akihikodaki suggested changes Jun 27, 2017

View reviewed changes

muffinista added 2 commits June 27, 2017 07:56

Tweak spec a bit

5fecc29

Cleanup fixture creation by using let!

83d69ec

akihikodaki approved these changes Jun 27, 2017

View reviewed changes

muffinista added 2 commits July 28, 2017 14:24

Merge branch 'master' into application-prefs-section

e43568c

Remove unused key

9af51f8

Gargron requested changes Aug 21, 2017

View reviewed changes

Add foreign key for application<->user

d7a4ab4

Gargron approved these changes Aug 22, 2017

View reviewed changes

Gargron merged commit 871c0d2 into mastodon:master Aug 22, 2017

ykzts mentioned this pull request Aug 22, 2017

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

Merged

tribela pushed a commit to tribela/mastodon that referenced this pull request Jun 26, 2024

Merge pull request mastodon#2758 from ClearlyClaire/glitch-soc/merge-…

1eb69c5

…upstream Merge upstream changes up to 2c7eed1

Uh oh!

Conversation

muffinista commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muffinista commented May 10, 2017

Uh oh!

beatrix-bitrot left a comment

Choose a reason for hiding this comment

Uh oh!

muffinista commented May 10, 2017

Uh oh!

beatrix-bitrot commented May 11, 2017

Uh oh!

muffinista commented May 11, 2017

Uh oh!

akihikodaki left a comment

Choose a reason for hiding this comment

Uh oh!

nightpool commented Jun 19, 2017

Uh oh!

akihikodaki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muffinista commented Jun 26, 2017

Uh oh!

akihikodaki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muffinista commented Aug 16, 2017

Uh oh!

beatrix-bitrot commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muffinista Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muffinista Aug 22, 2017 •

edited

Loading