Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Saml logout #6414

Closed
wants to merge 2 commits into from
Closed

Saml logout #6414

wants to merge 2 commits into from

Conversation

mjattiot
Copy link

@mjattiot mjattiot commented Nov 26, 2019 •
edited
Loading

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

Signed-off-by: Maxime Jattiot mjattiot@opensense.fr

@mjattiot mjattiot changed the base branch from master to develop November 26, 2019 08:26
@mjattiot mjattiot force-pushed the saml_logout branch 2 times, most recently from 9f4c012 to 9e1468d Compare November 26, 2019 10:42
@mjattiot mjattiot marked this pull request as ready for review November 26, 2019 10:45
@mjattiot mjattiot mentioned this pull request Nov 26, 2019
Open
@richvdh
Copy link
Member

richvdh commented Nov 26, 2019

@mjattiot thanks for the contribution... first step is to merge recent develop into your branch so that the CI works.

@mjattiot
Copy link
Author

Mmmh I have just rebased from develop but I still see some checks failing. Could you help me understand why it fails ?
This is the commit on which I rebased:
commit f9f1c8a (synapse/develop)
Merge: c01d543 35f9165
Author: Erik Johnston erik@matrix.org
Date: Tue Nov 26 12:56:05 2019 +0000

@richvdh
Copy link
Member

richvdh commented Nov 26, 2019

For example, look at the 'homeserver.log' under the artifacts tab at https://buildkite.com/matrix-dot-org/synapse/builds/5660#0ab2e7e5-c876-46dc-ab5f-bd3ad395a2f2. Looks like a syntax error of some sort.

@mjattiot mjattiot force-pushed the saml_logout branch 2 times, most recently from a88bf54 to 93ef60b Compare November 29, 2019 14:54
@mjattiot
Copy link
Author

mjattiot commented Nov 29, 2019 •
edited
Loading

I had some syntax errors because I was using f-strings which are not compatible with python 3.5.
However, @richvdh, I still have a strange error:
AttributeError: ('saml2_sp_config', "not found in ['server', 'tls', 'database', ..
link
I don't understand why I am getting this error. I have not changed a single line in the saml config file. Could you help me pass this test ? Sorry I am quite new to Synapse.

@richvdh richvdh self-requested a review December 2, 2019 11:37
@richvdh
Copy link
Member

richvdh commented Dec 4, 2019

The problem is the call to get_saml_handler in logout.py. This will instantiate the SamlHandler, which assumes that saml2_sp_config is set in the config file - which will not be true unless saml is enabled.

@richvdh richvdh removed their request for review December 4, 2019 10:31
@richvdh richvdh self-assigned this Dec 4, 2019
@mjattiot mjattiot force-pushed the saml_logout branch 2 times, most recently from 95dc1fa to 1b845fd Compare December 23, 2019 14:30
@mjattiot
Copy link
Author

@richvdh thank you for your insights and sorry for the response delay.
I did some changes and all tests passed ! If you have time to review some day please feel free :)

@richvdh richvdh self-requested a review January 8, 2020 09:39
richvdh
richvdh suggested changes Jan 9, 2020
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Thanks very much for continuing your work on this.

I've started some review here, but there's quite a lot here: I suggest you have a look at my comments so far.

I think you're actually implementing two different things here:

  • making a logout request to the IdP when we receive a request from a matrix client to /_matrix/client/r0/logout.
  • invalidating synapse access tokens when we receive a _matrix/saml2/logout request from the IdP
    I don't think there is any reason that we have to implement both of these at the same time, so I suggest that, to make it easier for us to work on the PR, you split up this PR so that we are just doing one at a time.

synapse/python_dependencies.py
@@ -89,7 +89,7 @@
# python 3.5.2, as per https://github.com/itamarst/eliot/issues/418
'eliot<1.8.0;python_version<"3.5.3"',
],
"saml2": ["pysaml2>=4.5.0"],
"saml2": ["pysaml2>=4.9.0"],
Copy link
Member

Choose a reason for hiding this comment

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

if there's a particular reason we need this (something not supported in 4.5?) please could you add a comment to say so, in the same way as some of the other entries?

Copy link
Author

Choose a reason for hiding this comment

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

no particular reason but 4.5.0 has been released in october 2017, and according to release notes, 4.9.0 don't change any API. So I assume it would have been better to bump the minor version for stability improvement. Up to you, I can revert

Copy link
Member

Choose a reason for hiding this comment

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

generally we don't bump our minimum required dependencies unless there is a need to solve a particular problem. Bumping the version for no reason makes things difficult for distribution package maintainers.

synapse/handlers/saml_handler.py Outdated
resp_bytes, BINDING_HTTP_REDIRECT
)
logger.info("Received SAML logout response %s", resp_saml)
if resp_saml.response.status.status_code.value == STATUS_SUCCESS:
Copy link
Member

Choose a reason for hiding this comment

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

suggest inverting this condition

Copy link
Author

Choose a reason for hiding this comment

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

what do you mean ?
if STATUS_SUCCESS == resp_saml.response.status.status_code.value: ?

mjattiot
mjattiot commented Feb 26, 2020
View reviewed changes
Copy link
Author

@mjattiot mjattiot left a comment

Choose a reason for hiding this comment

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

Hello @richvdh , sorry for the late response, I have been quite busy.
I rebased to develop, reviewed all your comments, corrected accordingly for most of them and reviewed some logic. If you have time in coming weeks, please feel free to have another pass.

synapse/handlers/saml_handler.py
# local cache. In that case we select the last session and remove
# other ones.
def _find_session_from_user(self, username):
subjects = self._saml_client.users.subjects()
Copy link
Author

@mjattiot mjattiot Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is clearly the weakness of my implementation. In case the server restarts, local sessions are lost, therefore we cannot retrieve the "session_index" of a user and we cannot proceed with a logout.
However I found this weakness acceptable in terms of user experience : if the server crashes and a user presses logout button, it won't logged him out but in the background it will restore a session index in local cache. Therefore if the user presses the button a second time the logout will work. It's an usual behavior for a user to re-press a button if the first time it doesn't work. But I admit it's not ideal.
If we want to make it works smoothly after a crash we need a way to persist this sessions cache. I am not familiar enough with Synapse to know which is the best way to implement persistence cleanly but if you show me the way I can implement it.

synapse/handlers/saml_handler.py
Comment on lines +196 to +236
except Exception as e:
raise SynapseError(
500, "error while creating SAML logout request: %s" % (e,)
)
Copy link
Author

Choose a reason for hiding this comment

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

Not sure. self._saml_client.do_logout() can raise a LogoutError we want to catch it and convert it to a Synapse Error instead.

synapse/handlers/saml_handler.py Outdated
resp_bytes, BINDING_HTTP_REDIRECT
)
logger.info("Received SAML logout response %s", resp_saml)
if resp_saml.response.status.status_code.value == STATUS_SUCCESS:
Copy link
Author

Choose a reason for hiding this comment

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

what do you mean ?
if STATUS_SUCCESS == resp_saml.response.status.status_code.value: ?

@richvdh richvdh self-requested a review February 26, 2020 22:26
richvdh
richvdh suggested changes Mar 7, 2020
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I think I've answered your questions, but again: I think this PR is quite big, and I'd like to see it split into two parts: one to handle _matrix/saml2/logout, and the other to extend /_matrix/client/r0/logout

In fact, it's not entirely clear to me that a logout request from a matrix client should be unconditionally passed to on to the saml server as a single-logout-request. Just because I want to log out of Matrix doesn't necessarily mean I want to log out of everything?

I suggest you focus on handling logout requests from the SAML server first, and then we can consider if we need the more complicated second case.

@richvdh
Copy link
Member

richvdh commented Apr 1, 2020

this is bitrotting rapidly. I'm going to close it for now. we'd still welcome contributions in this area; if you'd like to resume work we can reopen.

@richvdh richvdh closed this Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richvdh richvdh richvdh requested changes

Assignees

@richvdh richvdh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mjattiot @richvdh