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

Make oauth tokens TTL configurable #448

Merged
merged 4 commits into from
Oct 9, 2017
Merged

Conversation

davidor
Copy link
Contributor

@davidor davidor commented Oct 9, 2017

Fixes #190

@davidor davidor force-pushed the configurable-oauth-token-ttl branch from 10642f3 to a0b0214 Compare October 9, 2017 14:19
@octobot
Copy link

octobot commented Oct 9, 2017

Spell Checker found issues

doc/parameters.md

Line Typo
3 APIcast v2 has a number of parameters co
3 ariables) that can modify the behavior of the gateway. The following
5 e that when deploying APIcast v2 with OpenShift, some of thee
15 Default: **stderr**
17 or more information. The file pathcan be either absolute, or relati
21 nfo
21 warn
46 Default: "apicast"
77 Values: a number > **60**
78 Default: 0
80 r. The value should be set to 0 or more than 60. For example,
80 ould be set to 0 or more than 60. For example, if `APICAST_CON
80 ONFIGURATION_CACHE` is set to 120, the gateway will reload the
80 eload the configuration every 2 minutes (120 seconds).
80 onfiguration every 2 minutes (120 seconds).
84 Default: "127.0.0.1"
86 ning Redis instance for OAuth 2.0 flow. REDIS_HOST parameter
90 Default: 6379
92 ning Redis instance for OAuth 2.0 flow. REDIS_PORT parameter
98 ning Redis instance for OAuth 2.0 flow. REDIS_URL parameter c
98 e used to set the full URI as DSN format like: `redis://PASSWOR
103 Default: 604800
105 uthenticate using OAuth, this param specifies the TTL (in seconds
109 pty, the DNS resolver will be autodiscovered.
161 Setting it to particual version will make it not auto
173 tificate bundle generated by [export-builtin-trusted-certs](https://github.com/openresty

Generated by 🚫 Danger

[error]

=== TEST 18: default token TTL
When a default TTL is not specified, it applies a default of 7 days (604800 s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try with empty value? That is common OpenShift fuckup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I added a new test (TEST 19) which is basically a copy-pasta of TEST 18. The only difference is an env. Any ideas to avoid this kind of duplication in Test::Nginx ?

Copy link
Contributor

Choose a reason for hiding this comment

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

No idea :( It really sucks to have this kind of duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe would be useful to test it on lower level - in busted.

Copy link
Contributor Author

@davidor davidor Oct 9, 2017

Choose a reason for hiding this comment

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

Agreed. I thought about that, but then I saw that there are no unit tests for oauth, so I thought that it would be better to just complete the existing integration tests for now. Maybe this is something we could address in a future PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely. 👍

@davidor davidor force-pushed the configurable-oauth-token-ttl branch from a0b0214 to a9338d4 Compare October 9, 2017 18:16
@mikz mikz merged commit 20ec156 into master Oct 9, 2017
@mikz mikz deleted the configurable-oauth-token-ttl branch October 9, 2017 22:23
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