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

[oidc] do not useaud JWT claim [THREESCALE-2263] #1007

Merged
merged 2 commits into from
Apr 9, 2019
Merged

Conversation

mikz
Copy link
Contributor

@mikz mikz commented Apr 3, 2019

it might not contain client_id, so better not to use it
#988 (comment)

@mikz mikz requested a review from a team as a code owner April 3, 2019 07:59
@mikz mikz force-pushed the oidc-aud-check branch 2 times, most recently from f1a23dc to 96f7768 Compare April 3, 2019 08:11
@@ -91,7 +91,7 @@ function _M:get_all(service_id)

local report = keys_helper.report_from_key_batched_report(key, value)

if value and value > 0 and report.service_id == service_id then
if value and value > 0 and report and report.service_id == service_id then
Copy link
Contributor

@davidor davidor Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the failing test was because of this:

We should change that to azp => "appid".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but still report_from_key_batched_report can return nil and this will crash. That should not happen.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that too. I think that apart from that, we should add a warning in report_from_key_batched_report, because if it returns nil, it means that the policy received a combination of "auth params" (app_id, app_key, ...) that it's not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That probably deserves its own PR with a test. Will create one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikz mikz force-pushed the oidc-aud-check branch 2 times, most recently from c4726ae to 50d7e6d Compare April 4, 2019 15:19
@mikz mikz changed the title [oidc] do not useaud JWT claim [oidc] do not useaud JWT claim [THREESCALE-2263] Apr 8, 2019
@mikz mikz force-pushed the oidc-aud-check branch from 23c2a4b to 00ca44e Compare April 9, 2019 08:37
@mikz mikz requested a review from davidor April 9, 2019 08:44
it might not contain client_id, so better not to use it
#988 (comment)
@mikz mikz force-pushed the oidc-aud-check branch from 00ca44e to 8b5e18c Compare April 9, 2019 08:45
@davidor davidor merged commit 9d8a83e into master Apr 9, 2019
@davidor davidor deleted the oidc-aud-check branch April 9, 2019 13:39
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.

2 participants