Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reader module for Strategies configurations #51

Closed
yordis opened this issue Mar 18, 2017 · 15 comments
Closed

Add reader module for Strategies configurations #51

yordis opened this issue Mar 18, 2017 · 15 comments

Comments

@yordis
Copy link
Member

yordis commented Mar 18, 2017

Add the ability to change the way we read the configuration of the strategies. Right now we have to reload or coding the configuration (See https://github.com/ueberauth/ueberauth/blob/master/lib/ueberauth.ex#L175)

Use Case

Services like Firebase or Auth0 allow you to configure the social media integration on their platform. They save the configuration in the database which allow us to easy change the configuration.

Examples:
screen shot 2017-03-17 at 11 02 31 pm
screen shot 2017-03-17 at 11 03 08 pm

Thought

We could add a configuration, like

config :ueberauth, Ueberauth,
  config_reader: MyApp.Ueberauth.Reader
....

That allow use to load the configuration of the provider. This module could be using Ecto or whatever we want to load that configuration.

The Reader module should follow a protocol in how to send the data back for sure (I need your help in how to think a little bit deeper on this)

cc: @doomspork

@scrogson
Copy link
Member

@yordis sorry, I'm not following the use-case you've outlined. Could you expand on it?

@yordis
Copy link
Member Author

yordis commented Mar 18, 2017

@scrogson Right now we have to put, for example client_id and client_secret, the configurations in the config.exs so it's hard coded using the language.

The idea is that you could have different ways to load the configuration for the strategies. I could use Ecto (meaning database more than actually Ecto) for load the configuration data from a database.

This allow us to even have multiple configurations for the strategies if we want to actually base the configuration per plug config.

@doomspork
Copy link
Member

@scrogson after discussing this with @yordis on Slack, this doesn't sound like a bad idea so long as we provide a sane default. In his case they'd like to load their third-party credentials from a database and not config/*.exs or system env.

@hassox
Copy link
Member

hassox commented May 24, 2017

When we're talking about credentials, are we talking about ueberauth or particular strategies? The solution will likely be different

@yordis
Copy link
Member Author

yordis commented May 24, 2017

@hassox the idea is to be able to configure the strategies from the database.

For example: Facebook strategy, needs client_id, client_secret, default_scope, display, profile_fields; which it can be configure from some database or any module that you want to implement.

Of course, we need to agree in some interface for the module, but that's the idea

@arjan
Copy link

arjan commented Mar 20, 2018

Is there any progress on this? If not, I might work on this, as we need this in the near future.

@yordis
Copy link
Member Author

yordis commented Apr 25, 2018

@arjan do you have any thing in mind on how to start with this?

@scrogson and I talked about it but we didn't settle on anything.

There is some thoughts around this like:

Configuration based on some parameter: this parameter could come from some HTTP request or you calling directly the Ueberauth workflow with some key I am guessing. We need to think about it.

@arjan
Copy link

arjan commented Apr 26, 2018

So, basically we would like all configuration related to a strategy to be dynamically retrieved, right?

Maybe we can create a config setting:

config :ueberauth, Ueberauth, strategy_reader: {ReaderModule, :read_config, []}

Which specifies a MFA tuple for the reader function. The strategy reader would be called every time a strategy configuration needs to be retrieved (using Kernel.apply/3, with as argument the strategy module, the %Plug.Conn{} of the current request, plus the extra "static" arguments given in the MFA tuple)

The default implementation would be something like:

defmodule ReaderModule do
  def read_config(strategy_mod, _conn), do: Application.fetch_env!(:ueberauth, strategy_mod)
end

Where strategy_mod is something like Ueberauth.Strategy.Facebook.OAuth.

Based on the conn, it is possible to fetch different configuration variations, for example by using the conn.host header. Or by retrieving something from the session.

@yordis
Copy link
Member Author

yordis commented Apr 26, 2018

@arjan yes and no,

That could be a possibility but we are trying to move away from Plug.Conn as a direct requirement, in my case, I use GraphQL API with Absinthe so I don't have access to Plug.Conn or in the other hand my architecture do not depends of Plug.Conn so we need to find a way to remove.

Also, related to that, I would pass some parameter that helps the reader to know which strategy is because.

What if you have multiple strategies fro Facebook? How do you know which one you refer to if you don't pass an extra information?

@arjan
Copy link

arjan commented Apr 26, 2018

Agreed. Did not know about the moving away from Plug part (I saw it listed in the dependencies that's why
my proposal). Anyhow, of course you can always (mis)use the process dictionary for this... e.g. an Absinthe middleware putting some identifier in, and the strategy reader function using that info.

Absinthe itself uses the pdict also quite heavily btw ;-)

Alternatively we can create some initialization function which you need to call on each request before the ueberauth magic happens. It returns some {:ok, value} tuple and this value is passed in to the reader function by Ueberauth. This init function then needs to be called from a Plug or Absinthe middleware.

@rmoorman
Copy link

Also keep in mind that (based on e.g. request information) one request would only be allowed to use a Facebook strategy while another one could use microsoft and fitbit. I don't know if that kind of flexibility is planned to be built in (or at least planned to be achievable through extension; it has been suggested though within #61) but it would be pretty great to have it for multi-tenant scenarios.

@yordis
Copy link
Member Author

yordis commented Apr 26, 2018

@rmoorman the first step IMHO is to remove the dependency with Plug.Conn from the Core (we add the HTTP dependency on top of the Core) This will allow us to deal with function calls without any Plug.Conn involve so it is much easier.

Multi-tenant at that moment becomes easier because for the Core still the same but you can create your own Controller/Plug/Middleware that knows how to read the configuration either by using URLs or Headers or whatever decision you made.

But I think that we should tackle the Plug.Conn first and then move to this one, because it becomes much easier, maybe we do not need to configure anything per module because it is easier to write some Controller that initialize the workflow

Thinking out loud

@rmoorman
Copy link

rmoorman commented Apr 26, 2018

Makes sense @yordis , thank you!

@arjan
Copy link

arjan commented Apr 26, 2018

Okay, so I hear it's not time for me to start on this, I guess :-) Is there already an issue for the Plug.Conn rewrite that I could track?

@yordis
Copy link
Member Author

yordis commented Dec 24, 2018

Moved to #90

@yordis yordis closed this as completed Dec 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants