-
-
Notifications
You must be signed in to change notification settings - Fork 128
Extended MFA api #200
Extended MFA api #200
Conversation
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.
Thank you for putting this together @apamildner! It looks pretty good apart from some minor things. Let me know when you'd like another review and I can merge this in.
@@ -11,6 +11,16 @@ type MultiFactor struct { | |||
TrialExpired *bool `json:"trial_expired,omitempty"` | |||
} | |||
|
|||
type MultiFactorPolicies []string |
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'm not sure if we should alias []string
. Do you see some advantage to this, or did you take this as an example from a different method in this package?
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.
Good point. I just felt it looked more neat since almost all other methods use a struct type, so the method pattern looks similar for most of the api calls in the file. But we can go without it if you think it's better.
management/guardian.go
Outdated
type MultiFactorMessageTypes struct { | ||
MessageTypes *[]string `json:"message_types,omitempty"` | ||
} |
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 is okay, but I'm wondering if we should apply the simplification we did in user blocks (see below). Do we expect any other fields to be present in the future for MultiFactorMessageTypes
? If no we could return a []string
from MessageType()
and its siblings. WDYT?
Lines 225 to 232 in 40ec893
type userBlock struct { | |
BlockedFor []*UserBlock `json:"blocked_for,omitempty"` | |
} | |
type UserBlock struct { | |
Identifier *string `json:"identifier,omitempty"` | |
IP *string `json:"ip,omitempty"` | |
} |
Lines 399 to 403 in 40ec893
func (m *UserManager) Blocks(id string, opts ...RequestOption) ([]*UserBlock, error) { | |
b := new(userBlock) | |
err := m.Request("GET", m.URI("user-blocks", id), &b, opts...) | |
return b.BlockedFor, err | |
} |
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.
Hi! I'm sorry but I don't understand how this becomes a simplification. But to answer your question MultiFactorMessageTypes
would not be expected to contain any other fields. I don't think it has any siblings, maybe the confusion lies in the name of the struct. I changed this in the latest commit.
225ad28
to
c160c70
Compare
Hi! Any chance you can review this again? @alexkappa |
Thanks for your contribution @apamildner. It looks like you addressed @alexkappa's concerns. I've double-checked that tests run w/o issue locally. Merging! |
Fix formatting & new accessors for PR #200
Proposed Changes
We can now update MFA policy and some settings regarding what provider and message type we use.
Fixes #199
Acceptance Test Output
Community Note