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

Require the secret config to be configured #31492

Merged
merged 3 commits into from
Apr 26, 2022
Merged

Require the secret config to be configured #31492

merged 3 commits into from
Apr 26, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Mar 8, 2022 •
edited
Loading

If it's not configured the instance will look like it is working but
various features will silently break (end to end encryption, setting
alternate email and probably more).

One issue is that changing the secret from empty to something will
break various other stuff (app token). I don't think there is a good way
to solve this issue other than breaking early instead of having to
handle a painful migration later on.

image

@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Mar 8, 2022
@CarlSchwan CarlSchwan added this to the Nextcloud 24 milestone Mar 8, 2022
@CarlSchwan CarlSchwan requested a review from a team March 8, 2022 12:53
@CarlSchwan CarlSchwan self-assigned this Mar 8, 2022
@CarlSchwan CarlSchwan requested review from blizzz, skjnldsv, come-nc and juliusknorr and removed request for a team March 8, 2022 12:53
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Mar 8, 2022 •
edited
Loading

This breaks unit tests since we don't have a secret yet when doing occ commands. I'll move this code somewhere else Probably fixed and moved the code to the OC_Util::checkServer this is also more correct :)

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 8, 2022
@szaimen
Copy link
Contributor

szaimen commented Mar 8, 2022

Still breaking many tests?

@juliusknorr
Copy link
Member

Generally nice to have this checked 👍 We just should probably not backport and only show a setup warning on stable releases so that admins already get aware that this might be an issue.

The one failing test case looks interesting, I'm wondering a bit why the occ installation does not add the secret then:
Screenshot 2022-03-08 at 16 13 38

@CarlSchwan
Require the secret config to be configured
7496bf3 
If it's not configured the instance will look like it is working but
various features will silently break (end to end encryption, setting
alternate email and probably more).

One issue is that changing the secret from empty to something will
break various other stuff (app token). I don't think there is a good way
to solve this issue other than breaking early instead of having to
handle a painful migration later on.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@szaimen
Copy link
Contributor

szaimen commented Mar 8, 2022

only show a setup warning on stable releases so that admins already get aware that this might be an issue.

shall I create a setup check for this?

@szaimen
Copy link
Contributor

szaimen commented Mar 8, 2022 •
edited
Loading

only show a setup warning on stable releases so that admins already get aware that this might be an issue.

shall I create a setup check for this?

I mean a setup check could be an alternative approach to this (instead of breaking hard)...

juliusknorr
juliusknorr reviewed Mar 9, 2022
View reviewed changes
tests/travis/install.sh Outdated Show resolved Hide resolved
@CarlSchwan
Also check for instanceid and passwordhash
e6161af 
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan
Only check if secret is set if the nextcloud is installed
0e58c11 
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@szaimen
Copy link
Contributor

szaimen commented Mar 10, 2022

only show a setup warning on stable releases so that admins already get aware that this might be an issue.

shall I create a setup check for this?

ping

@blizzz
Copy link
Member

blizzz commented Mar 10, 2022

state is still set to "developing"?

@juliusknorr juliusknorr mentioned this pull request Mar 10, 2022
Merged
3 tasks
@CarlSchwan
Copy link
Member Author

only show a setup warning on stable releases so that admins already get aware that this might be an issue.

shall I create a setup check for this?

ping

The idea is to have a migration path, see #31499

@szaimen
Copy link
Contributor

szaimen commented Mar 14, 2022

The idea is to have a migration path, see #31499

nice 👍

@szaimen
Copy link
Contributor

szaimen commented Mar 14, 2022

So merge then?

@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Merged
@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
This was referenced Apr 7, 2022
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Apr 26, 2022
@skjnldsv skjnldsv merged commit 036f871 into master Apr 26, 2022
@skjnldsv skjnldsv deleted the fix/check-secret-configured branch April 26, 2022 15:28
@solracsf solracsf mentioned this pull request Sep 12, 2022
Open
9 tasks
@joshtrichards joshtrichards mentioned this pull request Jun 27, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr left review comments

@come-nc come-nc come-nc approved these changes

@szaimen szaimen szaimen approved these changes

@blizzz blizzz Awaiting requested review from blizzz

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees

@CarlSchwan CarlSchwan

Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
6 participants
@CarlSchwan @szaimen @juliusknorr @blizzz @come-nc @skjnldsv