Define AuthLog struct

[tiffanycitra]

Adding defined struct for AuthLog instead of a map of generic interface.

[alexeagle]

LGTM
@jordan-wright ping, we are hoping to merge this so we can get off our fork of the project :)

[jordan-wright]

Hey there!

Thanks for sending this in @tiffanycitra. I'll give this a full review ASAP but at first glance it LGTM.

One thing I want to check with the team on is how we want to handle breaking changes. I don't believe we've cut any new releases yet (which is something on our radar 😄) so we should be ok re: honoring Go's versioning expectations but I still want to be sure we're taking this into consideration.

I'll keep y'all updated!

[tiffanycitra]

Hey @jordan-wright, thanks for the reply! That sounds good. I actually just checked the latest API documentation here and it looks like you've added more fields to the response? I copied over the struct on this PR from our fork a few months ago (right after v2 API gets released). If that's the case, I can also update this ASAP.

[jordan-wright]

@tiffanycitra Yes! I was just looking back at the docs and I think you're right. I was going to get a sample auth log so we could make sure this PR covers all the various fields but it looks like you're one step ahead of me 😄

[tiffanycitra]

@jordan-wright haha great! :) i referred to the doc to get the latest API response fields and made the update accordingly. i also tested the code locally with our API host and tokens. a couple of notes:

• i noticed that is_encryption_enabled, is_firewall_enabled, and is_password_set are set to be string on the API documentation. However, on your sample returned response on the API documentation, it's typed as boolean. i updated the test payload to be string since that's the actual returned value type.
• it seems that security_agents with our API is returned as unknown (string), whereas the API mentioned that it's a list and so is the example (set as an empty list). i updated it to be string on the struct. can you double check?
  thanks!

[jordan-wright]

Thanks for the quick updates!

i noticed that is_encryption_enabled, is_firewall_enabled, and is_password_set are set to be string on the API documentation. However, on your sample returned response on the API documentation, it's typed as boolean.

This sounds like a possible documentation miss, since I believe the values can be "true"/"false"/"unknown" (the unknown is why it'd have to be a string).

it seems that security_agents with our API is returned as unknown (string), whereas the API mentioned that it's a list and so is the example (set as an empty list).

That's interesting. Let me follow up on that as well.

In both cases, great catch! I'll follow-up to get answers on these and we can make any revisions we need to. Sorry for any confusion between the API's/docs, etc. This feedback is super, super helpful.

[jordan-wright]

I've looked a bit more into this and it seems that the security_agents field can either be the string unknown or a list containing zero or more agents. Additionally, the is_encryption_enabled, is_firewall_enabled, etc. fields are returned as either a boolean or the string "unknown". This is easier to handle in Python, but for us on the Go side having mixed types is a bit more difficult. 😄

Let me do some thinking about how we can handle this and I'll get back to you ASAP. My apologies for the confusion here, and I appreciate you taking the time to send in these changes!

[alexeagle]

Ping again :)

[BradleyHiggins]

Hi! Thanks so much for the PR! In fun news, Jordan has moved on to a new opportunity, and we wish him all the best. I'm happy to help get this PR merged.

I think there are two things that need addressing before this code can be merged. As noted earlier in this conversation, the Duo API results for security agents is...suboptimal. The API returns either a string "unknown", OR a list of agents. Unfortunately, this means that a type of string for the security_agents field will break any API call that actually results in a list being returned. I'm going to see what we can do on the backend to resolve this. In the mean time, we can omit the field from the object, and add it back in once the API acts consistently with regard to its returned types.

Second, this repo has never been versioned. But, there are clients of this repo that depend on it, so we've always maintained its backwards-compatibility. This PR change will modify the AuthLog exported variable type, which will break any existing clients that are using this type. The choice we're presented with now is - would we rather introduce a new type to represent an unmarshalled AuthLog, say AuthLog2 (or some other clever name) - or should we introduce a new version of this api, and modify the existing AuthLog struct? Do you have a preference?