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 scope to documentation #159

Merged
merged 9 commits into from
Oct 30, 2023

Conversation

nikstuckenbrock
Copy link
Contributor

I'm sorry for the mess! I've somehow managed to destroy my previous PR #106 and have created this one to fix the history problems. Sorry for the chaos. :(

I would appreciate if you could add the HACKTOBERFEST-ACCEPTED label to the pull request.

@davidhuser
Copy link
Contributor

just did a quick scan, I think it misses setting the default scope during FastAPI.app instantiation, see bottom part here #106 (comment)

@@ -185,23 +185,46 @@ We've used two flags: `usePkceWithAuthorizationCodeGrant`, which is the authenti

Now, the fun part begins! 🚀

Import the `SingleTenantAzureAuthorizationCodeBearer` from `fastapi_azure_auth` and configure it:
Import the `SingleTenantAzureAuthorizationCodeBearer` from `fastapi_azure_auth` and configure it. You can use the `Settings` class to compute the required URLs:
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick but we also compute other properties, so maybe "... compute the required fields"

f'api://{settings.APP_CLIENT_ID}/user_impersonation': 'user_impersonation',
}
scopes=settings.SCOPES,
openapi_authorization_url=settings.OPENAPI_AUTHORIZATION_URL,
Copy link
Contributor

Choose a reason for hiding this comment

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

These are new params added to SingleTenant not previously in the docs. I don't have much experience with Single Tenant and would refer to @JonasKs

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, these should be default 99% of the time. No reason to confuse with these params. 😊
If they are required for the padlock I'd say we use the azure_schemes URL in the docs instead.

Copy link
Contributor

@davidhuser davidhuser left a comment

Choose a reason for hiding this comment

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

LGTM

@JonasKs JonasKs merged commit 3ff82f7 into intility:main Oct 30, 2023
5 of 7 checks passed
@JonasKs
Copy link
Member

JonasKs commented Oct 30, 2023

Thank you so much 😊

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

Successfully merging this pull request may close these issues.

3 participants