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.

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

Stop adding passwords to passwordless authtokens during updatePasswords() #30895

Closed

Conversation

mdoggydog
Copy link

Fixes #30894 (at least, it is supposed to).

Signed-off-by: Matt Marjanovic [email protected]

@szaimen szaimen added the 3. to review Waiting for reviews label Jan 28, 2022
@szaimen szaimen added this to the Nextcloud 24 milestone Jan 28, 2022
@szaimen szaimen requested review from nickvergessen, ChristophWurst, a team, PVince81 and icewind1991 and removed request for a team January 28, 2022 08:37
…rds()`

Fixes nextcloud#30894 (at least, it is supposed to).

Signed-off-by: Matt Marjanovic <[email protected]>
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
This was referenced Apr 7, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@PVince81 PVince81 requested a review from CarlSchwan June 10, 2022 15:00
@ChristophWurst
Copy link
Member

I'm not entirely sure if we want passwordless tokens to remain passwordless. IIRC the idea once was that by injecting a password from another login these token upgrade to a full-fledged token that also works with external storage and friends.

Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Fix some bugs

@CarlSchwan
Copy link
Member

/backport to stable24

@CarlSchwan
Copy link
Member

/backport to stable23

@CarlSchwan
Copy link
Member

/backport to stable22

Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Actually, @ChristophWurst is right, this might cause some issues in case some user is using the clients to access external resources

@mdoggydog Could you test if this PR also solves your issue? #33110 you will need to set the 'auth.storeCryptedPassword' option in your config.php to false

@mdoggydog
Copy link
Author

Actually, @ChristophWurst is right, this might cause some issues in case some user is using the clients to access external resources

Yes, I see that now. Originally I was only considering that captured passwords were used for access to email services --- and did not see any reason for an authtoken for an app to need to facilitate email access. I didn't consider external storage.

@mdoggydog Could you test if this PR also solves your issue? #33110 you will need to set the 'auth.storeCryptedPassword' option in your config.php to false

I'm not sure that this will help, because this would prevent all authtokens from storing captured passwords ...which would prevent capturing passwords needed for access to external email accounts, right? As @ChristophWurst alluded to in #30894 (comment), the real problem here is likely other bugs in the authtoken managment; this PR and #33110 provide detours around those bugs, but don't actually fix them. (I still need to respond to his comments there.)

For what it's worth, I do think being able to turn off password capture altogether is great. It is good to be able to ensure that capture will never happen unintentionally (if one knows the password-capture mechanism should never be needed on a particular NC instance).

I will try to try out #33110 later this week; I need to think about a way to experiment without invalidating everyone's authtokens (which is incredibly annoying to everyone).

This was referenced Aug 12, 2022
@blizzz blizzz mentioned this pull request Aug 24, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv
Copy link
Member

I will try to try out #33110 later this week; I need to think about a way to experiment without invalidating everyone's authtokens (which is incredibly annoying to everyone).

Any news? :)

If not, as there is no feedback since a while, I will close this ticket:see_no_evil:
If you will decide to work on this feature again and if it hasn't been fixed or implemented already, feel free to rebase and solve the various conflicts.

Thanks for the interest in Nextcloud and the effort put into this! 🙇

@skjnldsv skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Feb 27, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Feb 27, 2024
@skjnldsv skjnldsv marked this pull request as draft February 27, 2024 16:40
@susnux susnux added this to the Nextcloud 30 milestone Apr 18, 2024
@skjnldsv
Copy link
Member

skjnldsv commented May 2, 2024

Closing this pull request due to lack of recent activity and updates. We appreciate your contribution and encourage you to reopen or provide further updates if necessary. ☺️
Our aim is to keep the project moving forward with active collaboration. Thank you for your understanding and continued support! 🙏

@skjnldsv skjnldsv closed this May 2, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress backport-request stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Passwordless authtokens should remain passwordless
8 participants