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

Sqservices 1028 be add sso SCIM attributes to user profile for team admins unvalidated email #2220

Conversation

battermann
Copy link
Contributor

@battermann battermann commented Mar 21, 2022

as a team admin
when I search for team contacts in team management
I want to see un-validated emails in the user profiles that are returned by the search

https://wearezeta.atlassian.net/browse/SQSERVICES-1028

Checklist

  • The PR Title explains the impact of the change.
  • The PR 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.
  • If this PR changes development workflow or dependencies, they have been A) automated and B) documented under docs/developer/. All efforts have been taken to minimize development setup breakage or slowdown for co-workers.
  • If a cassandra schema migration has been added, I ran make git-add-cassandra-schema to update the cassandra schema documentation.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

also good... mostly. :)

feel free to second-guess me if you can make @smatting agree with you!

@@ -200,7 +200,8 @@ data TeamContact = TeamContact
teamContactSAMLIdp :: Maybe Text,
teamContactRole :: Maybe Role,
teamContactScimExternalId :: Maybe Text,
teamContactSso :: Maybe ContactSso
teamContactSso :: Maybe ContactSso,
teamContactEmailUnvalidated :: Maybe Email
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this type only visible to admins? If not, hiding the unvalidated email should be enforced:

data TeamContact f = TeamContact
  { ...
    teamContactEmailUnvalidated :: f Email
  }

...
userSearch :: ... -> m (TeamContact (Const ()))
...
adminSearch :: ... -> m (TeamContact Id)
...

Copy link
Contributor Author

@battermann battermann Apr 7, 2022

Choose a reason for hiding this comment

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

AFAICT this type is only visible to team admins. It already contains potentially confidential data.
see: https://github.com/wireapp/wire-server/blob/develop/services/brig/src/Brig/User/API/Search.hs#L234

Migration 70 "Add email_unvalidated to user table" $
schema'
[r| ALTER TABLE user ADD (
email_unvalidated text
Copy link
Contributor

Choose a reason for hiding this comment

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

given that changing this column and changing other columns usually don't coincided (or do they?), wouldn't it be more straight-forward and more readable to have new table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is one big advantage when having it in a single table, and that is we only have to query C* once when creating/updating the ES index.
if you insist, I'm also ok with having a new table.

services/brig/src/Brig/API/User.hs Outdated Show resolved Hide resolved
libs/brig-types/src/Brig/Types/User/Event.hs Outdated Show resolved Hide resolved
@battermann battermann force-pushed the SQSERVICES-1028-be-add-sso-scim-attributes-to-user-profile-for-team-admins branch 2 times, most recently from b9b07d9 to c1bf280 Compare April 1, 2022 14:04
@battermann battermann force-pushed the SQSERVICES-1028-be-add-sso-scim-attributes-to-user-profile-for-team-admins-unvalidated-email branch from f95d288 to c99198d Compare April 5, 2022 12:37
Base automatically changed from SQSERVICES-1028-be-add-sso-scim-attributes-to-user-profile-for-team-admins to develop April 5, 2022 13:33
@battermann battermann force-pushed the SQSERVICES-1028-be-add-sso-scim-attributes-to-user-profile-for-team-admins-unvalidated-email branch from c99198d to 84748ca Compare April 5, 2022 13:37
@battermann battermann temporarily deployed to cachix April 5, 2022 13:37 Inactive
added email_unvalidated field to user table

update and delete user email unvalidated
@battermann battermann force-pushed the SQSERVICES-1028-be-add-sso-scim-attributes-to-user-profile-for-team-admins-unvalidated-email branch from 84748ca to 2e23596 Compare April 7, 2022 09:53
@battermann battermann temporarily deployed to cachix April 7, 2022 09:54 Inactive
@battermann battermann temporarily deployed to cachix April 7, 2022 11:16 Inactive
@battermann battermann temporarily deployed to cachix April 7, 2022 14:38 Inactive
@battermann battermann temporarily deployed to cachix April 7, 2022 14:42 Inactive
@battermann battermann temporarily deployed to cachix April 7, 2022 14:49 Inactive
@battermann battermann marked this pull request as ready for review April 7, 2022 14:49
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.

Can you please bumpt expectedMigrationVersion to trigger a reindexing?
Also please update oldMapping in the brig integration tests.

@battermann battermann temporarily deployed to cachix April 11, 2022 09:23 Inactive
@battermann battermann merged commit cefdda9 into develop Apr 11, 2022
@battermann battermann deleted the SQSERVICES-1028-be-add-sso-scim-attributes-to-user-profile-for-team-admins-unvalidated-email branch April 11, 2022 13:44
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

3 participants