storage: fix storage implementation. Authorization should use AuthorizeRequester interface

[cristiangraz]

The last commit for work on #20 made major API updates, but broke the authorize code implementation in the storage. Storage should use fosite.AuthorizeRequester interface in CreateAuthorizeCodeSession and GetAuthorizeCodeSession -- fosite.Requester is used. This PR updates the storage interface and related tests.

Signed-off-by: Cristian Graziano

[aeneasr]

It is intended that all storage APIs use fosite.Requester instead of the more specific AuthorizeRequest or RefreshRequest. This is because, for example, GetAuthorizeCode is used at the token endpoint as well.

What broke exactly? Maybe there is another way to fix this issue?

[cristiangraz]

Hmm maybe I looked at the PR too quickly. When creating an authorization code, there is no access to store the redirect uri or state any more (as far as I can tell). Likewise when retrieving the authorize code, there is no way to return the redirect uri and state from the storage.

[aeneasr]

Thanks for the review :) There is a new method added: GetRequestForm. This will give you all the form data, including redirect_uri and state and it is much more flexible as you can store all request data, not just a subset.

Does this answer your question?

[aeneasr]

By the way, GetPostForm will equal to http.Request.Form (query and body variables) at the authorize endpoint and http.Request.PostForm (only body variables) at the token endpoint. As far as I checked that should be conforming to spec :)

[cristiangraz]

I'm sorry -- I dug through the implementation as I refactored my storage but somehow missed that with all the changes. Thanks for clarifying, that does indeed work to retrieve the values and store them with the authorize code.

So when returning a fosite.Requester from GetAuthorizeCodeSession, should the storage respond with something like this in order to return back the redirect_uri and state?

req := &fosite.AuthorizeRequest{
    Request: fosite.Request{
        Client:      app,
        Scopes:      fosite.Arguments(scopes),
        RequestedAt: created.Time,
    },
    RedirectURI: u,
    State:       state,
}

That was part of my confusion. The token response is supposed to return the state per section 4.1.2 of the RFC if one was provided with the request. When I make the request, the state is set as some-random-state-foobar and I am returning that as shown above.

But when I write the response using this:

provider.WriteAccessResponse(c, accessRequest, response)

The state is not returned. You can see the output of response.ToMap() has an empty state:

map[refresh_token:rFgAxLlIcqKq0+hbZBwdFlHRGViG9buaIl/LrL/v9e8.c/iL5GajIdhtXR29ifnwysLES6vWshXTmtRdi0W50CI state: scope:fosite access_token:Y14Y9haXUSRjrBDOQgvbWmQN11kVwnQ7Yg5u2ag5puA.WH6s3VhFhLRlxyhf7gRbNzbVhGQQ0YkNQqbok0WZqS8 token_type:bearer expires_in:3600]

That's why I thought AuthorizeRequest should be used. Otherwise fosite is going to have to do a type assertion in order to return the state, and users implementing the storage may not return the correct type when returning from GetAuthorizeCodeSession.

Am I missing something?

[aeneasr]

GetAuthorizeCodeSession(code string, session interface{}) (request fosite.Requester, err error) returns a requester, so no need to do anything else (you can return an AccessRequest but there's not really a need for it!). Something like this should be enough:

req := Request: fosite.Request{
        Client:             client,
        Scopes:          fosite.Arguments(scopes),
        RequestedAt: createdAt,
        RequestForm: url.Values{
               "redirect_uri": "https://foobar.com/", // be aware that if a redirect_uri is present, it also needs to be present when you make the token request. It's written somewhere in the spec, not sure where exactly right now
               "state": "foobar",
        },
}

[aeneasr]

You can also take a look at the very simply example storage implementation here.

[cristiangraz]

Ok thank you -- that did it (using Form rather than RequestForm) and I now see the state being returned.

I apologize for the unnecessary PR here. Thanks for being so helpful!

[aeneasr]

No need to apologize, I'm glad that it worked out for you and I'm also glad that you are one of the early adopters of fosite :)

If you run into issues or trouble again, feel free to ask any time.

I am looking to have a first stable API by end of february so hopefully BC breaks will be much less common then