Adding additional Omniauth strategies via plugin?

(Mike Murray) #1

There have been several suggestions for additional Omniauth strategies to be added, including one discussion that touched on building a custom strategy for integration with some parent site.

Given the HUGE number of Omniauth strategies available, to continue like this could quickly get out of hand, not to mention the additional burden on the site settings page. I was wondering about the viability of installing strategies as plugins? This would give users the freedom to pick and choose whichever, or even drop in their own, with minimal effort.

If this is a good idea for the future, then would it also be better to provide the default strategies as plugins? Allowing the end user (Admin) to remove any unwanted code, whilst freeing up their settings interface from unwanted fields.

Giving Microsoft some love with omniauth
(Jeff Atwood) #2

I like the idea of optional auth as plugin, but we are absolutely editorializing with the providers we have… if I go to your Discourse site and you have not “enabled” the Google / Gmail login, I’m going to be pissed – as would millions and millions of other GMail users. :smile:

But for the lesser and more obscure login providers, sure, agreed.

(Mike Murray) #3

I can’t argue with that. In fact I’m not advocating shipping without default providers at all.
My thinking came about when considering how to implement Auth as plugin. It made sense to me to implement all OmniAuth strategies in the same way, as opposed to one method for implementing default strategies, and another for lesser optional strategies (via plugin).

Given Discourse’s position on the default providers, what I would advocate is still providing all strategies as plugins for the sake of simplicity (one implementation, one set of tests), but simply locking the defaults (which would be pre-installed) so they can’t be removed. I would even go as far as to give admins a popup when they disable one of the default providers, encouraging them not to, for the reason you gave above (although I’d probably word it a little differently :wink:).

This approach should also make it simpler to provide a single tab/area in the settings to manage auth keys/secrets and add/remove additional auth strategies and their settings.

Anyway, this is purely conjecture. I should really wait until the plugin interface has matured a little and try this approach out for myself before advocating it to anyone. :blush:

(Sam Saffron) #4

I just noticed this pull request

^^ what no onebox

A site setting for linkedin auth. Face value it seems quite innocuous, not a big deal just a few additional files, but I am starting to worry quite a bit for quite a few reasons.

  • The strategies are all being loaded by default, even if you do not use them, this means that people who do not use these options still have objects in memory that get crawled during the GC, Gemfile is bigger and so on. (can be improved with a :require => false
gem "omniauth-github"
gem 'omniauth-linkedin'
  • There are a TON of strategies out there. If we accept PRs for every strategy under the sun, our core just gets way too big and full of conditional on top of conditional with piles of client settings.

What I think we should do?

  • Provide a clear interface for adding auth plugins.
  • ONLY include in core strategies that are on by default, meaning persona and github get spun out to plugins

We have started down a rabbit hole here I am not that comfortable with and Persona and Github paved the way.

cc @eviltrout

(ahadsh) #5

whats onebox?

(Sam Saffron) #6


just created it :smile:

(Robin Ward) #7

I think we should base our decisions for such things on evidence, so I profiled memory usage with 35 additional omniauth providers: additional_providers.rb · GitHub - I chose all the names I’d heard of that actually worked. A few seemed broken.

This is on ruby 1.9.3p327, running on my OSX machine (64-bit) in production mode. I ran apachebench for 5 minutes on each to make sure they were fully cycled.

  • With our current 5 strategies, OSX Real mem: 230MB
  • With 40 omntauth strategies: OSX Real mem: 240MB

So we are really talking about a 4% difference in memory usage, assuming we’d include all 35 strategies I chose, which I doubt we ever would.

I discussed this recently with @codinghorror. Our original idea was to include a top 4 or so that would represent the most popular providers. Then there would be a “More…” link that would spill out all the additional ones. Note I am not suggesting we add all 35 here, probably an additional 10 would suffice to cross off those even remotely useful to users.

Having said that I do think it would be useful to figure out a clean way to nicely conditionally include gems in our app. Some are only required when a site setting is present for example. We also have the problem that right now installing a plugin means changing your Gemfile, which is locked to our tested versions of stuff.

I wonder if there’s a good solution to having our core Gemfile for discourse as a base, and a secondary one users can edit or something like that to configure the plugins they’d like? It would be even more useful if they could add/remove from it via our admin UI.

(Sam Saffron) #8

GCs in prd take 40ms, adding another 4% there and turning it into 42ms for absolutely no gain makes no sense. Further more VPSes run tight. Anyway, resolving the memory issue is trivial, require => nil in the Gemfile, spin up the ones you need in the initializer.

The trickier thing is that the code is getting unwieldy. It makes much more sense to keep these puppies in plugins, it allows you to update and extend auth methods without touching core or waiting for core releases. It helps define a proper interface for auth so others can use whatever weird and wonderful auth method they want.

(Robin Ward) #9

There are a couple of flaws here – first I pointed out the 4% came from adding 35 providers and I said we’d only reasonably want 10. So then we’d be down to 1.14% more memory per process.

Secondly, it’s making an assumption that just because RAM grows the speed of GC grows. It follows that if you’re using more RAM than it has more to clean up, but if they are largely resident code paths (and not extra allocations per request) that it wouldn’t increase the GC speed at all. I think we’d have to prove this is the case before taking it seriously.

It’s not trivial when the SiteSettings have to work though. They require the app to be initialized before they are read. Right now we have no easy way to conditionally include code that is enabled/disabled via a site setting.

I think including 10 will cross off all the major ones (and then some) and won’t affect our performance at all, while providing a nice way to making logging in more accessible. But if @codinghorror’s gone sour on the idea I’m happy to give in on it.

(David Justice) #10

I’d like to throw a different perspective out there regarding this conversation. So far, the discussion seems to have focused on the performance impact of additional omniauth providers. While this is valid, it may end up being slightly less impactful than the code changes that need to be added to the core project to implement a new provider (as it stands currently).

I just implemented a new provider for cisimple for our “meta.cisimple” (yeah… it was not incredibly creative). Anyway, it included several code changes that probably should be insulated within the core framework.

  1. Shouldn’t have to change / add onto core methods in Discourse. For example: Users::OmniauthCallbacksController is going to need a create_or_sign_on_user_using_‘provider’ method within core.

  2. Shouldn’t have to add a new model for each provider. For example: TwitterUserInfo model which holds the twitter_user_id and other Twitter meta.

  3. Shouldn’t have to add SiteSetting specifically for the provider (enabled, client_secret, client_id). (edit: Not to say that setting shouldn’t be available, they should just be part of the provider plugin architecture, rather than one off SiteSettings)

From a third party perspective, it would be really nice to add a gem that fulfills an interface for those interaction points. It would also be nice to possibly create a OAuth meta model in core that consists of provider, uid, and hstore for provider metadata, so there will not be a bazillion models and a bazillion providers.

I haven’t dug really deep into the Discourse source, so I may not understand the full story. Please be gentle if I’m off base.


(Jeff Atwood) #11

I think the answer here is both.

As @eviltrout said, we should include the “top 10” login strategies that people are likely to want. You can see from recent Stack Exchange data that beyond local logins, Google, Facebook (and probably Twitter), most everything else is a tiny percentage of the total. The top 10 should easily cover the vast majority of logins people would want to use.

As @sam noted, we also need a sane way to plug in custom authentication, not so much for adding things like a StumbleUpon login or whatever, but for people’s in-house and homebrewed and custom logins for systems they already use. They need a way to plug in here that is logical and doesn’t require touching 10 different files in source code plus adding custom admin settings…

(mojzis) #12

i think that with openID strategy already in, you could have many people with custom auth needs covered only by providing a setting that would allow to set an address other than google.

(dougalcorn) #13

This doesn’t seem to be resolved? Did you guys ever come to some conclusion?

(adam8797) #14

Well for now, it appears that if you’re really in a pinch and need other methods of authentication, messing around with the omniauth.rb file in /config/initializers/ may open the door… I haven’t messed with it yet (and I will), but I’m just sharing for now.

(Sam Saffron) #15

Working on adding a clean mechanism for custom auth this week.

(dougalcorn) #16

Good news, @sam. Let me know if I can help. @ven88 is pretty interested in having linkedin authentication. If you get the custom auth working, he and I could probably test it via linkedin

(Sam Saffron) #17

no problems, btw for linkedin we are probably going to at least need to ship the gem dependency in our Gemfile with require false for it all to hang correctly, secondary gem files is a very hard problem I would like to sidestep for now.

(Denis Peplin) #18

Authentication as the plugin will really help for some Discourse installations. For example, I have in mind two installations, one for the local cyclists community in my city, and one for discussing some things on my job. So, for the first installation I will need to add Vkontakte (most popular social network in Russia) authentication, and for the second - custom Doorkeeper-based authentication.
I’m guessing that these methods do not have a chance to be in the Discourse source code base, so without plugin there will be need to maintain patches for them, resolve conflicts when updating, etc., which is obviously a painful way.

(Sam Saffron) #19

Be sure to read through:

(Michael John Kirk) #20

After having read through this thread, the new plugin interface thread, and this commit, my impression is that plugins can only provide additional authentication methods so long as the type of authentication is supported in core (i.e. one of these authentication types).

Is that right, @sam?

Just like @denis_peplin, I want to implement an internal forum which uses another rails app as the authentication provider (probably using oauth via doorkeeper). It looks like at this point, I’ll still have to have to hack omniauth_callbacks_controller with something like:

def create_or_sign_on_using_my_custom_oauth_method(auth_token)
    user_info = # new Model!

Does that sound right, given the current state of things?