Fix ticket #1080 - Allow multiple redirect_uris per oauth2 app

[PGBI]

The aim of this PR is to address the ticket #1080 and allowing an oauth2 app (aka "oauth2 credentials") to register several redirect_uris.

Before the change, oauth2 credentials are of the form:

{
      "id": "25b31464-6245-4e11-9205-59c723152444",
      "consumer_id": "f23bf76d-e691-4b0f-bc29-950e4cf4d651",
      "client_id": "0323125f47beb13a8f3eb47dee55240c",
      "client_secret": "3e806877319486387a0528cd5a0bf95f",
      "redirect_uri": "http://aaa.com",
      "name": "test",
      "created_at": 1459329919000,
}

after the change:

{
      "id": "25b31464-6245-4e11-9205-59c723152444",
      "consumer_id": "f23bf76d-e691-4b0f-bc29-950e4cf4d651",
      "client_id": "0323125f47beb13a8f3eb47dee55240c",
      "client_secret": "3e806877319486387a0528cd5a0bf95f",
      "redirect_uri": [
            "http://aaa.com",
            "http://bbb.com",
      ],
      "name": "test",
      "created_at": 1459329919000,
}

To be noted: the oauth2 spec specifies that a redirect_uri must not contain a fragment. This check was performed at the authorization endpoint. I moved this check to the admin API, when creating / updating oauth2 credentials.

[PGBI]

Quick comment about this: the aim of this code is to allow the code to retrieve oauth2 credentials whose redirect_uri were saved before this PR is (hopefully) merged. For these legacy records, redirect_uri is indeed not serialized, it's just a string, and the json decoding is failing.

I hesitated between doing so and writing a migration file that would convert all the redirect_uri in the new array format. I didn't do so because:

• I believe this approach is better to be able to upgrade a cluster of kong nodes without downtime,
• I don't feel comfortable coding migration files, and don't know how to test them.

If you guys would rather have a migration file, I would appreciate you help me on that one.

[coveralls]

Coverage decreased (-0.3%) to 64.479% when pulling 4a67a276f0f28c0422c92788a2b063cacdca7b0e on classy-org:feature/multiple_oauth2_redirect_uris into cbd03da on Mashape:next.

[PGBI]

@thefosk any idea why the tests are failing? I have no pb when I run them locally.

[subnetmarco]

Hi @PGBI - sorry for keeping this pending for a while. Could you fix the merge problems with next, and then I will be happy to merge it. The only thing I will need to double-check is the changes in cassandra_db.lua: we are already using arrays somewhere else and I would be surprised if this wasn't already handled by the Cassandra DAO implementation.

[PGBI]

@thefosk I fixed the conflict. Again, unit tests are failing on travis. Any idea why? I can't reproduce on my dev setup (I'm using kong-vagrant for developing and running tests) when running busted

[subnetmarco]

@PGBI correct me if I am wrong, but since redirect_uri is now an array in the schema, we don't have to check if it's a table. It should always be parsed like a table, or nil.

So if v and type(v) == "table" then becomes if v then and that should be enough.

[PGBI]

You're right. Corrected.

[subnetmarco]

Regarding the tests don't worry, if they work locally it's okay - we are having problems with TravisCI.

[PGBI]

@thefosk Is there any other concern I did not address?

[subnetmarco]

@PGBI can you squash your commits?

[PGBI]

Hi,

I'm in vacation right now. Can you please take care of that? Otherwise will do on Monday.

Thanks

Le 26 mai 2016 à 03:06, Marco Palladino notifications@github.com a écrit :

@PGBI can you squash your commits?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[subnetmarco]

Squashed and merged with fb22516

[DonMartin76]

When did you fix the encoding of the redirect_uri array as a string? In 0.8.3, you got this in Kong's response:

"redirect_uri":"[\"http:\\/\\/dummy.org\"]"

In 0.9.2, it seems to be returned they way you would expect, as a proper JSON array.

[thibaultcha]

This is actually proper JSON. See mpx/lua-cjson#2

[DonMartin76]

Hehe, sure it's proper JSON, but still not what you would expect. Had a workaround for this in wicked.haufe.io which bit me now when switching to 0.9.2.