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

feat: experimental transfer sharepoint authentication to AzureAD #326

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

mateuszkuprowski
Copy link
Contributor

Refined authentication method to get rid of deprecated login/password with sharepoint client in favour of EntraID auth.

permissions_config = SharepointPermissionsConfig(
permissions_application_id=None,
permissions_tenant=tenant,
permissions_client_cred=None, # or SecretStr(...)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we provide with a value for permissions_client_cred and permissions_application_id here? otherwise this integration test would fail in CI pipeline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't actually fail (and it doesn't) as this connector can work in 2 modes, either with or without permissions. There's a separate e2e test for that in the test_e2e/dest/sharepoint.sh and sharepoint_with_permissions.sh

)
token_result = app.acquire_token_for_client(
scopes=[
f"https://{self.permissions_config.permissions_tenant}.sharepoint.com/.default"
Copy link
Contributor

Choose a reason for hiding this comment

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

when I use SHAREPOINT_PERMISSIONS_TENANT value in Keeper, this line reports an error. After I remove the second part of the string and only keeps the first part, it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might need a little more explanation on this not sure which part do you mean? Like it works without the /.default/ but it doesn't with it?

@bryan-unstructured
Copy link
Contributor

I've taken the route of minimal changes to the whole structure. I've been reading and experimenting a bit and came up with this solution. It feels elegant and should cover what we need - changed the authorisation method without changing the whole code structure. The doownside of this is that tenant_id which is required when using sharepoint's REST API had to be taken from permissions, which is fine for tests, but we'd have to ensure that it works correctly with entire system via UI as well. Hence I'm asking for you opinion - can we stick with it as is or should we maybe create separate parameter for tenant_id besides permissions? That'd most likely duplicate which isn't ideal either, but would have to be this way since permissions are using graph api instead of REST api that has different capabilities.

on the platform UI, what's differences between permissions and AccessConfig? Aren't both of them shown on the UI?

@mateuszkuprowski
Copy link
Contributor Author

mateuszkuprowski commented Jan 10, 2025

I've taken the route of minimal changes to the whole structure. I've been reading and experimenting a bit and came up with this solution. It feels elegant and should cover what we need - changed the authorisation method without changing the whole code structure. The doownside of this is that tenant_id which is required when using sharepoint's REST API had to be taken from permissions, which is fine for tests, but we'd have to ensure that it works correctly with entire system via UI as well. Hence I'm asking for you opinion - can we stick with it as is or should we maybe create separate parameter for tenant_id besides permissions? That'd most likely duplicate which isn't ideal either, but would have to be this way since permissions are using graph api instead of REST api that has different capabilities.

on the platform UI, what's differences between permissions and AccessConfig? Aren't both of them shown on the UI?

Unfortunately there's no permission config to be setup in the UI. All I need is tenant for this to work. Actually you indirectly input a tenant when you give link to the proper site, I was wondering if simple copy that info to tenant wouldn't work. In tests it's exactly like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants