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

Make account registration whitelists local #3043

Merged
merged 14 commits into from
Feb 23, 2023

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Feb 1, 2023

https://wearezeta.atlassian.net/browse/SQPIT-405 (a related wire infrastructure PR is linked in the ticket)

This is changing a feature wire has been using on our staging environment, and (probably?) not anywhere else. See the changelog if you think you may be affected.

Since the service is both outdated and almost unused, this PR moves the data from that service into the local server config yaml.

Migration should be painless, since the new settings are in a different place than the old ones. Just make sure the new fields are added to the config before the upgrade, and then you can remove the old ones at any time after.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Feb 1, 2023
-- email/phone address is whitelisted.
verify :: Maybe WhitelistEmailDomains -> Maybe WhitelistPhonePrefixes -> Either Email Phone -> Bool
verify (Just (WhitelistEmailDomains allowed)) _ (Left email) = emailDomain email `elem` allowed
verify _ (Just (WhitelistPhonePrefixes allowed)) (Right phone) = any (`Text.isPrefixOf` fromPhone phone) allowed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this allows for empty prefixes, which may or may not be what the admin wants, but i'm leaning against adding a special case that ignores or errors out on - "".

@fisx fisx requested a review from jschaul February 8, 2023 15:23
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.

While you're at it, could you rename Whitelist to Allowlist in the types everywhere consistently? I see the config is called Allowlist; but the code still uses Whitelist*.

@fisx
Copy link
Contributor Author

fisx commented Feb 8, 2023

While you're at it, could you rename Whitelist to Allowlist in the types everywhere consistently? I see the config is called Allowlist; but the code still uses Whitelist*.

oops, thanks! b1d530c

@fisx fisx requested a review from jschaul February 8, 2023 16:08
import Brig.API.Error
import qualified Brig.AWS as AWS
import Brig.App
import Brig.CanonicalInterpreter (BrigCanonicalEffects, runBrigToIO)
import Brig.Email (Email)
import Brig.Options (setWhitelist)
import Brig.Options (setAllowlistEmailDomains, setAllowlistPhonePrefixes)
import Brig.Phone (Phone, PhoneException (..))
import qualified Brig.Whitelist as Whitelist
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth going all in and changing terms everywhere? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh man! :)

cd42e8d

also 8c1626b

what else did i miss?

@fisx fisx requested a review from elland February 10, 2023 06:45
@fisx
Copy link
Contributor Author

fisx commented Feb 22, 2023

@jschaul or @elland can i get another chance? :)

if you have no objections and feel confident, please merge right away. our infra is ready (see ticket).

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.

🥳

@jschaul jschaul merged commit 7cd0a9c into develop Feb 23, 2023
@jschaul jschaul deleted the SQPIT-405-make-whitelist-local branch February 23, 2023 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants