-
Notifications
You must be signed in to change notification settings - Fork 14
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
Added single log out for auth_saml #27
Conversation
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.
I believe this is very much work in progress. I could look into adding sessions in a easy way (most likely starlettes session middleware will do the job) to provide the userdata throughout the app. Were you able to look at testing the already newly introduced endpoints and the other things I mentioned?
# sso_built_url = auth.login() | ||
# request.session['AuthNRequestID'] = auth.get_last_request_id() | ||
# return redirect(sso_built_url) | ||
elif 'sso2' in request.query_params: |
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.
This should be tested.
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.
Done
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.
I added a comment in the test. I don't quite agree that the test brings any value since it just repeats what's done in sso endpoint. In order to show the difference in a test you would also need to test the added attrs
on calling with the sso2 param, while you just send a different url into the single sign on method (and there you can send anything, so not really an additional case in my eyes).
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.
The add attrs are outside of the sso function, will think about how to test this.
Thanks very much for your comments. Will look out soon.
Cheers,
Tracy
…On Sat, 3 Jul 2021, 10:03 Matthias Osswald, ***@***.***> wrote:
Assigned #27 <#27> to
@TracyWR <https://github.com/TracyWR>.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#27 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGK5YXPXSR5TNOHIIJYTMW3TV3G6DANCNFSM47U5CJDQ>
.
|
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.
I looked at how to make use of sessions. I checked out different approaches but ended up with the starlette implementation.
from starlette.middleware.sessions import SessionMiddleware
...
app = FastAPI()
app.add_middleware(OPAMiddleware, config=opa_config)
app.add_middleware(SessionMiddleware, secret_key="very-secret", max_age=3600)
Within the app the session keys can easily be gotten/set by just changing the value for a key:
request.session["user_session"] = "{'some': 'dict', 'as': 'string'}"
request.session.get("user_session")
I think this will be a good solution since it does not require additional dependencies, is very easy to handle in the opa middleware and doesn't require a interface change.
876c481
to
8004b91
Compare
# sso_built_url = auth.login() | ||
# request.session['AuthNRequestID'] = auth.get_last_request_id() | ||
# return redirect(sso_built_url) | ||
elif 'sso2' in request.query_params: |
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.
I added a comment in the test. I don't quite agree that the test brings any value since it just repeats what's done in sso endpoint. In order to show the difference in a test you would also need to test the added attrs
on calling with the sso2 param, while you just send a different url into the single sign on method (and there you can send anything, so not really an additional case in my eyes).
a2d2215
to
dab4ef7
Compare
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.
I did not yet setup the complete environment, but I created a little POC to show how the session indeed should be working just fine.
Let's define an in-package app like this:
# fastapi_opa/main.py
from typing import Dict
from fastapi import FastAPI
from starlette.middleware.sessions import SessionMiddleware
from starlette.requests import Request
from fastapi_opa import OPAConfig
from fastapi_opa import OPAMiddleware
from fastapi_opa.auth.auth_saml import SAMLAuthentication
from fastapi_opa.auth.auth_saml import SAMLConfig
opa_host = "http://localhost:8181"
# In this example we use OIDC authentication flow (using Keycloak)
saml_config = SAMLConfig(settings_directory="./tests/test_data/saml")
saml_auth = SAMLAuthentication(saml_config)
opa_config = OPAConfig(authentication=saml_auth, opa_host=opa_host)
app = FastAPI()
app.add_middleware(OPAMiddleware, config=opa_config)
app.add_middleware(SessionMiddleware, secret_key="secret", max_age=24*60*60)
@app.get("/")
async def root(request: Request) -> Dict:
return {
"msg": request.session.get("foo"),
}
And now change the opa middleware into a simple example of using the session:
# fastapi_opa.opa.opa_middleware.py
import asyncio
import json
import logging
from json.decoder import JSONDecodeError
import requests
from fastapi.responses import JSONResponse
from starlette.requests import Request
from starlette.responses import RedirectResponse
from starlette.types import ASGIApp
from starlette.types import Receive
from starlette.types import Scope
from starlette.types import Send
from fastapi_opa.auth.exceptions import AuthenticationException
from fastapi_opa.opa.opa_config import OPAConfig
logger = logging.getLogger(__name__)
class OPAMiddleware:
def __init__(self, app: ASGIApp, config: OPAConfig) -> None:
self.config = config
self.app = app
async def __call__(
self, scope: Scope, receive: Receive, send: Send
) -> None:
request = Request(scope, receive, send)
if "bar" in request.query_params:
return await self.app(scope, receive, send)
request.session["foo"] = "some data"
return await RedirectResponse("?bar").__call__(
scope, receive, send
)
# end of file (for the POC)
Now additionally add uvicorn (just to run the example). If you start your app running poetry run uvicorn fastapi_opa.main:app
and visit /
a session is set with the key foo
and the data some-data
. Then the client is redirected to ?bar
. On hitting ?bar
the client will actually see the root
route of fastapi, which checks what's in request.session.get("foo")
and displays it in it's json msg
. This also works within the middleware (just try printing it or so...).
The same works within the saml auth injectable. Feel free to add the session middleware for the test client since this will soon become a requirement for OIDC as well.
errors = saml_settings.validate_metadata(metadata) | ||
status_code = 200 | ||
if len(errors) != 0: | ||
metadata = ', '.join(errors) |
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.
I'm not sure about the flow specs here, or how you are planning to use this method (right now it is not used at all). But exposing the errors is a security issue usually.
assert response.status_code == 307 | ||
|
||
|
||
async def async_return(result): |
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.
Where is this method used?
|
||
elif 'sls' in request.query_params: | ||
logger.debug('--sls--') | ||
return await self.single_log_out_from_IdP(request) |
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.
Please rename according to pep8 (self.single_log_out_from_idp
).
@@ -39,18 +62,47 @@ async def init_saml_auth(self, request_args: Dict) -> OneLogin_Saml2_Auth: | |||
) | |||
|
|||
@staticmethod | |||
async def single_sign_on(auth: OneLogin_Saml2_Auth) -> RedirectResponse: | |||
redirect_url = auth.login() | |||
async def single_log_out_from_IdP(request: Request) -> \ |
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.
If not explicitly required by black, please do now use \
here but wrap with parenthesis.
} | ||
|
||
async def get_metadata(self, request: Request): | ||
saml_settings = OneLogin_Saml2_Settings(custom_base_path=self.custom_folder, |
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.
According to the type checker I think this should be self.custom_folder.as_posix()
instead.
This might be related (SAML-Toolkits/python-saml#146) but I was not yet able to dig into the issue much. |
I have changed sls method, now it works |
Added single log out for auth_saml
Learning
Describe the research stage
Links to blog posts, patterns, libraries or addons used to solve this problem