-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add support for AuthenticationStrengthPolicies
#249
Conversation
AuthenticationStrengthPolicies
AuthenticationStrengthPolicies
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.
Many thanks for this @bubbletroubles, this is mostly looking good. I noted one correction below, and also thought that we should make sure to test this in the ConditionalAccessPolicyClient tests. For example:
authStrengthPolicy := testAuthenticationStrengthPoliciesClient_Create(t, c, msgraph.AuthenticationStrengthPolicy{
DisplayName: utils.StringPtr(fmt.Sprintf("test-policy-%s", c.RandomString)),
Description: utils.StringPtr("Password and Hardware OATH"),
AllowedCombinations: &[]string{"password, hardwareOath"},
},
)
policy := testConditionalAccessPolicysClient_Create(t, c, msgraph.ConditionalAccessPolicy{
DisplayName: utils.StringPtr(fmt.Sprintf("test-policy-%s", c.RandomString)),
State: utils.StringPtr("enabled"),
Conditions: &msgraph.ConditionalAccessConditionSet{
ClientAppTypes: &[]string{"mobileAppsAndDesktopClients", "browser"},
Applications: &msgraph.ConditionalAccessApplications{
IncludeApplications: &[]string{testAppId},
},
Users: &msgraph.ConditionalAccessUsers{
IncludeUsers: &[]string{"All"},
ExcludeUsers: &[]string{*testUser.ID(), "GuestsOrExternalUsers"},
IncludeGroups: &[]string{*testIncGroup.ID()},
ExcludeGroups: &[]string{*testExcGroup.ID()},
},
Locations: &msgraph.ConditionalAccessLocations{
IncludeLocations: &[]string{"All"},
ExcludeLocations: &[]string{"AllTrusted"},
},
},
GrantControls: &msgraph.ConditionalAccessGrantControls{
Operator: utils.StringPtr("OR"),
BuiltInControls: &[]string{"block"},
AuthenticationStrength: &msgraph.AuthenticationStrengthPolicy{
ID: authStrengthPolicy.ID,
},
},
})
If you can look to add this, then we should be good to merge 👍
Co-authored-by: Tom Bamford <[email protected]>
Thanks @manicminer - All suggested changes have been made and Conditional Access tests updated (and passing). I had to re-trigger the tests a couple of times as other resources APIs were giving errors. I believe this was a transient problem. |
It looks like MS Graph may be having some service issues at the moment |
Hi @manicminer It's now saying the limit of authentication strenth policies is reached. This is most likely because I don't have a delete action in the custom authentication strengths test, so each test is creating a policy but not deleting it when finished. I'll add a delete action to the test and also add a cleanup file in the Are you able to manually remove a couple of policies so the tests for this PR can run successfully? |
@bubbletroubles No problem, done. I've added Authentication Strength Policies to the cleanup tool (which runs nightly) which should help with cleanup of failed tests. The limit for this particular object seems to be 18 fwiw. |
thanks @manicminer . I've just checked and there is already a delete in the |
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.
Thanks @bubbletroubles, this LGTM 🚀
Adding support for
AuthenticationStrengthPolicies
.Microsoft GraphAPI doco - https://learn.microsoft.com/en-us/graph/api/resources/authenticationstrengthpolicy?view=graph-rest-1.0