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

Backport to stable 5.0 of https://github.com/owncloud/ocis/pull/8756 #8781

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

dragonchaser
Copy link
Member

No description provided.

Copy link

update-docs bot commented Apr 4, 2024

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Signed-off-by: Christian Richter <[email protected]>
Signed-off-by: Christian Richter <[email protected]>
Copy link

sonarcloud bot commented Apr 4, 2024

@dragonchaser dragonchaser marked this pull request as ready for review April 4, 2024 15:10
rhafer
rhafer previously requested changes Apr 4, 2024

type API struct {
ShowUserEmailInResults bool `yaml:"show_email_in_results" env:"OCIS_SHOW_USER_EMAIL_IN_RESULTS" desc:"Mask user email addresses in responses." introductionVersion:"5.1"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be 5.0.1, now that we backported it? (would need to be changed in master as well I guess)

Copy link
Member Author

@dragonchaser dragonchaser Apr 4, 2024

Choose a reason for hiding this comment

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

Thing is, it was/will be introduced in 5.1 not 5.0 it is a backport to 5.0 that does not follow the behavior it will have in 5.1 (see defaultconfig.go) so I am unsure what to actually put in there :) Let's decide tomorrow in/after the daily.

Copy link
Member

@butonic butonic Apr 5, 2024

Choose a reason for hiding this comment

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

We are sneaking in a new environment variable and default it to true for the v5.0 branch. For 5.1 we will change the default to false which will be a breaking change. It will be announced in the 5.1 release notes.

The future behavior can inofficially already be used in 5.0.1 by explicitly setting OCIS_SHOW_USER_EMAIL_IN_RESULTS=false. Yeah ... we're lying to save ourselves a breaking change ;-)

Setting introductionVersion:"5.0.1" only leads to more work. Maybe we can just add (EXPERIMENTAL) to the description, similar to how the linux kernel does it.

Co-authored-by: Jörn Friedrich Dreyer <[email protected]>
@butonic butonic dismissed rhafer’s stale review April 5, 2024 08:30

will add experimental to description and introduce in 5.1, not 5.0.1

@butonic butonic disabled auto-merge April 5, 2024 08:51
@butonic butonic merged commit 1f06142 into owncloud:stable-5.0 Apr 5, 2024
3 of 4 checks passed
@butonic
Copy link
Member

butonic commented Apr 5, 2024

ci was green, but github did not pick up the notification ... merged manually

ownclouders pushed a commit that referenced this pull request Apr 5, 2024
@mmattel
Copy link
Contributor

mmattel commented Apr 5, 2024

@dragonchaser

  • just checked:
    the envvar OCIS_SHOW_USER_EMAIL_IN_RESULTS has shown up in the docs-stable-5.0 branch for antora ✔️
  • you may delete the branch. It was not autodeleted on merge.
  • pls tell what to announce in the ocis 5 release notes additionally.
  • note that we do not have a changelog entry, intentionally...?

@dragonchaser dragonchaser deleted the backport-to-stable-5.0 branch April 5, 2024 09:28
@dragonchaser
Copy link
Member Author

@mmattel

  • it has the experimental in the description that is sufficient from my perspective.
  • done, github services were disrupted completely
  • nothing?
  • because there is no release (yet)

ownclouders pushed a commit that referenced this pull request Apr 6, 2024
ownclouders pushed a commit that referenced this pull request Apr 7, 2024
ownclouders pushed a commit that referenced this pull request Apr 8, 2024
@@ -4,3 +4,7 @@ package config
type TokenManager struct {
JWTSecret string `yaml:"jwt_secret" env:"OCIS_JWT_SECRET;OCS_JWT_SECRET" desc:"The secret to mint and validate jwt tokens." introductionVersion:"pre5.0"`
}

type API struct {
ShowUserEmailInResults bool `yaml:"show_email_in_results" env:"OCIS_SHOW_USER_EMAIL_IN_RESULTS" desc:"Mask user email addresses in responses. (EXPERIMENTAL)" introductionVersion:"5.1"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Just some stupid questions from my side:

  • isn't this supposed to go into the fronted service where the real ocs api lives? The ocs service is only about the signing-key endpoint (look at the proxy default routes).

  • but even then, isn't there a wire missing:

diff --git a/services/frontend/pkg/revaconfig/config.go b/services/frontend/pkg/revaconfig/config.go
index 0b38abaab5..5db9460c30 100644
--- a/services/frontend/pkg/revaconfig/config.go
+++ b/services/frontend/pkg/revaconfig/config.go
@@ -339,7 +339,8 @@ func FrontendConfigFromStruct(cfg *config.Config, logger log.Logger) (map[string
                                                        "productversion": version.GetString(),
                                                },
                                        },
-                                       "include_ocm_sharees": cfg.OCS.IncludeOCMSharees,
+                                       "include_ocm_sharees":   cfg.OCS.IncludeOCMSharees,
+                                       "show_email_in_results": cfg.OCS.ShowEmailInResults,
                                },
                        },
                },

Currently this defaults to never show emails in results because bools default to false. At least I couldn't get emails to show up in the search

Copy link
Contributor

Choose a reason for hiding this comment

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

Default is true, see defaultconfig.go

Copy link
Contributor

Choose a reason for hiding this comment

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

But I don't see the emails when searching for a user.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes it work for me:

diff --git a/deployments/examples/ocis_traefik/docker-compose.yml b/deployments/examples/ocis_traefik/docker-compose.yml
index 10ef857021..d279bfafbb 100644
--- a/deployments/examples/ocis_traefik/docker-compose.yml
+++ b/deployments/examples/ocis_traefik/docker-compose.yml
@@ -46,7 +46,7 @@ services:
     restart: always
 
   ocis:
-    image: owncloud/ocis:${OCIS_DOCKER_TAG:-latest}
+    image: owncloud/ocis:dev
     networks:
       ocis-net:
     entrypoint:
diff --git a/services/frontend/pkg/config/config.go b/services/frontend/pkg/config/config.go
index 9cae9ab91a..c0dfdafc28 100644
--- a/services/frontend/pkg/config/config.go
+++ b/services/frontend/pkg/config/config.go
@@ -145,6 +145,7 @@ type OCS struct {
        PublicShareMustHavePassword          bool               `yaml:"public_sharing_share_must_have_password" env:"OCIS_SHARING_PUBLIC_SHARE_MUST_HAVE_PASSWORD;FRONTEND_OCS_PUBLIC_SHARE_MUST_HAVE_PASSWORD" desc:"Set this to true if you want to enforce passwords on all public shares." introductionVersion:"5.0"`
        WriteablePublicShareMustHavePassword bool               `yaml:"public_sharing_writeableshare_must_have_password" env:"OCIS_SHARING_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD;FRONTEND_OCS_PUBLIC_WRITEABLE_SHARE_MUST_HAVE_PASSWORD" desc:"Set this to true if you want to enforce passwords on Uploader, Editor or Contributor shares." introductionVersion:"5.0"`
        IncludeOCMSharees                    bool               `yaml:"include_ocm_sharees" env:"FRONTEND_OCS_INCLUDE_OCM_SHAREES" desc:"Include OCM sharees when listing sharees." introductionVersion:"5.0"`
+       ShowUserEmailInResults               bool               `yaml:"show_user_email_in_results" env:"OCIS_SHOW_USER_EMAIL_IN_RESULTS" desc:"Mask user email addresses in responses. (EXPERIMENTAL)" introductionVersion:"5.1"`
 }
 
 type CacheWarmupDrivers struct {
diff --git a/services/frontend/pkg/config/defaults/defaultconfig.go b/services/frontend/pkg/config/defaults/defaultconfig.go
index 5a9b81c9ac..ae99b4aadd 100644
--- a/services/frontend/pkg/config/defaults/defaultconfig.go
+++ b/services/frontend/pkg/config/defaults/defaultconfig.go
@@ -116,6 +116,7 @@ func DefaultConfig() *config.Config {
                        ListOCMShares:               true,
                        PublicShareMustHavePassword: true,
                        IncludeOCMSharees:           false,
+                       ShowUserEmailInResults:      true,
                },
                Middleware: config.Middleware{
                        Auth: config.Auth{
diff --git a/services/frontend/pkg/revaconfig/config.go b/services/frontend/pkg/revaconfig/config.go
index 0b38abaab5..9226737782 100644
--- a/services/frontend/pkg/revaconfig/config.go
+++ b/services/frontend/pkg/revaconfig/config.go
@@ -339,7 +339,8 @@ func FrontendConfigFromStruct(cfg *config.Config, logger log.Logger) (map[string
                                                        "productversion": version.GetString(),
                                                },
                                        },
-                                       "include_ocm_sharees": cfg.OCS.IncludeOCMSharees,
+                                       "include_ocm_sharees":   cfg.OCS.IncludeOCMSharees,
+                                       "show_email_in_results": cfg.OCS.ShowUserEmailInResults,
                                },
                        },
                },
diff --git a/services/ocs/pkg/config/config.go b/services/ocs/pkg/config/config.go
index ed7a0ab96e..4097ff4ef6 100644
--- a/services/ocs/pkg/config/config.go
+++ b/services/ocs/pkg/config/config.go
@@ -19,8 +19,7 @@ type Config struct {
        Debug   Debug    `yaml:"debug"`
 
        HTTP HTTP `yaml:"http"`
-       API  API  `yaml:"api"`
-       
+
        GRPCClientTLS *shared.GRPCClientTLS `yaml:"grpc_client_tls"`
        GrpcClient    client.Client         `yaml:"-"`
 
diff --git a/services/ocs/pkg/config/defaults/defaultconfig.go b/services/ocs/pkg/config/defaults/defaultconfig.go
index a665c78b2a..6a9ebec588 100644
--- a/services/ocs/pkg/config/defaults/defaultconfig.go
+++ b/services/ocs/pkg/config/defaults/defaultconfig.go
@@ -44,9 +44,6 @@ func DefaultConfig() *config.Config {
                        Nodes: []string{"127.0.0.1:9233"},
                        TTL:   time.Hour * 12,
                },
-               API: config.API{
-                       ShowUserEmailInResults: true,
-               },
        }
 }
 
diff --git a/services/ocs/pkg/config/reva.go b/services/ocs/pkg/config/reva.go
index 05e8d8c8d7..8413904dab 100644
--- a/services/ocs/pkg/config/reva.go
+++ b/services/ocs/pkg/config/reva.go
@@ -4,7 +4,3 @@ package config
 type TokenManager struct {
        JWTSecret string `yaml:"jwt_secret" env:"OCIS_JWT_SECRET;OCS_JWT_SECRET" desc:"The secret to mint and validate jwt tokens." introductionVersion:"pre5.0"`
 }
-
-type API struct {
-       ShowUserEmailInResults bool `yaml:"show_email_in_results" env:"OCIS_SHOW_USER_EMAIL_IN_RESULTS" desc:"Mask user email addresses in responses. (EXPERIMENTAL)" introductionVersion:"5.1"`
-}

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be the same like my changes 12c785e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants