Adds support for accounts-trello #1227

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@RobertLowe

This is not considered a final patch.

This is a proof-of-concept/rough draft implementation, which attempts to complete a Trello oAuth integration.

I've made comments on the code which are areas of concern.

Let me know what you think.

Cheers,

  • Rob
@RobertLowe RobertLowe commented on the diff Jul 17, 2013
packages/oauth1/oauth1_binding.js
// @param consumerSecret {String} As supplied by the OAuth1 provider
// @param urls {Object}
// - requestToken (String): url
// - authorize (String): url
// - accessToken (String): url
// - authenticate (String): url
-OAuth1Binding = function(consumerKey, consumerSecret, urls) {
- this._consumerKey = consumerKey;
- this._secret = consumerSecret;
+OAuth1Binding = function(config, urls) {
+ this._config = config;
@RobertLowe
RobertLowe Jul 17, 2013

Refactoring to config, so you can access more configurable data in an OAuthBinding

@RobertLowe RobertLowe commented on the diff Jul 17, 2013
packages/oauth1/oauth1_binding.js
var self = this;
+ if (requestTokenSecret)
@RobertLowe
RobertLowe Jul 17, 2013

Trello uses a requestTokenSecret for signing

@RobertLowe RobertLowe commented on the diff Jul 17, 2013
packages/oauth1/oauth1_server.js
@@ -20,10 +21,19 @@ Oauth1._handleRequest = function (service, query, res) {
oauthBinding.prepareRequestToken(query.requestTokenAndRedirect);
// Keep track of request token so we can verify it on the next step
- Oauth1._requestTokens[query.state] = oauthBinding.requestToken;
+ Oauth1._requestTokens[query.state] = {
@RobertLowe
RobertLowe Jul 17, 2013

Keep track of requestTokenSecrets

@RobertLowe RobertLowe commented on the diff Jul 17, 2013
packages/oauth1/oauth1_server.js
// redirect to provider login, which will redirect back to "step 2" below
- var redirectUrl = urls.authenticate + '?oauth_token=' + oauthBinding.requestToken;
+ var redirectUrl = urls.authenticate + '?oauth_token=' + oauthBinding.requestToken
+
+ // this should be handled more magically - needed for trello
@RobertLowe
RobertLowe Jul 17, 2013

This needs discussion, how should scope, name, etc be handled?

@meteor-bot
Collaborator

@RobertLowe: Before we can merge your pull request, you'll need to sign the Meteor Contributor Agreement: https://contribute.meteor.com/

@meteor-bot
Collaborator

@RobertLowe: Before we can merge your pull request, you'll need to sign the Meteor Contributor Agreement: https://contribute.meteor.com/

@RobertLowe

@meteor-bot I think you have a bug, but I signed one. :)

@n1mmy
Member
n1mmy commented Jul 17, 2013

Oops, sorry. We accidentally lost the database of who signed for a few minutes, which caused the bot to post again. You're all signed and good to go.

@RobertLowe

@n1mmy cheers, thanks

@glasser
Member
glasser commented Jul 26, 2013

We haven't been taking pull requests for new accounts integrations; I recommend putting it on Atmosphere (atmosphere.meteor.com).

@glasser glasser closed this Jul 26, 2013
@RobertLowe

@glasser I don't think that's possible because the oauth1 package needs patching. This pull-request was also opened to discuss changes around oauth1 (which from what I can tell was never fully implemented).

EDIT: I'm happy to make any necessary changes to get this merged, let's talk about it!

@glasser
Member
glasser commented Jul 26, 2013

Ah, I missed that.

What I'd recommend is that you make a new PR that just shows the changes to oauth1 (etc), and then include a link to the git repo containing accounts-trello (which shows how the changes get used).

Also, make sure to make the PR against the devel branch, not master (you can't change an existing PR in this way so you'll need a new one).

Also, if you're making backwards-incompatible changes to OAuth1Binding, (a) try to avoid that if possible and (b) you'll need to update other calls to OAuth1Binding in our repo so that they aren't broken (eg in twitter).

@RobertLowe

@glasser Thanks for the prompt feedback.

Great, I'll get started soon.

The oauth1 changes will be backwards-compatible.

I'll prepare an atmosphere package for accounts-trello.

One last item remains: Trello's implementation has an array of parameters for Oauth scope, name, etc. How should these be handled? You have access to the config which feels like the right place to store them, but I'm assuming we don't want implementation specific logic here.

So, should that logic be made generic (config.params?), or should I add a callback for additional authenticate logic?

Glad to have this moving forward, thanks!

@RobertLowe RobertLowe referenced this pull request Jul 27, 2013
estark37 Add server static assets and an API for retrieving them.
Server assets can be included in a bundle by putting them in the private/
directory of an application, or by registering a build plugin that calls
compileStep.addAsset with a server file. The Assets API (Assets.getText and
Assets.getBinary) allows an application or package to retrieve the contents of
its own server assets.
b50a1bf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment