OAuth consumer #324

dsander · 2014-05-14T21:47:21Z

Follow up of #316 /cc @cantino @alias1 @mfclarke

Like I said my approach was to keep it really simple and it worked pretty well for twitter (the code is more of a proof of concept and not polished). The problem however is, that not every provider uses the same authentication keys. Twitter uses a token and secret, GitHub just a token like 37signals does. 37signals also provides a refresh_token which is needed to refresh the oauth token every two weeks.
So basically my "keep it simple and unintrusive" approach failed because we need at least a few different logics to store the authentication information.

Also the idea of @alias1 to have multiple account of the same provider really grew on me. Using a accounts page the user could add accounts to huginn to the services we added, marking it as global would allow every user to use the account in their agents.
I think I would not tie the registration of accounts to agents right now, for example a website agent could also use an OAuth account to access pages.

Todo list for this approach:

create the accounts model
add a services controller and the views to create and manage services
add a services drop down to the agent form and save the association
migrate the existing twitter agents to the new services model
add the ability to refresh tokens to the oauthable concern
write specs for the new controller and concern

0xdevalias · 2014-05-16T12:12:15Z

"marking it as global would allow every user to use the account in their agents"

I think this should be on a per account basis, as I plan to let friends use my instance but obviously don't want to give them access to my accounts.

dsander · 2014-05-16T12:19:51Z

@alias1 yeah, that is what i meant 😃

dsander · 2014-05-25T14:14:27Z

I am thinking about renaming the account model to service, because I think it is a bit confusing with the user credentials and the huginn account page. An other option would be to rename the huginn account link to something like "profile". What is your opinion on this? /cc @alias1 @cantino

0xdevalias · 2014-05-25T14:17:19Z

I kind of like the idea of changin the user account to profile and the other one to service.

Makes it a bit more self explanatory IMO.

cantino · 2014-05-25T20:36:25Z

I agree. Account generally means an 'account on the Huginn instance'. I'd suggest Service or ExternalAccount.

0xdevalias · 2014-05-30T02:10:11Z

How's this coming along?

Is the idea with Oauthable that I can kick of the Oauth dialog from the create new agent page?

(Can't quite remember the specifics, but thinking this will be a good place to start with implementing #344 or anything like that)

dsander · 2014-05-30T14:57:12Z

@alias1 I think the code is now pretty much done. What is still missing is some documentation on how to set it up properly (creating twitter/37signals apps and putting the token/keys in .env)
Oauthable is just adding a Service to an Agent (adds a dropdown menu to the agent form and validates that a service is present)
The new ServiceController is used to authenticate with external services and manage them. At the moment you can just mark them is global (so every user of the huginn installation can use the service) and delete them.
I will squash my commits and set up a huginn instance, so you guys can play with it, later tonight or sometime tomorrow.

cantino · 2014-05-31T05:57:52Z

Awesome @dsander, I'll try to play with it this weekend!

0xdevalias · 2014-05-31T06:54:19Z

Awesome! Will definitely have a play when it's up/when I get time to pull it :)—
Sent from Mailbox

On Sat, May 31, 2014 at 12:57 AM, Dominik Sander notifications@github.com
wrote:

@alias1 I think the code is now pretty much done. What is still missing is some documentation on how to set it up properly (creating twitter/37signals apps and putting the token/keys in .env)
Oauthable is just adding a Service to an Agent (adds a dropdown menu to the agent form and validates that a service is present)
The new ServiceController is used to authenticate with external services and manage them. At the moment you can just mark them is global (so every user of the huginn installation can use the service) and delete them.

I will squash my commits and set up a huginn instance, so you guys can play with it, later tonight or sometime tomorrow.

Reply to this email directly or view it on GitHub:
#324 (comment)

0xdevalias · 2014-06-01T13:31:16Z

Been having a little play with this on dev instance and so far I am REALLY liking how it is hanging together!

Doesn't have to be a part of this PR, but at some point I think a 'prettier' (eg. not plaintext) interface for the connect links at /services would be cool (maybe just each services oauth connect type button, or 'tiles' like IFTTT has?)

dsander · 2014-06-01T13:37:31Z

Yeah it certainly isn't pretty at the moment :)
The error you were getting, did it work the second time or is it persisting? I had some similar strange errors locally, which went away after waiting for a few minutes and was really happy it worked without any issues for me on the production instance.

0xdevalias · 2014-06-01T13:55:52Z

I clicked through it a second time and seemed to work automagically, so not too sure of the reasoning why it failed the first time. (link mentions CSRF: /auth/failure?message=csrf_detected&strategy=github)

dsander · 2014-06-01T16:02:53Z

I got that one too once, but all information I found about it was that the callback url might be wrong which, obviously, is not the case. Maybe one of the twitter servers is not up to date with the current information.

cantino · 2014-06-11T04:08:50Z

app/concerns/twitter_concern.rb

  end

  def twitter_consumer_secret
-    options['consumer_secret'].presence || credential('twitter_consumer_secret')


I guess one thing I'm unsure about is whether Huginn users will ever want differed users to have different API applications. With a credential, each user needs a Twitter application, but they can also have multiple ones. With this new changes, the whole huginn installation has one application, right? Is there anything we lose by going this direction?

You are right, I do not think omniauth has a way to support multiple applications for one provider. I think the only drawback this solution has is that you can not have one read-only twitter application and one write enabled. But if a user does not trust the huginn admin with write access he could always create a second fake twitter account.
The same probably works for every other service.

Perhaps just update

errors.add(:base, "Twitter consumer_key, consumer_secret, oauth_token, and oauth_token_secret are required to authenticate with the Twitter API. You can provide these as options to this Agent, or as Credentials with the same names, but starting with 'twitter_'.")

to also talk about the .env?

I am not sure if the validation is even valid anymore. An twitter agent can not be created without a service present, and to create a service you need to set up the ENV variables. I think that validation can be removed once we are sure the migration works as expected.

Do we need to remove the validation, or have it talk about the ENV?

cantino · 2014-06-11T04:15:00Z

What's the upgrade path for existing users?

dsander · 2014-06-11T19:26:35Z

I covered the upgrade for twitter and basecamp agents (the only two oauth enabled agents we have at the moment, afaik). The twitter upgrade works almost automatically, the you just have to copy and paste the OAUTH_KEY and OAUTH_SECRET the migration prints to your .env file and you are good.
Because basecamp was not using oauth previously it needs to be set up manually, I hope the wiki page explains the steps clearly enough.

Created a service model which stores external account information. Created a service controller which kicks off the oauth process and lets the user manage their external accounts. Oauthable concern connects a agent to a service and adds a drop down to the agents edit page. Added a migration which converts the existing twitter agents to use the new service model.

dsander · 2014-08-18T22:14:57Z

^^ Sorry, hoping we are merging this relatively soon I was too lazy to create a PR from master and dealing with the possible merge conflicts.

cantino · 2014-08-18T22:38:54Z

.travis.yml

@@ -2,7 +2,7 @@ language: ruby
 cache: bundler
 bundler_args: --without development production
 env:
-  - APP_SECRET_TOKEN=b2724973fd81c2f4ac0f92ac48eb3f0152c4a11824c122bcf783419a4c51d8b9bba81c8ba6a66c7de599677c7f486242cf819775c433908e77c739c5c8ae118d
+  - APP_SECRET_TOKEN=b2724973fd81c2f4ac0f92ac48eb3f0152c4a11824c122bcf783419a4c51d8b9bba81c8ba6a66c7de599677c7f486242cf819775c433908e77c739c5c8ae118d TWITTER_OAUTH_KEY=twitteroauthkey TWITTER_OAUTH_SECRET=twitteroauthsecret


Can we drop this now that we added env.test?

cantino · 2014-08-18T22:39:50Z

I think we're super close!

My only remaining concern is with the migration. Could you detail what the experience will be when someone tries to upgrade to this version and deploy, if they have existing Basecamp agents? Twitter agents?

cantino · 2014-08-20T04:15:34Z

So, I'm running this on my instance now!

First of all: this is AWESOME! It's a really significant improvement to how Huginn will work.

A few comments:

When I deployed, it migrated perfectly. Nice!
I had to remember to look for the update message:

 ** [out :: example.com] Your Twitter agents were successfully migrated. You need to update your .env file and add the following two lines:
 ** [out :: example.com] 
 ** [out :: example.com] TWITTER_OAUTH_KEY=sdfjkhskjdfhksjhdfkhsdf
 ** [out :: example.com] TWITTER_OAUTH_SECRET=dskfjhsdkfjhskdjfhkjsdf

Would you consider adding an .alert on the Huginn front page that shows up when the user has one or more TwitterAgents (current_user.agents.where("agents.type like 'twitter_%'").exists?), but ENV['TWITTER_OAUTH_KEY'] or ENV['TWITTER_OAUTH_SECRET'] are not set? We could also do this with the Basecamp agent. It should tell them to edit .env and then restart Huginn, and remind them to delete any Credentials that are no longer needed. Maybe this "configuration alert" could be something we reuse for future upgrades?

The migration created 4 services, when it feels like it should have created 1 (since they all used the same Twitter Credential):
Not a huge deal, but might be easy to fix.
When I export a Scenario, then import it again, the Services menu doesn't seem to remember the service currently selected for the Agent.
I'd love someone else who's using Twitter / Basecamp Agents to try this branch as well. @alias1?

knu · 2014-08-20T05:54:42Z

app/views/services/index.html.erb

+      </p>
+      <p><%= link_to "Authenticate with Twitter", "/auth/twitter" %></p>
+      <p><%= link_to "Authenticate with 37Signals (Basecamp)", "/auth/37signals" %></p>
+      <p><%= link_to "Authenticate with Github", "/auth/github" %></p>


Maybe each of these links should be hidden or disabled if the corresponding OAuth key/secret are not set.

@knu

Maybe each of these links should be hidden or disabled if the corresponding OAuth key/secret are not set.

Good idea, will try to add it tomorrow

dsander · 2014-08-20T21:48:39Z

@cantino

Could you detail what the experience will be when someone tries to upgrade to this version and deploy, if they have existing Basecamp agents? Twitter agents?

Sorry, I was quite busy the last days, but also happy you were keen eager to try it out :)

You already know about the experience with twitter agents. For Basecamp agents you will just get the message to go to the wiki for information on how to set up an OAuth application and update the .env. Previously the Basecamp agent used the username and password for authentication, so we do not have any OAuth information we can use.

Maybe this "configuration alert" could be something we reuse for future upgrades?

You are right, if someone has to run many migrations it is really hard to spot the information. However adding that check on every page request doesn't feel right. At one point we might need to do a few of those upgrade checks which might hurt the performance and add legacy code which we never really could remove. (the old twitter_consumer_key and twitter_consumer_secret methods are removed and we would need to store the key and secret somewhere)

In my opinion we should keep the upgrade information somewhere near the upgrade process, i.e. you are running your cap deploy or heroku push and get the information about the additional steps you have to take. A solution would be a UPGRADE.md like GitLabs, but since we do not versioning yet this really isn't a possibility. Another option would be just to sleep for x seconds while migrating. Is possible to wait for the user to hit before continuing with the migration?

The migration created 4 services, when it feels like it should have created 1 (since they all used the same Twitter Credential):

I guess it would be possible to check which agents use the same keys/tokens, but since I did not know every possible use case and combination I went the lazy way by cloning the old methods and use their responses. Since we just can support one twitter application after all, how about to just use the first key/token combination for all twitter agents?

When I export a Scenario, then import it again, the Services menu doesn't seem to remember the service currently selected for the Agent.

The service doesn't get exported with the scenario. Even if we would export the service, I am not sure of how much use it would be. If you use the export function as a backup for your agents we could use the services uid to identify the service when importing, but if you share the scenario with somebody else we could never match it to a specific service (besides just showing the "matching" services which is already done).

cantino · 2014-08-21T03:21:59Z

@dsander

Sorry, I was quite busy the last days, but also happy you were keen eager to try it out :)

No worries! This is a big effort.

You are right, if someone has to run many migrations it is really hard to spot the information. However adding that check on every page request doesn't feel right. At one point we might need to do a few of those upgrade checks which might hurt the performance and add legacy code which we never really could remove. (the old twitter_consumer_key and twitter_consumer_secret methods are removed and we would need to store the key and secret somewhere)

How about a compromise? For now, we do two things:

Add sleep 20 after the migration if it prints a message.
Only on HomeController#index, add a message shown when current_user.agents.where("agents.type like 'twitter_%'").exists?), but ENV['TWITTER_OAUTH_KEY'] or ENV['TWITTER_OAUTH_SECRET'] are not set. Then it's not every request, and we can remove it from the codebase in a month or two when most people have hopefully upgraded. What do you think?

In my opinion we should keep the upgrade information somewhere near the upgrade process, i.e. you are running your cap deploy or heroku push and get the information about the additional steps you have to take. A solution would be a UPGRADE.md like GitLabs, but since we do not versioning yet this really isn't a possibility.

Hopefully we get this soon-- I'm very interested in ideas about how to do versioning.

Since we just can support one twitter application after all, how about to just use the first key/token combination for all twitter agents?

They could have been using different authorized twitter users (access_tokens) though, so let's just leave it how you have it now.

The service doesn't get exported with the scenario.

Sorry, I meant that when you import an already-imported Scenario (or export and then re-import a Scenario), it shows you the Services menu, but doesn't pre-select the Service that you've already associated with the Agent that you're updating. I think the select box just isn't defaulting to that Agent's service_id, if one is set and the Agent already exists.

Conflicts: Gemfile.lock

Dotenv is loaded after the exception about the missing token is thrown

coveralls · 2014-08-21T21:51:53Z

Coverage increased (+0.05%) when pulling 508d493 on dsander:omniauth into 0f1b30b on cantino:master.

Conflicts: db/schema.rb

scenario

coveralls · 2014-08-22T19:54:29Z

Coverage increased (+0.33%) when pulling 6b4ca63 on dsander:omniauth into 2870750 on cantino:master.

cantino · 2014-08-24T18:35:46Z

Phew, this is so close. This is the last thing, I promise!

I had to log into https://apps.twitter.com/ and set my correct callback URL, which wasn't needed before. Our current instructions say you don't need it, which was previously true. I think it'd be worth noting this in the migration / warning message. I know you do mention it on the new wiki page.

Also, as well as being on the home page, could we show your upgrade warning on the Services page as well?

And that's it! Do those, and go ahead and merge! 😄

coveralls · 2014-08-26T20:52:07Z

Coverage increased (+0.2%) when pulling a144c2d on dsander:omniauth into 2870750 on cantino:master.

OAuth consumer

dsander · 2014-08-26T21:20:49Z

now we wait for the explosions 😄

So happy to have it merged. @cantino thanks for the countless reviews and feedback!

cantino · 2014-08-26T23:09:41Z

Awesome work on this effort!

0xdevalias · 2014-08-26T23:21:02Z

Yay! Excited! Well done :3—
Sent from Mailbox

On Wed, Aug 27, 2014 at 9:09 AM, Andrew Cantino notifications@github.com
wrote:

Awesome work on this effort!

Reply to this email directly or view it on GitHub:
#324 (comment)

knu · 2014-08-27T00:05:21Z

Great!

knu · 2014-08-27T06:33:49Z

app/views/services/index.html.erb

+        <table class='table table-striped events'>
+          <tr>
+            <th>Provider</th>
+            <th>Username</th>


@dsander Shouldn't this be "Name" instead of Username?

I guess it would also be "Name", I was going with "Username" because in most cases we will not get a "realname" but a account/nickname.

OK, I just felt "usernames" set by the migration looked weird as a username.

I made a "Refresh username" button to "fix" them, but I'm not sure if it's worth it: https://github.com/knu/huginn/commit/9f466599afea308030a82ee5fdf89c0780c544f0

Ah, right totally forgot the migrated names. I think it would be nice get rid of those strange names, but also unsure if it is worth adding the code since the only "real" use case are the migrated twitter accounts. @cantino what do you think?

Eh, I don't have a strong opinion on this one. I'd be okay leaving them, since they're one-time. OTOH, if the fix is easy, feel free.

dsander added enhancement labels May 14, 2014

dsander changed the title ~~OAuth consumer~~ OAuth consumer [work in progress] May 15, 2014

dsander changed the title ~~OAuth consumer [work in progress]~~ OAuth consumer May 31, 2014

dsander added in-development and removed discussion labels May 31, 2014

This was referenced May 31, 2014

API key management #76

Closed

User properties and substitution variables in Agents #136

Closed

Oauth for users (not for developers) #176

Closed

Toilal mentioned this pull request Jun 1, 2014

TrelloWebhookAgent (Initialization and Shutdown entry points) #346

Closed

0xdevalias mentioned this pull request Jun 1, 2014

Real RSS Agent #11

Closed

cantino reviewed Jun 11, 2014
View reviewed changes

cantino reviewed Aug 18, 2014
View reviewed changes

knu reviewed Aug 20, 2014
View reviewed changes

dsander added 2 commits August 20, 2014 22:21

Removed env variables from .travis.yml, we now use env.test

182a59f

Added a link to the oauth application wiki entry

6438d73

dsander added 4 commits August 21, 2014 21:58

Improved the migration experience

dad2875

Hide oauth providers which are not configured

fe192e8

Merge branch 'master' into omniauth

00663c1

Conflicts: Gemfile.lock

We still need the APP_SECRET_TOKEN in .travis.yml

508d493

Dotenv is loaded after the exception about the missing token is thrown

dsander added 2 commits August 22, 2014 20:35

Merge branch 'master' into omniauth

858dcdc

Conflicts: db/schema.rb

Select the current service if an agent already exists when importing a

6b4ca63

scenario

Also show the upgrade warning in the services page

a144c2d

dsander added a commit that referenced this pull request Aug 26, 2014

Merge pull request #324 from dsander/omniauth

74ced66

OAuth consumer

dsander merged commit 74ced66 into huginn:master Aug 26, 2014

dsander deleted the omniauth branch August 26, 2014 23:14

knu reviewed Aug 27, 2014
View reviewed changes

OAuth consumer #324

OAuth consumer #324

Conversation

dsander commented May 14, 2014

0xdevalias commented May 16, 2014

dsander commented May 16, 2014

dsander commented May 25, 2014

0xdevalias commented May 25, 2014

cantino commented May 25, 2014

0xdevalias commented May 30, 2014

dsander commented May 30, 2014

cantino commented May 31, 2014

0xdevalias commented May 31, 2014

I will squash my commits and set up a huginn instance, so you guys can play with it, later tonight or sometime tomorrow.

0xdevalias commented Jun 1, 2014

dsander commented Jun 1, 2014

0xdevalias commented Jun 1, 2014

dsander commented Jun 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cantino commented Jun 11, 2014

dsander commented Jun 11, 2014

dsander commented Aug 18, 2014

Choose a reason for hiding this comment

cantino commented Aug 18, 2014

cantino commented Aug 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsander commented Aug 20, 2014

cantino commented Aug 21, 2014

coveralls commented Aug 21, 2014

coveralls commented Aug 22, 2014

cantino commented Aug 24, 2014

coveralls commented Aug 26, 2014

dsander commented Aug 26, 2014

cantino commented Aug 26, 2014

0xdevalias commented Aug 26, 2014

Awesome work on this effort!

knu commented Aug 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment