Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

Require region codes are globally unique #621

Merged
merged 1 commit into from
Sep 22, 2020
Merged

Require region codes are globally unique #621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions cmd/server/assets/realmadmin/_form_abuse_prevention.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

<form method="POST" action="/realm/settings#abuse-prevention" class="floating-form">
{{ .csrfField }}
<input type="hidden" name="abuse_prevention" value="1" />

<p>
Abuse prevention uses the historical record of your realm's past
Expand All @@ -18,9 +19,6 @@
</p>

<div class="form-group form-check">
<!-- This forces the form to send a value of false (instead of no value at
all), when abuse prevention is unchecked. -->
<input type="hidden" name="abuse_prevention_enabled" value="0" />
<input type="checkbox" name="abuse_prevention_enabled" id="abuse-prevention-enabled" class="form-check-input" value="1" {{if $realm.AbusePreventionEnabled}} checked{{end}}>
<label class="form-check-label" for="abuse-prevention-enabled">
Enable abuse prevention
Expand Down
1 change: 1 addition & 0 deletions cmd/server/assets/realmadmin/_form_codes.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

<form method="POST" action="/realm/settings#codes" class="floating-form">
{{ .csrfField }}
<input type="hidden" name="codes" value="1" />

<div class="form-group">
<label>Allowed test types</label>
Expand Down
5 changes: 4 additions & 1 deletion cmd/server/assets/realmadmin/_form_general.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

<form method="POST" action="/realm/settings#general" class="floating-form">
{{ .csrfField }}
<input type="hidden" name="general" value="1" />

<div class="form-label-group">
<input type="text" name="name" id="name" class="form-control{{if $realm.ErrorsFor "name"}} is-invalid{{end}}"
Expand All @@ -22,11 +23,12 @@
<small class="form-text text-muted">
The realm name is displayed on the realm selection page and in the header
when selected. Choose a descriptive name that your team will recognize.
This value must be globally unique in the system.
</small>
</div>

<div class="form-label-group">
<input type="text" name="region_code" id="region-code" class="form-control{{if $realm.ErrorsFor "regionCode"}} is-invalid{{end}}"
<input type="text" name="region_code" id="region-code" class="form-control text-uppercase{{if $realm.ErrorsFor "regionCode"}} is-invalid{{end}}"
value="{{$realm.RegionCode}}" placeholder="Region code" />
<label for="region-code">Region code</label>
{{if $realm.ErrorsFor "regionCode"}}
Expand All @@ -41,6 +43,7 @@
<a href="https://en.wikipedia.org/wiki/List_of_ISO_3166_country_codes">ISO
3166-1 country codes and ISO 3166-2 subdivision codes</a> where
applicable. For example, Washington state would be <code>US-WA</code>.
This value must globally unique in the system.
</small>
</div>

Expand Down
1 change: 1 addition & 0 deletions cmd/server/assets/realmadmin/_form_security.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

<form method="POST" action="/realm/settings#security" class="floating-form">
{{ .csrfField }}
<input type="hidden" name="security" value="1" />

<div class="form-group">
<label for="email-verified-mode">Email verification</label>
Expand Down
2 changes: 1 addition & 1 deletion cmd/server/assets/realmadmin/_form_sms.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

<form method="POST" action="/realm/settings#sms" class="floating-form">
{{ .csrfField }}
<input type="hidden" name="twilio_sms_config" value="1" />
<input type="hidden" name="sms" value="1" />

<div class="form-label-group">
<input type="text" name="twilio_account_sid" id="twilio-account-sid" class="form-control text-monospace"
Expand Down
84 changes: 32 additions & 52 deletions pkg/controller/realmadmin/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,29 +46,33 @@ func init() {

func (c *Controller) HandleSettings() http.Handler {
type FormData struct {
General bool `form:"general"`
Name string `form:"name"`
RegionCode string `form:"region_code"`
WelcomeMessage string `form:"welcome_message"`

Codes bool `form:"codes"`
AllowedTestTypes database.TestType `form:"allowed_test_types"`
RequireDate *bool `form:"require_date"`
RequireDate bool `form:"require_date"`
CodeLength uint `form:"code_length"`
CodeDurationMinutes int64 `form:"code_duration"`
LongCodeLength uint `form:"long_code_length"`
LongCodeDurationHours int64 `form:"long_code_duration"`
SMSTextTemplate string `form:"sms_text_template"`

TwilioSMSConfig bool `form:"twilio_sms_config"`
SMS bool `form:"sms"`
TwilioAccountSid string `form:"twilio_account_sid"`
TwilioAuthToken string `form:"twilio_auth_token"`
TwilioFromNumber string `form:"twilio_from_number"`

MFAMode *int16 `form:"mfa_mode"`
EmailVerifiedMode *int16 `form:"email_verified_mode"`
PasswordRotationPeriodDays uint `form:"password_rotation_period_days"`
PasswordRotationWarningDays uint `form:"password_rotation_warning_days"`
Security bool `form:"security"`
MFAMode int16 `form:"mfa_mode"`
EmailVerifiedMode int16 `form:"email_verified_mode"`
PasswordRotationPeriodDays uint `form:"password_rotation_period_days"`
PasswordRotationWarningDays uint `form:"password_rotation_warning_days"`

AbusePreventionEnabled *bool `form:"abuse_prevention_enabled"`
AbusePrevention bool `form:"abuse_prevention"`
AbusePreventionEnabled bool `form:"abuse_prevention_enabled"`
AbusePreventionLimitFactor float32 `form:"abuse_prevention_limit_factor"`
AbusePreventionBurst uint64 `form:"abuse_prevention_burst"`
}
Expand Down Expand Up @@ -110,59 +114,35 @@ func (c *Controller) HandleSettings() http.Handler {
}

// General
if v := form.Name; v != "" {
realm.Name = v
}
if v := form.RegionCode; v != "" {
realm.RegionCode = v
}
if v := form.WelcomeMessage; v != "" {
realm.WelcomeMessage = v
if form.General {
realm.Name = form.Name
realm.RegionCode = form.RegionCode
realm.WelcomeMessage = form.WelcomeMessage
}

// Codes
if v := form.AllowedTestTypes; v > 0 {
realm.AllowedTestTypes = v
}
if v := form.RequireDate; v != nil {
realm.RequireDate = *v
}
if v := form.CodeLength; v > 0 {
realm.CodeLength = v
}
if v := form.CodeDurationMinutes; v > 0 {
realm.CodeDuration.Duration = time.Duration(v) * time.Minute
}
if v := form.LongCodeLength; v > 0 {
realm.LongCodeLength = v
}
if v := form.LongCodeDurationHours; v > 0 {
realm.LongCodeDuration.Duration = time.Duration(v) * time.Minute
}
if v := form.SMSTextTemplate; v != "" {
realm.SMSTextTemplate = v
if form.Codes {
realm.AllowedTestTypes = form.AllowedTestTypes
realm.RequireDate = form.RequireDate
realm.CodeLength = form.CodeLength
realm.CodeDuration.Duration = time.Duration(form.CodeDurationMinutes) * time.Minute
realm.LongCodeLength = form.LongCodeLength
realm.LongCodeDuration.Duration = time.Duration(form.LongCodeDurationHours) * time.Hour
realm.SMSTextTemplate = form.SMSTextTemplate
}

// Security
if v := form.EmailVerifiedMode; v != nil {
realm.EmailVerifiedMode = database.AuthRequirement(*v)
}
if v := form.MFAMode; v != nil {
realm.MFAMode = database.AuthRequirement(*v)
}
if v := form.PasswordRotationPeriodDays; v > 0 {
realm.PasswordRotationPeriodDays = v
}
if v := form.PasswordRotationWarningDays; v > 0 {
realm.PasswordRotationWarningDays = v
if form.Security {
realm.EmailVerifiedMode = database.AuthRequirement(form.EmailVerifiedMode)
realm.MFAMode = database.AuthRequirement(form.MFAMode)
realm.PasswordRotationPeriodDays = form.PasswordRotationPeriodDays
realm.PasswordRotationWarningDays = form.PasswordRotationWarningDays
}

// Abuse prevention
if v := form.AbusePreventionEnabled; v != nil {
realm.AbusePreventionEnabled = *v
}
if v := form.AbusePreventionLimitFactor; v > 0 {
realm.AbusePreventionLimitFactor = v
if form.AbusePrevention {
realm.AbusePreventionEnabled = form.AbusePreventionEnabled
realm.AbusePreventionLimitFactor = form.AbusePreventionLimitFactor
}

// Save
Expand All @@ -172,7 +152,7 @@ func (c *Controller) HandleSettings() http.Handler {
return
}

if form.TwilioSMSConfig {
if form.SMS {
// Process SMS settings
smsConfig, err := realm.SMSConfig(c.db)
if err != nil && !database.IsNotFound(err) {
Expand Down
18 changes: 17 additions & 1 deletion pkg/database/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ func callbackKMSEncrypt(ctx context.Context, keyManager keys.KeyManager, keyID,
}
}

// callback HMAC alters HMACs the value with the given key before saving.
// callbackHMAC alters HMACs the value with the given key before saving.
func callbackHMAC(ctx context.Context, hashFunc func(string) (string, error), table, column string) func(scope *gorm.Scope) {
return func(scope *gorm.Scope) {
// Do nothing if not the target table
Expand Down Expand Up @@ -486,3 +486,19 @@ func withRetries(ctx context.Context, f retry.RetryFunc) error {

return retry.Do(ctx, b, f)
}

// stringValue gets the value of the string pointer, returning "" for nil.
func stringValue(s *string) string {
if s == nil {
return ""
}
return *s
}

// stringPtr converts the string value to a pointer, returning nil for "".
func stringPtr(s string) *string {
if s == "" {
return nil
}
return &s
}
91 changes: 91 additions & 0 deletions pkg/database/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,6 +1131,97 @@ func (db *Database) getMigrations(ctx context.Context) *gormigrate.Gormigrate {
return nil
},
},
{
ID: "00049-MakeRegionCodeUnique",
Migrate: func(tx *gorm.DB) error {
sqls := []string{
// Make region code case insensitive and unique.
"ALTER TABLE realms ALTER COLUMN region_code TYPE CITEXT",
"ALTER TABLE realms ALTER COLUMN region_code DROP DEFAULT",
"ALTER TABLE realms ALTER COLUMN region_code DROP NOT NULL",
}
for _, sql := range sqls {
if err := tx.Exec(sql).Error; err != nil {
return err
}
}

// Make any existing empty string region codes to NULL. Without this,
// the new unique constraint will fail.
if err := tx.Exec("UPDATE realms SET region_code = NULL WHERE TRIM(region_code) = ''").Error; err != nil {
return err
}

// Make all region codes uppercase.
if err := tx.Exec("UPDATE realms SET region_code = UPPER(region_code) WHERE region_code IS NOT NULL").Error; err != nil {
return err
}

// Find any existing duplicate region codes - this could be combined
// into a much larger SQL statement with the next thing, but I'm
// optimizing for readability here.
var dupRegionCodes []string
if err := tx.Model(&Realm{}).
Unscoped().
Select("UPPER(region_code) AS region_code").
Where("region_code IS NOT NULL").
Group("region_code").
Having("COUNT(*) > 1").
Pluck("region_code", &dupRegionCodes).
Error; err != nil {
return err
}

// Update any duplicate regions to not be duplicate anymore.
for _, dupRegionCode := range dupRegionCodes {
sethvargo marked this conversation as resolved.
Show resolved Hide resolved
logger.Warn("de-duplicating region code %q", dupRegionCode)

// I call this the "Microsoft method". For each duplicate realm,
// append -N, starting with 1. If there are 3 realms with the region
// code "PA", their new values will be "PA", "PA-1", and "PA-2"
// respectively.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is a 'good' way do this this automatically.

I also don't have another suggestion.

sql := `
UPDATE
realms
SET region_code = CONCAT(realms.region_code, '-', (z-1)::text)
FROM (
SELECT
id,
region_code,
ROW_NUMBER() OVER (ORDER BY id ASC) AS z
FROM realms
WHERE UPPER(region_code) = UPPER($1)
) AS sq
WHERE realms.id = sq.id AND sq.z > 1`
if err := tx.Exec(sql, dupRegionCode).Error; err != nil {
return err
}
}

sqls = []string{
// There's already a runtime constraint and validation on names, this
// is just an extra layer of protection at the database layer.
"ALTER TABLE realms ALTER COLUMN name SET NOT NULL",

// Alter the unique index on realm names to be a column constraint.
"DROP INDEX IF EXISTS uix_realms_name",
"ALTER TABLE realms ADD CONSTRAINT uix_realms_name UNIQUE (name)",

// Now finally add a unique constraint on region codes.
"ALTER TABLE realms ADD CONSTRAINT uix_realms_region_code UNIQUE (region_code)",
}

for _, sql := range sqls {
if err := tx.Exec(sql).Error; err != nil {
return err
}
}
return nil
},
Rollback: func(tx *gorm.DB) error {
return nil
},
},
})
}

Expand Down
31 changes: 24 additions & 7 deletions pkg/database/realm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,20 +80,26 @@ type Realm struct {
// Name is the name of the realm.
Name string `gorm:"type:varchar(200);unique_index"`

// RegionCode is both a display attribute and required field for ENX. To
// handle NULL and uniqueness, the field is converted from it's ptr type to a
// concrete type in callbacks. Do not modify RegionCodePtr directly.
RegionCode string `gorm:"-"`
RegionCodePtr *string `gorm:"column:region_code; type:varchar(10);"`
Copy link
Contributor

Choose a reason for hiding this comment

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

does gorm work w/ sql.NullString?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, but our code doesn't (really). Changing the type of RegionCode requires every caller to update their type. So all cases of:

realm.RegionCode = "foo"
printf(realm.RegionCode)

become

if v != "" {
  realm.RegionCode = sql.NullString{Value:v, Valid: true}
} else {
  ream.RegionCode = sql.NullString{}
}

if realm.RegionCode.Valid() {
  printf(realm.RegionCode.Value)
} else {
  printf("")
}

and I don't want to put that burden on callers tbh.


// WelcomeMessage is arbitrary realm-defined data to display to users after
// selecting this realm. If empty, nothing is displayed. The format is
// markdown. Do not modify WelcomeMessagePtr directly.
WelcomeMessage string `gorm:"-"`
WelcomeMessagePtr *string `gorm:"column:welcome_message; type:text;"`

// Code configuration
RegionCode string `gorm:"type:varchar(10); not null; default: ''"`
CodeLength uint `gorm:"type:smallint; not null; default: 8"`
CodeDuration DurationSeconds `gorm:"type:bigint; not null; default: 900"` // default 15m (in seconds)
LongCodeLength uint `gorm:"type:smallint; not null; default: 16"`
LongCodeDuration DurationSeconds `gorm:"type:bigint; not null; default: 86400"` // default 24h
// SMS Content
SMSTextTemplate string `gorm:"type:varchar(400); not null; default: 'This is your Exposure Notifications Verification code: [longcode] Expires in [longexpires] hours'"`

// WelcomeMessage is arbitrary realm-defined data to display to users after
// selecting this realm. If empty, nothing is displayed. The format is
// markdown.
WelcomeMessage string `gorm:"type:text"`

// MFAMode represents the mode for Multi-Factor-Authorization requirements for the realm.
MFAMode AuthRequirement `gorm:"type:smallint; not null; default: 0"`

Expand Down Expand Up @@ -187,6 +193,14 @@ func (r *Realm) SigningKeyID() string {
return fmt.Sprintf("realm-%d", r.ID)
}

// AfterFind runs after a realm is found.
func (r *Realm) AfterFind(tx *gorm.DB) error {
r.RegionCode = stringValue(r.RegionCodePtr)
r.WelcomeMessage = stringValue(r.WelcomeMessagePtr)

return nil
}

// BeforeSave runs validations. If there are errors, the save fails.
func (r *Realm) BeforeSave(tx *gorm.DB) error {
r.Name = strings.TrimSpace(r.Name)
Expand All @@ -195,10 +209,13 @@ func (r *Realm) BeforeSave(tx *gorm.DB) error {
}

r.RegionCode = strings.ToUpper(strings.TrimSpace(r.RegionCode))

if len(r.RegionCode) > 10 {
r.AddError("regionCode", "cannot be more than 10 characters")
}
r.RegionCodePtr = stringPtr(r.RegionCode)

r.WelcomeMessage = strings.TrimSpace(r.WelcomeMessage)
r.WelcomeMessagePtr = stringPtr(r.WelcomeMessage)

if r.EnableENExpress {
if r.RegionCode == "" {
Expand Down