-
Notifications
You must be signed in to change notification settings - Fork 62
Enable rbac #264
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
base: master
Are you sure you want to change the base?
Enable rbac #264
Conversation
|
Thanks a lot for your contribution! I have to say I'm not sure adding |
|
Yes of course. For internal enterprise applications imo this is by far the most used method anyway so if someone wants it somewhere else I think it is a good idea to wait for that request when it happens. Just one clarification, I do think is great to directly get an openid object without having to parse the response in the desired application. Therefore, do you mean that the method is implemented in the Microsoft provider and the property roles is filled in, and in the other is just missing or always None for the time being ? My comment at the end was that the current logic flow, would benefit for not necessarily having this split between where the information is taken from, the token, the info endpoint or another... It will be difficult to extend if information lives in different places/endpoints as the current logic does not aggregate any information, it currently is either or. |
| import pydantic | ||
|
|
||
| from fastapi_sso.sso.base import DiscoveryDocument, OpenID, SSOBase | ||
| from fastapi_sso.sso.base import DiscoveryDocument, OpenID, SSOBase, _decode_id_token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make _decode_id_token public if we need it elsewhere 👌
|
|
||
| async def get_user_roles(self) -> list[str]: | ||
| """Get user roles from Microsoft ID token.""" | ||
| token_info = _decode_id_token(self._id_token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_id_token may be None, please doublecheck the failed workflows
This enables adding the role assignment from EntraID to the OpenID response. In general I think the way this code work for getting the user information either from the token or the user info endpoint is too restrictive and won't allow to extend to further functionality when trying to include information from another endpoint. Like in this case