-
-
Notifications
You must be signed in to change notification settings - Fork 128
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 submitting this @Serjlee, I made some mostly aesthetic suggestions as the code seems pretty good.
Can we perhaps add some tests that verify this new functionality?
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.
@Serjlee please let me know if you plan to accommodate the review comments.
Hey @alexkappa! I'm back from the dead, I'll address your suggestions in a bit |
@alexkappa done: I took the chance to squash my changes since I had to rebase to handle the way requests are now built |
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.
fyi. Your initial version was for v4 thus I've made quite a similar PR for v5 already at #157
// Delete an enrollment | ||
// | ||
// See: https://auth0.com/docs/api/management/v2#!/Guardian/delete_enrollments_by_id | ||
func (m *MultiFactorManager) Delete(id string) error { |
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 it would be better to add EnrollmentManager here while the API is for an enrollment rather than a factor.
@lambdalisue introducing an EnrollmentManager sounds like a good idea, but at this point I wouldn't mind closing this PR in favour of yours since they would became kinda identical. @alexkappa having either PR merged is ok for me: let us know what to do! |
Note that mine does not include // https://community.auth0.com/t/when-will-api-v2-users-userid-authenticators-be-documented/52722
type authenticator struct {
ID *string `json:"id"`
Type *string `json:"type"`
Confirmed *bool `json:"confirmed"`
Name *string `json:"name,omitempty"`
CreatedAt time.Time `json:"created_at"`
EnrolledAt *time.Time `json:"enrolled_at,omitempty"`
LastAuthAt *time.Time `json:"last_auth_at,omitempty"`
LinkID *string `json:"link_id,omitempty"`
}
func getUserAuthenticators(auth0UserID string, opts ...management.RequestOption) (ators []*authenticator, err error) {
err = m.Request(
"GET",
m.URI("users", auth0UserID, "authenticators"),
&ators,
opts...,
)
return
} I'll add an equivalent function to my PR if you need that function 👍 Let me know. |
I thought of exposing it in the library after receiving this reassuring reply from Auth0:
(but since in v5 it's possible to build requests from outside the package, I'm fine even without it) |
management/user.go
Outdated
// | ||
// It's an undocumented API, see: https://community.auth0.com/t/when-will-api-v2-users-userid-authenticators-be-documented/52722 | ||
func (m *UserManager) ListAuthenticators(id string) (enrols []*UserAuthenticator, err error) { | ||
err = m.Request("POST", m.URI("users", id, "authenticators"), &enrols) |
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.
Hello and thanks for making these additions! Just wanted to pop by and make a quick comment. Using POST for the request in ListAuthenticators
gave me a 404, but using GET works. I think we might need to use GET instead (it's probably the same for Enrollments
as well).
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.
Yikes, thanks, That's why committing stuff late at night is a terrible idea.
@alexkappa So what should we do next? Close this PR and move to #157 or continue here? Let us know. |
@lambdalisue's I'm closing this PR in favour of yours since you are definitely keeping a keener eye on this |
Because of some custom integration of MFA, we needed a few more Management API endpoints not yet exposed by this library:
Let me know if I need to tweak something in order to merge!