Allow Okta auth backend to specify TTL and max TTL values #2915
Conversation
|
Hi @wjam , Thanks for sending this in! I'm happy with the feature being added, but I don't like the TTL/MaxTTL being returned from the login function. I'd much prefer that pathLogin/pathRenew read the configuration directly. |
|
Changed implementation |
| @@ -26,6 +27,14 @@ func pathConfig(b *backend) *framework.Path { | |||
| Description: `The API endpoint to use. Useful if you | |||
| are using Okta development accounts.`, | |||
| }, | |||
| "ttl": &framework.FieldSchema{ | |||
| Type: framework.TypeString, | |||
There was a problem hiding this comment.
This should be TypeDurationSecond, same with max_ttl.
| @@ -75,6 +84,8 @@ func (b *backend) pathConfigRead( | |||
| Data: map[string]interface{}{ | |||
| "Org": cfg.Org, | |||
There was a problem hiding this comment.
Please use lowercase for these and match the input values, e.g. ttl and max_ttl. Please also change the existing values -- I know it would be an incompatible change but it's an uncommon endpoint that will be read mostly by humans. It's better for them to match the input.
| @@ -118,6 +129,31 @@ func (b *backend) pathConfigWrite( | |||
| cfg.BaseURL = d.Get("base_url").(string) | |||
| } | |||
|
|
|||
| var ttl time.Duration | |||
| ttlRaw, ok := d.GetOk("ttl") | |||
There was a problem hiding this comment.
Once you switch to TypeDurationSecond most of this goes away -- you are guaranteed to get a valid int out of a Get call. So a zero value can just mean "not set" and can be used directly.
| } | ||
| } | ||
|
|
||
| cfg.TTL = ttl |
There was a problem hiding this comment.
this will then be ttl * time.Second.
| return 0, 0, errors.New("Okta backend not configured") | ||
| } | ||
|
|
||
| ttl, maxTTL, err = b.SanitizeTTLStr(cfg.TTL.String(), cfg.MaxTTL.String()) |
There was a problem hiding this comment.
This call is unnecessary, you're converting from a duration to a string back to a duration. LeaseExtend does bounds checking so you'll be fine as-is.
|
Pushed changes from code review. The GitHub auth backend should probably be changed at some point to use |
|
@wjam Yes, probably. Happy to take a PR from you that does that :-) |
| testAccUserGroups(t, username, "local_group,local_group2"), | ||
| testAccGroups(t, "local_group", "local_group_policy"), | ||
| testLoginWrite(t, username, password, "", []string{"local_group_policy"}), | ||
| testLoginWrite(t, username, password, "", defaultLeaseTTLVal.Nanoseconds(), []string{"local_group_policy"}), |
There was a problem hiding this comment.
Why are you using .Nanoseconds() everywhere? You can just compare the time.Duration values directly!
|
This is looking much better -- just a bit of cleanup on the tests and I think it's good to go! |
|
No idea why I was converting both durations to nanoseconds, I guess at some point they weren't both durations? Pushed change to fix code review issue |
|
Thanks! |
Allow Okta auth backend to have a TTL different to the system max TTL.
The TTL value is configurable per-backend rather than per-group as when logging in one user may match against multiple groups.