Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(Session): avoid password confirmation on SSO #43942

Merged
merged 2 commits into from
Jun 7, 2024
Merged

fix(Session): avoid password confirmation on SSO #43942

merged 2 commits into from
Jun 7, 2024

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Mar 1, 2024 •
edited
Loading

Summary

SSO backends like SAML and OIDC tried a trick to suppress password confirmations as they are not possible by design. At least for SAML it was not reliable when existing user backends where used as user repositories.

Now we are setting a special scope with the token, and also make sure that the scope is taken over when tokens are regenerated.

The root source might be elsewhere actually… The last-password-confirm value in the session that both SAML and OIDC store should persists (otherwise, if session data was lost, the SAML user would be logged out). It might be overwritten from elswhere, though I could not find a good candidate either. Anyway, modifying that timestamp in the session was a hack from the start.

Todos

  • In PasswordConfirmationMiddleware, confirm excludedUserBackEnds can go away and remove it

Checklist

@blizzz blizzz added this to the Nextcloud 29 milestone Mar 1, 2024
@blizzz
Copy link
Member Author

blizzz commented Mar 1, 2024

@ArtificialOwl do you think it works with GSS, there is also an IApacheBackend implemented, or would this be tricky?

@nickvergessen
Copy link
Member

The scopes are for "what to access". Bringing in the origin into it sounds like abuse?

@blizzz
Copy link
Member Author

blizzz commented Mar 1, 2024

The scopes are for "what to access". Bringing in the origin into it sounds like abuse?

You could argue that, definitely. Extending the Tokens would break API though.

@blizzz blizzz force-pushed the fix/43612/avoid-pwd-confirm-sso branch from 6c63a57 to 9ed2917 Compare March 4, 2024 16:25
@juliushaertl
Copy link
Member

The scopes are for "what to access". Bringing in the origin into it sounds like abuse?

You could also argue that this new scope allow you to access password confirmation requiring endpoints without the password.

juliushaertl
juliushaertl reviewed Mar 4, 2024
View reviewed changes
lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php Outdated
$sessionId = $this->session->getId();
$token = $this->tokenProvider->getToken($sessionId);
$scope = $token->getScopeAsArray();
if (isset($scope['sso-based-login']) && $scope['sso-based-login'] === true) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we actually make those strings constants (also the only other scope filesystem) and put them to IToken so that we have somehow documented what is in use?

Copy link
Member Author

@blizzz blizzz Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, yes, good idea. But perhaps only 29+.

This was referenced Mar 12, 2024
Merged
Merged
@blizzz blizzz self-assigned this Mar 15, 2024
@blizzz blizzz force-pushed the fix/43612/avoid-pwd-confirm-sso branch 3 times, most recently from 9c4d59e to e050f03 Compare March 15, 2024 12:00
This was referenced Mar 18, 2024
Merged
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
Merged
81 tasks
artonge
artonge approved these changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Good enough for me with nitpick

lib/private/Template/JSConfigHelper.php Outdated Show resolved Hide resolved
@blizzz blizzz added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 4, 2024
@blizzz blizzz force-pushed the fix/43612/avoid-pwd-confirm-sso branch from 416acb9 to ab94c02 Compare June 5, 2024 10:37
artonge
artonge approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Good for me

@blizzz
fix(Session): avoid password confirmation on SSO
340939e 
SSO backends like SAML and OIDC tried a trick to suppress password
confirmations as they are not possible by design. At least for SAML it was
not reliable when existing user backends where used as user repositories.

Now we are setting a special scope with the token, and also make sure that
the scope is taken over when tokens are regenerated.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
refactor(Token): introduce scope constants
f6d6efe 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/43612/avoid-pwd-confirm-sso branch from ab94c02 to f6d6efe Compare June 5, 2024 17:01
@blizzz
Copy link
Member Author

blizzz commented Jun 5, 2024

/backport 340939e to stable29

@blizzz
Copy link
Member Author

blizzz commented Jun 5, 2024

/backport 340939e to stable28

@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 5, 2024
ChristophWurst
ChristophWurst approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Change makes sense, code is clean (even split between a fix and a refactoring) but did not test

:shipit:

@blizzz
Copy link
Member Author

blizzz commented Jun 7, 2024

/backport 340939e to stable27

@blizzz blizzz merged commit 98b5cdc into master Jun 7, 2024
155 checks passed
@blizzz blizzz deleted the fix/43612/avoid-pwd-confirm-sso branch June 7, 2024 09:25
This was referenced Jun 7, 2024
Merged
Merged
Merged
@blizzz blizzz mentioned this pull request Jun 12, 2024
Merged
@biredel biredel mentioned this pull request Jun 20, 2024
Closed
8 tasks
@blizzz blizzz mentioned this pull request Jun 24, 2024
Closed
8 tasks
@adsche adsche mentioned this pull request Jun 28, 2024
Open
8 tasks
@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label Jul 18, 2024
This was referenced Jul 19, 2024
Merged
Merged
@pierreozoux pierreozoux mentioned this pull request Aug 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@joshtrichards joshtrichards joshtrichards left review comments

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@artonge artonge artonge approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@julien-nc julien-nc Awaiting requested review from julien-nc

@nc-fkl nc-fkl Awaiting requested review from nc-fkl

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

Assignees

@blizzz blizzz

Labels
3. to review Waiting for reviews bug feature: authentication pending documentation This pull request needs an associated documentation update
Projects
📝 Office team
Archived in project
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

[Bug]: Can not confirm my password for administrative actions when logged in via LDAP / SAML
8 participants
@blizzz @nickvergessen @juliushaertl @Keeper-of-the-Keys @ChristophWurst @joshtrichards @artonge @skjnldsv