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

Federation: Provide a New Endpoint for Obtaining Classified Domains #1626

Merged
merged 76 commits into from
Jul 14, 2021

Conversation

mdimjasevic
Copy link
Contributor

@mdimjasevic mdimjasevic commented Jun 24, 2021

The PR implements a new endpoint GET /teams/:teamId/features/classifiedDomains that fetches a list of classified domains. There is an internal endpoint equivalent.

Checklist

  • Title of this PR explains the impact of the change.
  • The description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • The CHANGELOG.md file in the Unreleased section has been updated to explain the change which will be included in the release notes.
  • If a component uses a new or changed internal endpoint of another component, this is mentioned in the CHANGELOG.md.
  • If this PR creates a new endpoint, or adds a new configuration flag, the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed.
  • Add a test that checks if a user is authenticated when getting classified domains

@mdimjasevic mdimjasevic marked this pull request as draft June 24, 2021 12:40
docs/reference/config-options.md Outdated Show resolved Hide resolved
hack/helm_vars/wire-server/values.yaml Outdated Show resolved Hide resolved
libs/galley-types/src/Galley/Types/Teams.hs Outdated Show resolved Hide resolved
services/galley/src/Galley/API/Public.hs Outdated Show resolved Hide resolved
services/galley/test/integration/API/Teams/Feature.hs Outdated Show resolved Hide resolved
@mdimjasevic
Copy link
Contributor Author

@smatting , should I also add an endpoint in Stern for classified domains, just like the app lock feature has one at

mkFeaturePutGetRoute @'Public.TeamFeatureAppLock
?

Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

I ran helm install --debug --dry-run wire-server .local/charts/wire-server -f hack/helm_vars/wire-server/values.yaml > output.yaml using a valid kubernetes server config, which results in a final galley config of

#snippet from configmap of galley
galley:
  config:
    cassandra:
      host: cassandra-ephemeral
      replicaCount: 1
    enableFederator: true
    journal:
      endpoint: http://fake-aws-sqs:4568
      queue: integration-team-events.fifo
    settings:
      conversationCodeURI: https://kube-staging-nginz-https.zinfra.io/conversation-join/
      enableIndexedBillingTeamMembers: true
      featureFlags:
        classifiedDomains:
          config:
            domains:
            - example.com
          status: enabled
        legalhold: whitelist-teams-and-implicit-consent
        sso: disabled-by-default
        teamSearchVisibility: disabled-by-default
      federationDomain: integration.example.com

which appears to be in line with what's in galley.integration.yaml.

Do integration tests pass locally?

services/galley/test/integration/API/Teams/Feature.hs Outdated Show resolved Hide resolved
test s "ValidateSAMLEmails" $ testSimpleFlag @'Public.TeamFeatureValidateSAMLEmails
test s "ValidateSAMLEmails" $ testSimpleFlag @'Public.TeamFeatureValidateSAMLEmails,
test s "Classified Domains (enabled)" testClassifiedDomainsEnabled,
test s "Classified Domains (disabled)" testClassifiedDomainsDisabled
Copy link
Member

Choose a reason for hiding this comment

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

Can you test that when querying the GET /teams/{tid}/features endpoint, the classified domains stuff is included as well?

Copy link
Member

Choose a reason for hiding this comment

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

I performed a manual test:

curl -s http://localhost:8085/teams/b8509dc2-4b4e-4d2f-afd6-410666bbb464/features -H Z-User:c8b2bb6b-4980-4950-af5d-211d6cb951e9 | json_pp
{
   "appLock" : {
      "config" : {
         "enforceAppLock" : false,
         "inactivityTimeoutSecs" : 60
      },
      "status" : "enabled"
   },
   "classifiedDomains" : {
      "config" : {
         "domains" : [
            "example.com"
         ]
      },
      "status" : "enabled"
   },
   "digitalSignatures" : {
      "status" : "disabled"
   },
   "legalhold" : {
      "status" : "disabled"
   },
   "searchVisibility" : {
      "status" : "disabled"
   },
   "sso" : {
      "status" : "disabled"
   },
   "validateSAMLemails" : {
      "status" : "disabled"
   }
}

(and got some user/team ids using select team, binding, creator from galley_test.team limit 10; after make cqlsh and picked IDs from a binding team)

Still, an integration test here would be nice.

charts/galley/values.yaml Outdated Show resolved Hide resolved
test s "ValidateSAMLEmails" $ testSimpleFlag @'Public.TeamFeatureValidateSAMLEmails
test s "ValidateSAMLEmails" $ testSimpleFlag @'Public.TeamFeatureValidateSAMLEmails,
test s "Classified Domains (enabled)" testClassifiedDomainsEnabled,
test s "Classified Domains (disabled)" testClassifiedDomainsDisabled
Copy link
Member

Choose a reason for hiding this comment

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

I performed a manual test:

curl -s http://localhost:8085/teams/b8509dc2-4b4e-4d2f-afd6-410666bbb464/features -H Z-User:c8b2bb6b-4980-4950-af5d-211d6cb951e9 | json_pp
{
   "appLock" : {
      "config" : {
         "enforceAppLock" : false,
         "inactivityTimeoutSecs" : 60
      },
      "status" : "enabled"
   },
   "classifiedDomains" : {
      "config" : {
         "domains" : [
            "example.com"
         ]
      },
      "status" : "enabled"
   },
   "digitalSignatures" : {
      "status" : "disabled"
   },
   "legalhold" : {
      "status" : "disabled"
   },
   "searchVisibility" : {
      "status" : "disabled"
   },
   "sso" : {
      "status" : "disabled"
   },
   "validateSAMLemails" : {
      "status" : "disabled"
   }
}

(and got some user/team ids using select team, binding, creator from galley_test.team limit 10; after make cqlsh and picked IDs from a binding team)

Still, an integration test here would be nice.

@mdimjasevic mdimjasevic requested a review from jschaul July 12, 2021 11:49
@mdimjasevic
Copy link
Contributor Author

@jschaul , I just added an integration test for the all features endpoint.

Copy link
Contributor

@smatting smatting left a comment

Choose a reason for hiding this comment

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

I think this good to merge since the 1 failing test is known to be unrelated.

@smatting
Copy link
Contributor

@smatting , should I also add an endpoint in Stern for classified domains, just like the app lock feature has one at

no, applock is supposed to be configured only by team admins with the public endpoint

@mdimjasevic mdimjasevic merged commit 0687a0b into develop Jul 14, 2021
@mdimjasevic mdimjasevic deleted the mdimjasevic/fed-classified-domains branch July 14, 2021 08:58
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.

4 participants