Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added openidc_ensure_discovered_data to openidc_authorization_response #224

Closed
wants to merge 1 commit into from
Closed

Conversation

mijohansen
Copy link

When running against a Discovery-url I get the following error:

2018/11/05 14:30:58 [error] 30#30: *1 lua entry thread aborted: runtime error: /usr/local/openresty/luajit/share/lua/5.1/resty/openidc.lua:1007: attempt to concatenate field 'issuer' (a nil value)

When running openidc_ensure_discovered_data(opts) before the check inside the function openidc_authorization_response that seem to fix the problem.

When running against a Discovery-url I get the following error:

```
2018/11/05 14:30:58 [error] 30#30: *1 lua entry thread aborted: runtime error: /usr/local/openresty/luajit/share/lua/5.1/resty/openidc.lua:1007: attempt to concatenate field 'issuer' (a nil value)
```
When running  `openidc_ensure_discovered_data(opts)` before the check inside the function `openidc_authorization_response` that seem to fix the problem.
@zandbelt
Copy link
Contributor

zandbelt commented Nov 5, 2018

interesting: I guess you bumped into this because your OP is returning an "iss" parameter in the authorization response, can you confirm that?

Anyhow, a little further down is the err = ensure_config(opts) clause that should really be moved up a few lines, up to where you added the openidc_ensure_discovered_data call now. (having both is redundant)

@mijohansen
Copy link
Author

Yes, or OP is returning the "iss" parameter.

@zandbelt
Copy link
Contributor

zandbelt commented Nov 5, 2018

do you want to update the patch or do you want me to do it? (if you you do you get to add yourself to the AUTHORS file... )

@mijohansen
Copy link
Author

Hehe, it would be great if you just add it if you have time. :-)

@zandbelt zandbelt closed this in 2879b43 Nov 6, 2018
@zandbelt
Copy link
Contributor

zandbelt commented Nov 6, 2018

@mijohansen all set now, thanks; @bodewig as this is an actual bug, do you think we should release 1.7.1? and if so, anything else that you'd want to go in?

@bodewig
Copy link
Collaborator

bodewig commented Nov 7, 2018

We could probably merge #217 right away an I could mix in the client_secret_jwt support. But if you want to cut 1.7.1 first, that's also fine with me.

@zandbelt
Copy link
Contributor

zandbelt commented Nov 7, 2018

I'll hold off, the "iss" parameter is a non-core option that not too many OPs use; take your time to prep the additional authentication methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants