fix IDP-initiated Logout

[DylannCordel]

cf #161

With those websites configured to use SSO:

• IDP: the IDP / SSO service
• N: nextcloud instance
• O : an other website using the SSO

When user disconnect from site O, it send a request to IDP. IDP send a request to N (and all other websites where current user as a session opened) to disconnect the user on those websites too.

This PR fix this behaviour which was not working because of CRSF check which can not be valid in this scenario: validation must be done via the validation of the IDP SAML request.

[rullzer]

Thanks for this @DylannCordel cool stuff

[rullzer]

@schiessle @blizzz if you test with onelogin be sure to configure it so that the SLO url configured in one login points to <server>/apps/user_saml/saml/sls then you can see it in action

[blizzz]

looks good, didn't test though

[schiessle]

logic looks good, but didn't tested it yet.

One security concern: We accept now logout request from everywhere without verifying if it is a valid logout request. I wonder if we could use the data from the 'SAMLRequest' parameter to verify if it is a valid request (https://github.com/nextcloud/user_saml/pull/334/files#diff-08ab8d6da860d4cc7a5530b6414b0f6eR323)?

[DylannCordel]

@schiessle you right. Last commit should solve this problem. I investigate a little deeper in code and saw processSLO was able to not perform the redirect itself. Now, session logout will only be called if performSLO was a success (that means IDP request was valid).

I still have a problem with a loop login redirection when disconnecting from nextcloud (but not from IDP, and it works if I reload the page 😱...) due to if(!$this->request->passesStrictCookieCheck()) {, from beforeController in SecurityMiddleWare.php but I believe it's not related to this issue. I'll confirm that today and create an other issue and PR if needed.

[skjnldsv]

Hey there!
What is the status?
Can I help somehow? :)

[DylannCordel]

just merged with master to stay up to date for testing.
How can I avoid the « You have N commits incorrectly signed off. To fix, head to your local branch and run: » error for DCO check ?

[skjnldsv]

@DylannCordel you need to sign your commits 😉

[skjnldsv]

just merged with master to stay up to date for testing.

It would have been better to rebase to master instead of pulling master :)

[DylannCordel]

just corrected noise I introduced in commits 😉
Thanks @skjnldsv for your advices.

[skjnldsv]

Testing on forks seems broken, I created a potential fix #338

Pushed and rebased!

Updated

[skjnldsv]

Tests are greeeen :D
@blizzz @schiessle I'll let you give the green light to merge!

@DylannCordel do you think you could add a few tests for your changes?

[ghost]

Thanks a lot @DylannCordel for tackling this issue. I tested in on my instance and it works just fine!
I had one more issue with my IdP (Keycloak), that was already reported here: #82. I prepared a fix/hack for this on top of this change (fschrempf@c8f6021) and will send a PR for it as soon as this one was merged.

[skjnldsv]

@DylannCordel let's get this in?

[DylannCordel]

@fri-sch thanks for feed-back.

@skjnldsv yes I can check how tests works in Nextcloud and try to add some soon. Concerning modifications of @fri-sch it looks good. How do you want to proceed ? Do I cherry-pick its commit to add it on the PR (or maybe @fri-sch can PR into webu/user_saml repo ?) or do you want two distinct PR on nextcloud/user_saml ?

[skjnldsv]

@DylannCordel can you open a pr against this one?

[DylannCordel]

@skjnldsv I'm sorry, I don't understand what I must do:

1. is it for tests you would like a new PR based on this one ?
2. or is it for @fri-sch 's commit ? In this case, is'n't it better if it's @fri-sch who creates a PR based on my branch ?

Sorry for time loss 😳

[skjnldsv]

Sorry for time loss

No worries :)
I think I misunderstood. I thought you were asking if you should add the tests here or in another pr (which is fine to have here as well, but if you really wanted to separate the two, you could have created another pr to merge in thi sone).

Regarding @fri-sch's pr, both are fine really.
We can merge this one and have another pr for the rest of the work :)

[skjnldsv]

Done, let's not wait too long :)
@fri-sch could you open your pull request?

[ghost]

@skjnldsv I opened the PR: #340