oauth2_config: what's the point?

[dvv]

I wonder if configuration proplist can be passed to each function so that one could use several oauth2 configurations (can't yet put clear use case beyond dynamic config changes) and un-pin library off application:get_env/2 which is way inflexible in cases other than boring ( :) ) vanilla releases? --Vladimir

[bipthelin]

Sure it can be done, I would love if you could give me an convincing use case. I'm very much for implementing features that are used by people but implementing features "just because", just add weight to the project.

Maybe you can use some application:set_env/3 magic, but then again I don't know your use case.

[IvanMartinez]

I would replace it with a different alternative. I copy below part of an e-mail I sent Bip:

I have noticed that every time that a oauth2_backend function is evaluated, the following chain of function evaluations is performed to obtain the module that implements the backend:

• oauth2_config:backend()
• get_required(backend)
• application:get_env(oauth2, backend)
• case

It seems to me like too many things going on to do what should be a direct evaluation of the function that actually implements the backend operation. It also took me a while to realize how this worked. Why don't we just remove oauth2_backend implementation, and add a template file like "oauth2_backend.erl.txt" explaining what needs to be done?. I think this would be more straightforward for the developers and more efficient to execute.

Actually, I'm currently having trouble with oauth2_config because I can't make it find the value of "backend" in "app.config".

I'm afraid I'm totally against passing the configuration as a parameter to every function, it messes things up.

[bipthelin]

@IvanMartinez Do you start oauth2 with application:start(oauth2). ?

[IvanMartinez]

Yes, I do this:

before_tests() ->
    oauth2_ets_backend:start(),
    oauth2_ets_backend:add_client(?CLIENT1ID, ?CLIENT1SECRET, "", ?CLIENT1SCOPE),
    error_logger:info_msg("oauth2 start = ~p~n", [application:start(oauth2)]),
    error_logger:info_msg("Env = ~p~n", [application:get_all_env(oauth2)]),
    ok.

And I get the following:

[oauth2_webmachine]$ rebar eunit skip_deps=true
...
=INFO REPORT==== 9-Apr-2013::21:22:32 ===
oauth2 start = ok

=INFO REPORT==== 9-Apr-2013::21:22:32 ===
Env = [{included_applications,[]}]

The test files are in oauth2_webmachine/test and I put copies of app.config in oauth2_webmachine and oauth2_webmachine/priv, with no success.

[bipthelin]

I think the problem is that you don't start your own application. Look at how the example does it: https://github.com/kivra/oauth2_example

Yes I know the example is way old.

[dvv]

@IvanMartinez Frankly, I don't think configuration treated as options and passed as last parameter messes things up -- I believe that's the way most library functions do. @bipthelin I believe oauth2_backend can be transformed to behaviour definition -- then it's up to implementation to decide what options it needs and how those options are read/managed

[bipthelin]

I agree that the backend should become a behavior/callbacks definition but I don't see how that will change anything with regards to config parameters.

[dvv]

The only use of oauth2_config apart from proxying backend is getting expiry time (afaics) which imho is pretty perfect to be an option in options proplist. For example, admin users may be given bigger TTL while users from Antarctica -- smaller.

[dvv]

Another (not so strong) consideration is that at some point OAuth2 is meant (I believe) to be exposed to Web -- hence options to oauth2 can be naturally given as request handler options (which is rather well imho shown here)

[bipthelin]

What could be nice is to provide the standard application support we have today but add the possibility to add an optional Options paramater that gets precedence over any application level options.

[dvv]

That would elegantly solve the issue providing that overriding options are first class citizens and it won't complain on the absense of standard application level config. I would also +3 turning backend into behaviour.

[IvanMartinez]

@bipthelin Apparently my problem is that i'm missing the "erl -config app.config" option, but I can't see how to pass it when doing "rebar eunit".

[IvanMartinez]

I still prefer my idea of removing "oauth2_backend.erl". Why do we want the library to compile and run without an actual backend, anyway?.

[dvv]

Do you mean replacing one file which does proxying with another which is textually describes what a backend shall do?

[IvanMartinez]

@dvv I don't like the idea of adding a "take anything" parameter to every function just in case you need to pass anything besides what is expected, because in most cases you won't. I don't think your example illustrates the need for this, because you are just passing in State what oauth2 handles with specific parameters:

check_redirection_uri(Req, State = #state{client_id = ClientId,
    redirect_uri = RedirectUri, backend = Backend}) ->

Why don't you do?:

check_redirection_uri(Req, ClientId, RedirectUri, Backend}) ->

And still, I can't see why the backend needs to be dynamic.

[IvanMartinez]

@dvv I mean something very simple:

• Remove the "oauth2_backend" module. Then any call to an oauth2_backend function will fail, but of course, you need to implement your own backend!.
• Add documentation to explain how the oauth2_backend module should be implemented to make things work.

[dvv]

This is exactly what behaviours are -- define callbacks one must implement and document them with specs :)

[dvv]

In my example I dance from the fact you expose token to Web and tried to go till the end with cowboy REST request handler -- thus (Req, State)

[IvanMartinez]

I do it in two layers, one does the webmachine-oauth2 tranlations (oauth2_wrq module) and the other is this oauth2 library. See below my unfinished implementation of the Client Credentials Grant flow:

process(ReqData, State) ->
    case oauth2_wrq:get_grant_type(ReqData) of
        client_credentials ->
            case oauth2_wrq:get_client_credentials(ReqData) of
                undefined ->
                    oauth2_wrq:invalid_client_response(ReqData, State);
                {ClientId, ClientSecret} ->
                    case oauth2:authorize_client_credentials(ClientId, ClientSecret, oauth2_wrq:get_scope(ReqData)) of
                        {ok, _Identity, _Response} ->
                            {{halt, 200}, wrq:set_resp_body("{ \"a\":\"1\" }", ReqData), State};
                        {error, _Reason} ->
                            oauth2_wrq:invalid_client_response(ReqData, State)
                    end
            end;
        undefined ->
            oauth2_wrq:invalid_request_response(ReqData, State);
        _ ->
            oauth2_wrq:unsupported_grant_type_response(ReqData, State)
end.

This is how the library is meant to be isued, IMHO.

[IvanMartinez]

@dvv I may not be getting your idea because I'm not very experienced with Erlang. I understand you have a custom or gen_server oauth2_backend behaviour in the library and then you implement it with a module in your application, say my_backend. How do you tell oauth2 functions to use my_backend?. Aren't we still in the same situation in which we need a configuration parameter?.

[dvv]

In this case we have State to bear all options considering oauth2. I'm using my dvv/termit to make tokens serialized self-contained crypted and signed erlang terms, no need to store them at all. I'm not advocating this approach but it imho has rights to live. So for full fledged authorization code flow I have minimum three pairs of options: secret and TTL for authorization code, for access token and for refresh token

[IvanMartinez]

I just figured out how to solve my testing problem by mocking oauth2_config:

meck:new(oauth2_config),
meck:expect(oauth2_config, backend, fun() -> oauth2_ets_backend end),
meck:expect(oauth2_config, expiry_time, fun(_) -> 3600 end),

[dvv]

Any conclusion on the point?

[bipthelin]

I'll whip up a patch and let's discuss it from there!

-Bip Thelin

On 11 apr 2013, at 09:11, Vladimir Dronnikov notifications@github.com wrote:

Any conclusion on the point?

— Reply to this email directly or view it on GitHubhttps://github.com/kivra/oauth2/issues/18#issuecomment-16219882 .

[IvanMartinez]

If possible, before making any changes that may cause conflicts with my fork, please consider PR #16 and then the PR that I will create afterwards to fix #12. Thanks.

[bipthelin]

Ok, I've merged PR #16 and made changes to how the backend works. Take a look and comment!