https://kotlinlang.org logo
Join Slack
Powered by
^oauth2/openid
# ktor
d
^oauth2/openid
PHP has (as far as I remember) a 
$_REQUEST
global, that merges 
$_GET
and 
$_POST
for convenience.
I think in this case we could use get and fallback to receiving the body parameters and using them in this case?
m
That would be handy, @Deactivated User
d
Do you have by chance a link to the oauth (or openid) spec that states that 
code
and 
state
could come either from get or post parameters?
m
We’re actually trying to build a production feature around this.
d
nice!
m
This is what I was using.
d
perfect, that will do!
I’m going to check if oauth2 allows this too
looks like it is the case
m
Wouldn’t this be a general HTTP feature, though?
The ability to handle a POST with form_urlencoded parameters and treat them as regular params should be normal behavior, no?
d
actually since you can determine where to get the fields from (using the form_post), maybe it is not necessary
m
What do you mean?
d
the problem in this case is that PHP reads POST before handling the request, while in ktor you have to read explicitly and parse it if you want (for memory and performance sake). You would have to do something more explicit eitherway. For example: 
receive Parameters + merge with getParameters
m
But isn’t that the behavior you want in general when you get a POST with 
application/x-www-form-urlencoded
?
When you get that, you know that the body is effectively one giant query string.
And it should be merged into the ktor call parameters structure.
d
maybe something like a 
FormPost
feature doing this, configurable to limit the payload size, would do, right?
m
Yeah, you want ktor to have some configurable limit on the size of the POST body
d
Ok, from my side I think it is a reasonable proposal. And if it can be a feature, people can enable it only if they need it. But still what about the code that used to work without that feature enabled? would break? (since the body can be read only once) for example if you call 
receive
twice, the second time you call it, it has an empty body to process (thinking out loud) At any rate, I think this could be treat separately. For oauth, we should honor the post_ and we can determine that from the get parameter.
m
So I think someone with better knowledge of the HTTP specification and how it is usually implemented would be a better person to answer this than me whether that breaking behavior you mention should in fact be the default.
For now, I know that what is biting us in the ass is this not working for oauth2/OpenID.
And if I can’t build this in ktor, I’m going to have to resort to standing up a node server, and no one wants that 😉
Also, @Deactivated User, if you look at that OAuth2 document (https://auth0.com/docs/protocols/oauth2#how-response-type-works), you’ll see that for OpenID, we would actually want to get something other than 
code
.
d
yeah 🙂 , let me try to do a branch + PR to fix the post issue. If you feel comfortable with it, you can fork the repo and install locally to try the snapshot version so you can continue working on it
uhm
m
Specifically, we’d want to get the 
id_token
.
So, not sure if this means ktor now needs a separate OpenID handler?
d
actually I think it would be a good idea to have a feature for supporting openid, and obviously that could reuse oauth2 as much as possible. If somehow openid is not fully compatible with the standard oauth2, maybe would be nice to be able to configure it
m
If you point me at a branch, I can work on consuming it on our end and provide feedback.
It seems like the big difference is what response_type you look for.
For OAuth2, you want either the 
code
or the 
token
.
For OpenID, you want 
id_token
.
d
the problem is that maybe I’m not doing all the changes required for it, so if you are working on an openid feature, probably it would be a good idea to fork ktor itself, make all the required changes to the oauth2 to work with the openid feature and then make a PR with them
m
OK, I will see what we can do on our end 🙂
d
nice!
m
Should it live as another implementation under the Authorization feature?
I mean, technically it is just authentication, not authz, but…
d
do you mean openid or oauth?
m
openid
oauth2 is already configured as part of the Authorization feature, isn’t it?
d
I would modify oauth2 to be able to configure it so it is compatible with its standard and with openid (to reduce code duplication), and then would create a new feature in another artifact 
ktor-openid
for example in the case it is big enough or has additional dependencies. If the changes required for oauth2 are too big. I would do the typical “abstraction + two implementations” to keep it clean and DRY.
m
Errr, wait…
there is no Authorization featre.
There’s the Authentication feature.
So yeah, I answered my own question.
d
🙂
m
If we added another provider under 
OAuthServerSettings
, and called it 
OpenIDServerSettings
, would that be a reasonable place to hang it?
You already have 
OAuth1aServerSettings
and 
OAuth2ServerSettings
there…
Hmmm, actually, take a look at 
OAuth2.kt:simpleOAuth2Step2
d
Looks reasonable to me. But I didn’t created that code, so I can tell for sure. Still I think that if the logic is there in a reasonable way, it should be easy to adjust a few things later if required
m
where you call 
when (method)
, you already try and consume the 
request.body
d
let me check
looking at that code, it seems that in the when it is building a http request to an external server, and later at 
val body = response.readText()
it reads the contents of that requests.
m
Ah, right, this is a little later on, after you call 
client.call(request).response
OK, it’s past midnight here, so I’m going to knock off… if you have any other thoughts on how to best go about this, post them up here and I’ll look at them tomorrow. Otherwise, we’ll try and look at this on our end tomorrow or Monday.
Thanks for the help, @Deactivated User
d
okay, I will try to advance this as much as I can
you are welcome, and thank you too for all the feedback and the work 🙂
42 Views
Open in Slack
PreviousNext
Powered by