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

add possibility to configure tenant id #751

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tiago-peres
Copy link

@tiago-peres tiago-peres commented Feb 9, 2023

Bug: #748

Tested locally and was able to change the Tenant ID from common to organizations with

SOCIAL_AUTH_AZUREAD_OAUTH2_TENANT_ID = "organizations"

@nijel
Copy link
Member

nijel commented Feb 9, 2023

There already is dedicated backend for tenant authentication: https://github.com/python-social-auth/social-core/blob/master/social_core/backends/azuread_tenant.py

@tiago-peres
Copy link
Author

@nijel read the bug report.
What I'm changing is not the Tenant, but the assumption that we use common in the url all the time.
That's not true and leads to errors doing that.

@tiago-peres
Copy link
Author

The reason for using is that nomenclature is the way the BASE_URL is formed

BASE_URL = "https://{authority_host}/{tenant_id}"

The {tenant_id} is what I'd like have the possibility to change, between common or organizations, depending on the type of Azure application.

@tiago-peres
Copy link
Author

can change it to signin_url and SOCIAL_AUTH_AZUREAD_OAUTH2_SIGNIN_URL if you prefer (based on Microsoft's documentation)

@codecov
Copy link

codecov bot commented Feb 26, 2023

Codecov Report

Merging #751 (6501fc0) into master (3f49832) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #751   +/-   ##
=======================================
  Coverage   77.27%   77.27%           
=======================================
  Files         325      325           
  Lines        9875     9875           
  Branches     1180     1180           
=======================================
  Hits         7631     7631           
  Misses       2088     2088           
  Partials      156      156           
Flag Coverage Δ
unittests 77.27% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
social_core/backends/azuread.py 79.68% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nijel
Copy link
Member

nijel commented Mar 1, 2023

I'd like to hear another opinion here as well. Using TENANT name for this seems confusing to me - it is not really tenant in this case, but rather a scope.

@tiago-peres
Copy link
Author

Yea... the reason to use TENANT was to make as less changes as possible to what's already there... Look forward to see a resolution though.

@nijel
Copy link
Member

nijel commented Apr 25, 2024

Instead of misusing TENANT I'd rather introduce configuration for sign-in URL, where whole URL would be specified. This way it would be consistent with Azure docs where they document which URL to use. If URL would not be configured, it would fall back to the current implementation.

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

Successfully merging this pull request may close these issues.

2 participants