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

Fix OAuth auth caching #304

Merged
merged 5 commits into from
Mar 16, 2017
Merged

Fix OAuth auth caching #304

merged 5 commits into from
Mar 16, 2017

Conversation

mayorova
Copy link
Contributor

I realized that in OAuth mode the auth caching was not working (not sure if it never worked, the bug or was introduced at some point.

Also, I've decided to remove the duplication in authrep and oauth_authrep, as only the internal location should be different for them. This can probably be improved further, by using the co-socket http library instead of internal locations.

@mikz
Copy link
Contributor

mikz commented Mar 15, 2017

@mpguerra is making changes to the same code in #283

Might be better to sync and contribute it there.

@mikz mikz requested a review from mpguerra March 15, 2017 10:13
@mayorova
Copy link
Contributor Author

mayorova commented Mar 15, 2017

From what I can see the only changes in proxy.lua are these: https://github.com/3scale/apicast/pull/283/files#diff-b3576d69565b2de9e29a4980dbc3d09a

They shouldn't even conflict.

I've also noticed that the internal location contents were different for oauth and no-oauth, and this is indeed being fixed here: https://github.com/3scale/apicast/pull/283/files#diff-031add362afcf189262a1e0dc060ea7f (but I didn't touch it in this PR)

I am not sure this bug fix should be merged with the new feature implementation by #283, because it's not related. But I can do it if you think it's better.

@mikz
Copy link
Contributor

mikz commented Mar 15, 2017

@mayorova it is not committed yet.

@mikz mikz merged commit 643a58c into master Mar 16, 2017
@mikz mikz deleted the fix-oauth-caching branch March 16, 2017 07:28
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.

4 participants