-
Notifications
You must be signed in to change notification settings - Fork 83
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
Adding auth_params
for passwordless email connections (1/3)
#235
Conversation
… documentation changes
Codecov Report
@@ Coverage Diff @@
## main #235 +/- ##
==========================================
- Coverage 82.62% 82.60% -0.02%
==========================================
Files 36 36
Lines 6842 6852 +10
==========================================
+ Hits 5653 5660 +7
- Misses 938 940 +2
- Partials 251 252 +1
Continue to review full report at Codecov.
|
@@ -385,6 +386,13 @@ func flattenConnectionOptionsEmail(options *management.ConnectionOptionsEmail) ( | |||
} | |||
m["upstream_params"] = upstreamParams | |||
|
|||
if options.AuthParams != nil { | |||
v, ok := options.AuthParams.(map[string]string) | |||
if ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to throw an error if auth params are not a map? Or at least a warning (using the diagnostics pkg)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think throwing an error is a little heavy-handed here, but certainly agree that some action is necessary. I think logging a warning strikes a good balance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the warning. However, I'm unable to get it to render in the console, despite that LoC being reached and appropriate log verbosity configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To raise a warning you need to use a diagnostics message with SeverityWarning: https://github.com/hashicorp/terraform-plugin-sdk/blob/70ce77bce6118b74a49762bb401b46a723c0bab8/diag/diagnostic.go#L101. You can get inspiration from the hook resource:
func checkForUntrackedHookSecrets(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logging is performed in this upstream PR: #241
…therwise it would set literally as null
auth_params
for passwordless email connectionsauth_params
for passwordless email connections (1/3)
Description
As requested by #31 , this PR introduces the ability to manage authentication params for passwordless email connections. These are key-value properties that override or append to the query params of OTP links via email. One important deviation from the original request is the intentional decision to only add for passwordless email connections. I could not support the claim that that field existing for passwordless SMS connections too.
Checklist
Note: Checklist required to be completed before a PR is considered to be reviewable.
Auth0 Code of Conduct
Auth0 General Contribution Guidelines
Changes include test coverage?
Does the description provide the correct amount of context?
Have you updated the documentation?
Is this code ready for production?