truqu / elm-oauth2

OAuth 2.0 client-side utils in Elm
MIT License
81 stars 29 forks source link

State of OAuth.Implicit.Success is Nothing #21

Closed manuscrypt closed 3 years ago

manuscrypt commented 4 years ago

When using version 7.0.0, everything works as expected, except the value of state is always Nothing. The query-string contains a param named state. All the other strings are parsed.

Debug.log of the success-object:

success: { expiresIn = Just 3600, refreshToken = Nothing, scope = [""], state = Nothing, token = Bearer "eyJ0eX..." }

What am I missing? I looked at the implementation and that looks fine to me. Thanks for any pointers.

KtorZ commented 4 years ago

This is surprising because the state is basically free-form so there's no reason for the parser to fail if the value is present in the query. May I ask what your raw query looks like :thinking: ?

manuscrypt commented 4 years ago

Certainly, here's what I pass to OAuth.Implicit.parseToken:

{ fragment = Just "access_token=eyJ0e...PPjuBg&token_type=bearer&expires_in=3600&state=z31j7AMHBiAHySvY8PvtcA==", host = "localhost", path = "/dashboard", port_ = Just 4200, protocol = Http, query = Just "client-request-id=d6b27f0f-dba7-4a00-bd4b-0080000000d4" }

http://localhost:4200/dashboard?client-request-id=d6b27f0f-dba7-4a00-bd4b-0080000000d4#access_token=eyJ0e...PPjuBg&token_type=bearer&expires_in=3600&state=z31j7AMHBiAHySvY8PvtcA==

Here is my parseAuth function, adapted from your google-example to within my elm-spa app:

parseAuth : Commands msg -> Model -> ( Model, Cmd Msg, Cmd msg )
parseAuth cmds model =
    let
        randomState =
            model.session.randomState
    in
    case OAuth.Implicit.parseToken (Debug.log "query" model.session.origin) of
        OAuth.Implicit.Empty ->
            ( model
            , Cmd.none
            , Cmd.none
            )

        OAuth.Implicit.Success success ->
            case randomState of
                Nothing ->
                    ( model
                    , Cmd.none
                    , clearUrl cmds
                    )

                Just jumble ->
                    if Debug.log "state" success.state /= Debug.log "jumble" (Just jumble) then
                        ( model
                        , Cmd.none
                        , clearUrl cmds
                        )

                    else
                        ( model
                        , Cmd.batch [ Ports.log (Maybe.withDefault "nothing-state" success.state), Ports.storeToken (String.replace "Bearer " "" (OAuth.tokenToString success.token)) ]
                        , clearUrl cmds
                        )

        OAuth.Implicit.Error error ->
            ( { model | errors = [ error ] }
            , Cmd.none
            , clearUrl cmds
            )
KtorZ commented 4 years ago

Interesting. So it seems that the Url.Parser.Query which is used to parse the fragment internally doesn't quite like the padding (==) of the base64 state. Since the = sign has a very specific semantic for queries, this makes sense. This will be a good opportunity to add some unit tests to this :grimacing:

I also realize that since 7.0.0, the internal parsers are no longer exposed, which doesn't allow you to write a custom parser. That's very inconvenient. This needs fix.

manuscrypt commented 4 years ago

Thank you for looking into it (and restoring my sanity). If you're going to touch it anyway, maybe i could sneakily suggest a couple of changes here? In order to get the auth-url I really wanted, I had to:

    authorization
        |> OAuth.Implicit.makeAuthorizationUrl
        |> Url.toString
        |> String.replace "response_type=token" "response_type=token+id_token"
        |> (\s -> String.append s "&resource=95777017-55a5-4bbb-b662-b6b1fbf91a31")
        |> Navigation.load

so, i had to modify response_type and add resource. otherwise, i would not get the correct access_token (both modifications necessary). it would also be nice, to be able to get to the id_token returned, which is supplied as part of the response.

Neither this issue, nor my suggested changes are hindering me to use your package for which I am grateful. Thanks!

KtorZ commented 4 years ago

id_token isn't part of OAuth 2.0 though. I believe you're dealing with a system that implements OpenID Connect ( https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthResponse ) which is another standard built on top of OAuth. I've long thought of also implementing an OpenID Connect client library in Elm but the standard is big and I haven't found much time / or interest to be honest :blush: ...

I've purposely left the parsers in this library somewhat open, so you should be able to work around pretty easily by using parseTokenWith, and re-using some parsers of the library, while completing the OpenID Connect specifics parts with others. Although, as I mentioned above, some low-level parsers aren't exposed anymore which I'll fix very soon in a new release.

blawlor commented 3 years ago

Hi - I came across this issue after a few hours of head-scratching. Like the OP I'm happy that sanity is restored. But can I ask why the issue was closed? I'm using 7.0.1 and this problem is still present. Any suggestions for a workaround?

Actually - I guess using state that has length which is a multiple of 3?