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 postLogout hook to finish sessions from external session managers #27048

Merged
merged 3 commits into from Feb 6, 2017

Conversation

Projects
None yet
5 participants
@felixrupp
Contributor

felixrupp commented Jan 29, 2017

Description

Added a second hook emit in the logout method in \OC\User\Session to achieve a cleaner logout routine, when using external session managers, like CAS.

Related Issue

Issue 26692

Motivation and Context

When using external session managers, owncloud logout only allows to execute my own code before the oc session is terminated. Some of the mentioned session managers must be called after local sessions are terminated, otherwise the oc session is not terminated correctly.

How Has This Been Tested?

The new hook has been testet on an Ubuntu 14 installation with PHP 5.6 and Owncloud 9.1.3-1.1 and a CAS session management via user_cas Application.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jan 29, 2017

@felixrupp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @butonic and @PVince81 to be potential reviewers.

@felixrupp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @butonic and @PVince81 to be potential reviewers.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Jan 29, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Jan 29, 2017

CLA assistant check
All committers have signed the CLA.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 30, 2017

Member

Thanks.

@PhilippSchaffrath please review

Member

PVince81 commented Jan 30, 2017

Thanks.

@PhilippSchaffrath please review

@PVince81 PVince81 added this to the 10.0 milestone Jan 30, 2017

@phisch

phisch approved these changes Feb 1, 2017

@phisch

This comment has been minimized.

Show comment
Hide comment
@phisch

phisch Feb 1, 2017

Contributor

Files changed looks good, but there are 2 commits containing the same changes.

  • Squash required

@DeepDiver1975 should we backport this?

Contributor

phisch commented Feb 1, 2017

Files changed looks good, but there are 2 commits containing the same changes.

  • Squash required

@DeepDiver1975 should we backport this?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 6, 2017

Member

We usually don't backport features or API changes. Adding a hook like this is like adding a new API, so I'd say no.

I'll squash on merge.

Member

PVince81 commented Feb 6, 2017

We usually don't backport features or API changes. Adding a hook like this is like adding a new API, so I'd say no.

I'll squash on merge.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 6, 2017

Member

@felixrupp can you sign the CLA ? #27048 (comment)

Member

PVince81 commented Feb 6, 2017

@felixrupp can you sign the CLA ? #27048 (comment)

@felixrupp

This comment has been minimized.

Show comment
Hide comment
@felixrupp

felixrupp Feb 6, 2017

Contributor

@PVince81 Sorry, I signed the CLA, but my commit-e-mail-adress was not linked with github. Should be signed by now!

Contributor

felixrupp commented Feb 6, 2017

@PVince81 Sorry, I signed the CLA, but my commit-e-mail-adress was not linked with github. Should be signed by now!

@PVince81 PVince81 merged commit d6b06c9 into owncloud:master Feb 6, 2017

4 checks passed

Scrutinizer 5 new issues
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment