Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

migrate to msgraph #966

Merged
merged 93 commits into from
Oct 22, 2021
Merged

migrate to msgraph #966

merged 93 commits into from
Oct 22, 2021

Conversation

chkeita
Copy link
Contributor

@chkeita chkeita commented Jun 7, 2021

This PR removes the references to soon to be deprecated azure-graphrbac and uses the Microsoft Graph Api instead
closes #870

Note this PR disables the undocumented feature of setting ONEFUZZ_AAD_GROUP_ID on the server

Test:

  • run check-pr on on both windows and linux
  • check that ONEFUZZ_AAD_GROUP_ID has been disabled
    • Add an application setting call ONEFUZZ_AAD_GROUP_ID to the instance azure function
    • Set the value to a guid
    • from the cli execute onefuzz jobs list
    • response: An error containing the error message "ONEFUZZ_AAD_GROUP_ID configuration not supported"

@chkeita chkeita linked an issue Jun 10, 2021 that may be closed by this pull request
@chkeita
Copy link
Contributor Author

chkeita commented Jun 14, 2021

Postponing the merge of this until linux machines are allowed to connect to Azure active directory.

src/api-service/__app__/onefuzzlib/azure/creds.py Outdated Show resolved Hide resolved
src/deployment/registration.py Outdated Show resolved Hide resolved
Copy link
Member

@ranweiler ranweiler left a comment

Choose a reason for hiding this comment

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

I don't think our integration tests are sufficient to test this PR.

I think we really need to test the graph queries themselves, and in particular, make sure any code that uses them for membership tests rejects authorization as appropriate, based on group membership, in e.g. check_access().

@chkeita
Copy link
Contributor Author

chkeita commented Oct 13, 2021

@ranweiler

I don't think our integration tests are sufficient to test this PR.

I think we really need to test the graph queries themselves, and in particular, make sure any code that uses them for membership tests rejects authorization as appropriate, based on group membership, in e.g. check_access().

The check_access was manually tested both by me and @nharper285 will update the description

@chkeita chkeita requested a review from ranweiler October 19, 2021 20:17
@chkeita chkeita requested a review from ranweiler October 19, 2021 22:19
Copy link
Member

@ranweiler ranweiler left a comment

Choose a reason for hiding this comment

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

Can we explicitly test (if we haven't already) and document (in the PR text) the new expected upgrade behavior if the $ONEFUZZ_AAD_GROUP_ID variable is set?

@chkeita
Copy link
Contributor Author

chkeita commented Oct 20, 2021

ONEFUZZ_AAD_GROUP_ID behavior tested

@chkeita chkeita merged commit 98cd7c9 into microsoft:main Oct 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azure-graphrbac is no longer supported
4 participants