Update msgraph.py to_msal_proxies

[morelgeorge]

*closes: #61197

To be honest, this PR is only a draft. From the Code I can see that Authority can be retrieved from connection as an extra parameter. However, there is no information about it in the documentation.

So, this is a little bit confusing when using the proxy. So I propose to update the logic of a function to_msal_proxies. Return proxies attribute even when the variable authority is not filled.

Testing
In our environment I did a functional logic test with and without proxy. It works as expected

[shahar1]

Not too familiar with the Azure provider yet, but I'm a bit concerned due to the lack of unit tests.
I'll be happy if you could add some, at least for this method as it had already 4 if statements, non of which unit tested - which could potentially lead to unnoticed regression in the future.
Thanks for testing it on your environment though!

[morelgeorge]

hello @shahar1,
yes, I agree with you about your concerns.
I updated this pull request by updating test_msgraph.py file and added some additional unit tests.
However, I was not able to run these tests as they are in our environment due to some ImportErrors.

At least I ran added tests directly over the changed code and all of them were successful.
Also I did a test through the different setup of DAGs.

[dabla]

hello @shahar1, yes, I agree with you about your concerns. I updated this pull request by updating test_msgraph.py file and added some additional unit tests. However, I was not able to run these tests as they are in our environment due to some ImportErrors.

At least I ran added tests directly over the changed code and all of them were successful. Also I did a test through the different setup of DAGs.

Will also check the PR and come back to you.

[dabla]

*closes: #61197

To be honest, this PR is only a draft. From the Code I can see that Authority can be retrieved from connection as an extra parameter. However, there is no information about it in the documentation.

So, this is a little bit confusing when using the proxy. So I propose to update the logic of a function to_msal_proxies. Return proxies attribute even when the variable authority is not filled.

Testing In our environment I did a functional logic test with and without proxy. It works as expected

msal_proxies is only supposed to return proxies if an authority is defined, otherwise I don't think it's supposed to return any proxies.

[dabla]

I would leave the logic as is, but the addition of tests is a very good think though, so I would keep the PR, revert the msal proxies logic to original, and you'll probably need to adapt assert of one test when authority is None.

[morelgeorge]

Hello @dabla ,
thank you for the response!
If I understand this correctly we are not able to use a proxy connectivity without authority extra parameter in Power BI connection?
Originally, when you are connecting to a PowerBI with the proxies parameter set, the code is sending proxies dict to this method and transforms it automatically to None.

In our internal testing this update worked fine even without authority attribute in connection. But maybe I am missing the whole logic, but method get_credentials in the same msgraph.py hook file is using msal_proxies value for ClientSecretCredential:

return ClientSecretCredential(
tenant_id=tenant_id,
client_id=login, # type: ignore
client_secret=password, # type: ignore
authority=authority,
proxies=msal_proxies,
disable_instance_discovery=disable_instance_discovery,
connection_verify=verify,
)

[morelgeorge]

Hello @dabla,
I am able to connect through msal proxy parameter. I had to add following extra parameter to the connection:

"authority": "login.microsoftonline.com"

After that proxy authentication is working well even with the original code. However I would recommend to update a documentation when the client wants to use a proxy connectivity. It is a little bit confusing how the proxies are transformed and finally passed when connecting to a Microsoft services in my opinion. You have to go deep to the code to understand whole logic.

I updated the comment in the linked issue. If you want I am open to update the unit tests in this PR. :-)

[dabla]

Hello @dabla, I am able to connect through msal proxy parameter. I had to add following extra parameter to the connection:

"authority": "login.microsoftonline.com"

After that proxy authentication is working well even with the original code. However I would recommend to update a documentation when the client wants to use a proxy connectivity. It is a little bit confusing how the proxies are transformed and finally passed when connecting to a Microsoft services in my opinion. You have to go deep to the code to understand whole logic.

I updated the comment in the linked issue. If you want I am open to update the unit tests in this PR. :-)

Hello @morelgeorge thank you for the explanation, very well investigation. We can keep the PR as is, after some thought I think the authority check doesn't make sense indeed and is confusing, because even if you don't define it, the default Microsoft one will be applied so your PR is fine. Thanks again!

[dabla]

You'll have to fix static checks though, then I think we can merge it.

[eladkal]

Can you please amend commit message to describe the bug you fix? We generate release notes from commit messages so it's important.