Skip to content
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 Country location to NamedLocation under conditional access - MS365 #4848

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

HRouhani
Copy link
Contributor

@HRouhani HRouhani commented Nov 13, 2024

Screenshot from 2024-11-13 16-00-33

@HRouhani HRouhani added the ms365 label Nov 13, 2024
@tas50 tas50 changed the title 🧹 Adding Country location to NamedLocation under confitional access - MS365 🧹 Adding Country location to NamedLocation under conditional access - MS365 Nov 13, 2024
Copy link
Contributor

github-actions bot commented Nov 13, 2024

Test Results

3 145 tests  ±0   3 144 ✅ ±0   1m 23s ⏱️ -2s
  371 suites ±0       1 💤 ±0 
   28 files   ±0       0 ❌ ±0 

Results for commit 88ca6ba. ± Comparison against base commit a4e4356.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@czunker czunker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise LGTM.
But I think we should rename the resource.

@@ -60,6 +60,8 @@ microsoft.tenant @defaults("name") {
microsoft.conditionalAccess {
// IP named location
namedLocations() []microsoft.conditionalAccess.ipNamedLocation
// Country-based named location
countryLocations() []microsoft.conditionalAccess.countryNamedLocation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading the docs, I think, this should be called namedLocations(). It can be a country or an IP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points. I changed the structure totally to make it both be under the umbrella of namedLocation

@@ -70,6 +72,14 @@ microsoft.conditionalAccess.ipNamedLocation @defaults("name trusted") {
trusted bool
}

// Microsoft Conditional Access Country named location
microsoft.conditionalAccess.countryNamedLocation @defaults("name lookupMethod") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:
namedLocation instead of country...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I did

@HRouhani
Copy link
Contributor Author

After the merging I need to change all the policies get effected with this changes!

Copy link
Contributor

@czunker czunker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactor, just error checks are missing.

}

ctx := context.Background()
namedLocations, err := graphClient.Identity().ConditionalAccess().NamedLocations().Get(ctx, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should check the error here.

@@ -20,9 +20,6 @@ func (a *mqlMicrosoftConditionalAccess) namedLocations() ([]interface{}, error)

ctx := context.Background()
namedLocations, err := graphClient.Identity().ConditionalAccess().NamedLocations().Get(ctx, nil)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, you should keep the err check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@czunker czunker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @HRouhani

… MS365

Signed-off-by: Hossein Rouhani <h_rouhani@hotmail.com>
Signed-off-by: Hossein Rouhani <h_rouhani@hotmail.com>
Signed-off-by: Hossein Rouhani <h_rouhani@hotmail.com>
Signed-off-by: Hossein Rouhani <h_rouhani@hotmail.com>
@HRouhani HRouhani force-pushed the hossein/ms365-conditionalAccess-country branch from e518ac6 to 88ca6ba Compare November 13, 2024 14:53
@HRouhani HRouhani merged commit 7748bcf into main Nov 13, 2024
16 checks passed
@HRouhani HRouhani deleted the hossein/ms365-conditionalAccess-country branch November 13, 2024 15:01
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants