review fixes, bug fixes

irc/accounts.go

@@ -109,7 +109,7 @@ func (am *AccountManager) buildNickToAccountIndex() {
 }
 
 func (am *AccountManager) initVHostRequestQueue() {
-	if !am.server.AccountConfig().HostServ.Enabled {
+	if !am.server.AccountConfig().VHosts.Enabled {
 		return
 	}
 
@@ -642,10 +642,7 @@ func (am *AccountManager) Unregister(account string) error {
 		tx.Delete(credentialsKey)
 		tx.Delete(vhostKey)
 		_, err := tx.Delete(vhostQueueKey)
-		if err != nil {
+		am.decrementVHostQueueCount(casefoldedAccount, err)
-			// 2's complement decrement
-			atomic.AddUint64(&am.vhostRequestPendingCount, ^uint64(0))
-		}
 		return nil
 	})
 
@@ -737,7 +734,7 @@ type PendingVHostRequest struct {
 // callback type implementing the actual business logic of vhost operations
 type vhostMunger func(input VHostInfo) (output VHostInfo, err error)
 
-func (am *AccountManager) VHostSet(account string, vhost string) (err error) {
+func (am *AccountManager) VHostSet(account string, vhost string) (result VHostInfo, err error) {
 	munger := func(input VHostInfo) (output VHostInfo, err error) {
 		output = input
 		output.Enabled = true
@@ -748,7 +745,7 @@ func (am *AccountManager) VHostSet(account string, vhost string) (err error) {
 	return am.performVHostChange(account, munger)
 }
 
-func (am *AccountManager) VHostRequest(account string, vhost string) (err error) {
+func (am *AccountManager) VHostRequest(account string, vhost string) (result VHostInfo, err error) {
 	munger := func(input VHostInfo) (output VHostInfo, err error) {
 		output = input
 		output.RequestedVHost = vhost
@@ -761,7 +758,7 @@ func (am *AccountManager) VHostRequest(account string, vhost string) (err error)
 	return am.performVHostChange(account, munger)
 }
 
-func (am *AccountManager) VHostApprove(account string) (err error) {
+func (am *AccountManager) VHostApprove(account string) (result VHostInfo, err error) {
 	munger := func(input VHostInfo) (output VHostInfo, err error) {
 		output = input
 		output.Enabled = true
@@ -774,7 +771,7 @@ func (am *AccountManager) VHostApprove(account string) (err error) {
 	return am.performVHostChange(account, munger)
 }
 
-func (am *AccountManager) VHostReject(account string, reason string) (err error) {
+func (am *AccountManager) VHostReject(account string, reason string) (result VHostInfo, err error) {
 	munger := func(input VHostInfo) (output VHostInfo, err error) {
 		output = input
 		output.RejectedVHost = output.RequestedVHost
@@ -786,7 +783,7 @@ func (am *AccountManager) VHostReject(account string, reason string) (err error)
 	return am.performVHostChange(account, munger)
 }
 
-func (am *AccountManager) VHostSetEnabled(client *Client, enabled bool) (err error) {
+func (am *AccountManager) VHostSetEnabled(client *Client, enabled bool) (result VHostInfo, err error) {
 	munger := func(input VHostInfo) (output VHostInfo, err error) {
 		output = input
 		output.Enabled = enabled
@@ -796,10 +793,11 @@ func (am *AccountManager) VHostSetEnabled(client *Client, enabled bool) (err err
 	return am.performVHostChange(client.Account(), munger)
 }
 
-func (am *AccountManager) performVHostChange(account string, munger vhostMunger) (err error) {
+func (am *AccountManager) performVHostChange(account string, munger vhostMunger) (result VHostInfo, err error) {
 	account, err = CasefoldName(account)
 	if err != nil || account == "" {
-		return errAccountDoesNotExist
+		err = errAccountDoesNotExist
+		return
 	}
 
 	am.vHostUpdateMutex.Lock()
@@ -807,19 +805,22 @@ func (am *AccountManager) performVHostChange(account string, munger vhostMunger)
 
 	clientAccount, err := am.LoadAccount(account)
 	if err != nil {
-		return errAccountDoesNotExist
+		err = errAccountDoesNotExist
+		return
 	} else if !clientAccount.Verified {
-		return errAccountUnverified
+		err = errAccountUnverified
+		return
 	}
 
-	result, err := munger(clientAccount.VHost)
+	result, err = munger(clientAccount.VHost)
 	if err != nil {
-		return err
+		return
 	}
 
 	vhtext, err := json.Marshal(result)
 	if err != nil {
-		return errAccountUpdateFailed
+		err = errAccountUpdateFailed
+		return
 	}
 	vhstr := string(vhtext)
 
@@ -839,21 +840,30 @@ func (am *AccountManager) performVHostChange(account string, munger vhostMunger)
 			atomic.AddUint64(&am.vhostRequestPendingCount, 1)
 		} else if clientAccount.VHost.RequestedVHost != "" && result.RequestedVHost == "" {
 			_, err = tx.Delete(queueKey)
-			if err != nil {
+			am.decrementVHostQueueCount(account, err)
-				// XXX this is the decrement operation for two's complement
-				atomic.AddUint64(&am.vhostRequestPendingCount, ^uint64(0))
-			}
 		}
 
 		return nil
 	})
 
 	if err != nil {
-		return errAccountUpdateFailed
+		err = errAccountUpdateFailed
+		return
 	}
 
 	am.applyVhostToClients(account, result)
-	return nil
+	return result, nil
+}
+
+// XXX annoying helper method for keeping the queue count in sync with the DB
+// `err` is the buntdb error returned from deleting the queue key
+func (am *AccountManager) decrementVHostQueueCount(account string, err error) {
+	if err == nil {
+		// successfully deleted a queue entry, do a 2's complement decrement:
+		atomic.AddUint64(&am.vhostRequestPendingCount, ^uint64(0))
+	} else if err != buntdb.ErrNotFound {
+		am.server.logger.Error("internal", "buntdb dequeue error", account, err.Error())
+	}
 }
 
 func (am *AccountManager) VHostListRequests(limit int) (requests []PendingVHostRequest, total int) {
@@ -893,7 +903,7 @@ func (am *AccountManager) VHostListRequests(limit int) (requests []PendingVHostR
 func (am *AccountManager) applyVHostInfo(client *Client, info VHostInfo) {
 	// if hostserv is disabled in config, then don't grant vhosts
 	// that were previously approved while it was enabled
-	if !am.server.AccountConfig().HostServ.Enabled {
+	if !am.server.AccountConfig().VHosts.Enabled {
 		return
 	}
 

irc/client.go

@@ -311,10 +311,6 @@ func (client *Client) Ping() {
 
 }
 
-//
-// server goroutine
-//
-
 // Register sets the client details as appropriate when entering the network.
 func (client *Client) Register() {
 	client.stateMutex.Lock()
@@ -584,8 +580,8 @@ func (client *Client) SetVHost(vhost string) (updated bool) {
 func (client *Client) updateNick(nick string) {
 	casefoldedName, err := CasefoldName(nick)
 	if err != nil {
-		log.Println(fmt.Sprintf("ERROR: Nick [%s] couldn't be casefolded... this should never happen. Printing stacktrace.", client.nick))
+		client.server.logger.Error("internal", "nick couldn't be casefolded", nick, err.Error())
-		debug.PrintStack()
+		return
 	}
 	client.stateMutex.Lock()
 	client.nick = nick
@@ -616,8 +612,8 @@ func (client *Client) updateNickMaskNoMutex() {
 	nickMaskString := fmt.Sprintf("%s!%s@%s", client.nick, client.username, client.hostname)
 	nickMaskCasefolded, err := Casefold(nickMaskString)
 	if err != nil {
-		log.Println(fmt.Sprintf("ERROR: Nickmask [%s] couldn't be casefolded... this should never happen. Printing stacktrace.", client.nickMaskString))
+		client.server.logger.Error("internal", "nickmask couldn't be casefolded", nickMaskString, err.Error())
-		debug.PrintStack()
+		return
 	}
 
 	client.nickMaskString = nickMaskString

irc/config.go

@@ -64,7 +64,7 @@ type AccountConfig struct {
 	AuthenticationEnabled bool                  `yaml:"authentication-enabled"`
 	SkipServerPassword    bool                  `yaml:"skip-server-password"`
 	NickReservation       NickReservationConfig `yaml:"nick-reservation"`
-	HostServ              HostServConfig
+	VHosts                VHostConfig
 }
 
 // AccountRegistrationConfig controls account registration.
@@ -92,13 +92,16 @@ type AccountRegistrationConfig struct {
 	AllowMultiplePerConnection bool `yaml:"allow-multiple-per-connection"`
 }
 
-type HostServConfig struct {
+type VHostConfig struct {
-	Enabled             bool
+	Enabled        bool
-	UserRequestsEnabled bool `yaml:"user-requests-enabled"`
+	MaxLength      int    `yaml:"max-length"`
-	Cooldown            time.Duration
+	ValidRegexpRaw string `yaml:"valid-regexp"`
-	MaxVHostLen         int    `yaml:"max-vhost-len"`
+	ValidRegexp    *regexp.Regexp
-	ValidRegexpRaw      string `yaml:"valid-regexp"`
+	UserRequests   struct {
-	ValidRegexp         *regexp.Regexp
+		Enabled  bool
+		Channel  string
+		Cooldown time.Duration
+	} `yaml:"user-requests"`
 }
 
 type NickReservationMethod int
@@ -543,17 +546,17 @@ func LoadConfig(filename string) (config *Config, err error) {
 		}
 	}
 
-	rawRegexp := config.Accounts.HostServ.ValidRegexpRaw
+	rawRegexp := config.Accounts.VHosts.ValidRegexpRaw
 	if rawRegexp != "" {
 		regexp, err := regexp.Compile(rawRegexp)
 		if err == nil {
-			config.Accounts.HostServ.ValidRegexp = regexp
+			config.Accounts.VHosts.ValidRegexp = regexp
 		} else {
 			log.Printf("invalid vhost regexp: %s\n", err.Error())
 		}
 	}
-	if config.Accounts.HostServ.ValidRegexp == nil {
+	if config.Accounts.VHosts.ValidRegexp == nil {
-		config.Accounts.HostServ.ValidRegexp = validVhostRegex
+		config.Accounts.VHosts.ValidRegexp = defaultValidVhostRegex
 	}
 
 	maxSendQBytes, err := bytefmt.ToBytes(config.Server.MaxSendQString)

irc/gateways.go

@@ -75,9 +75,12 @@ func (client *Client) ApplyProxiedIP(proxiedIP string, tls bool) (exiting bool)
 	}
 
 	// given IP is sane! override the client's current IP
+	rawHostname := utils.LookupHostname(proxiedIP)
+	client.stateMutex.Lock()
 	client.proxiedIP = parsedProxiedIP
-	client.rawHostname = utils.LookupHostname(proxiedIP)
+	client.rawHostname = rawHostname
-	client.hostname = client.rawHostname
+	client.stateMutex.Unlock()
+	// nickmask will be updated when the client completes registration
 
 	// set tls info
 	client.certfp = ""

irc/hostserv.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2017 Daniel Oaks <daniel@danieloaks.net>
+// Copyright (c) 2018 Shivaram Lingamneni <slingamn@cs.stanford.edu>
 // released under the MIT license
 
 package irc
@@ -26,16 +26,16 @@ var (
 	errVHostBadCharacters = errors.New("Vhost contains prohibited characters")
 	errVHostTooLong       = errors.New("Vhost is too long")
 	// ascii only for now
-	validVhostRegex = regexp.MustCompile(`^[0-9A-Za-z.\-_/]+$`)
+	defaultValidVhostRegex = regexp.MustCompile(`^[0-9A-Za-z.\-_/]+$`)
 )
 
 func hostservEnabled(server *Server) bool {
-	return server.AccountConfig().HostServ.Enabled
+	return server.AccountConfig().VHosts.Enabled
 }
 
 func hostservRequestsEnabled(server *Server) bool {
 	ac := server.AccountConfig()
-	return ac.HostServ.Enabled && ac.HostServ.UserRequestsEnabled
+	return ac.VHosts.Enabled && ac.VHosts.UserRequests.Enabled
 }
 
 var (
@@ -84,7 +84,7 @@ request for a new one.`,
 
 SET sets a user's vhost, bypassing the request system.`,
 			helpShort: `$bSET$b sets a user's vhost.`,
-			capabs:    []string{"hostserv"},
+			capabs:    []string{"vhosts"},
 			enabled:   hostservEnabled,
 		},
 		"del": {
@@ -93,7 +93,7 @@ SET sets a user's vhost, bypassing the request system.`,
 
 DEL sets a user's vhost, bypassing the request system.`,
 			helpShort: `$bDEL$b deletes a user's vhost.`,
-			capabs:    []string{"hostserv"},
+			capabs:    []string{"vhosts"},
 			enabled:   hostservEnabled,
 		},
 		"waiting": {
@@ -103,7 +103,7 @@ DEL sets a user's vhost, bypassing the request system.`,
 WAITING shows a list of pending vhost requests, which can then be approved
 or rejected.`,
 			helpShort: `$bWAITING$b shows a list of pending vhost requests.`,
-			capabs:    []string{"hostserv"},
+			capabs:    []string{"vhosts"},
 			enabled:   hostservEnabled,
 		},
 		"approve": {
@@ -112,7 +112,7 @@ or rejected.`,
 
 APPROVE approves a user's vhost request.`,
 			helpShort: `$bAPPROVE$b approves a user's vhost request.`,
-			capabs:    []string{"hostserv"},
+			capabs:    []string{"vhosts"},
 			enabled:   hostservEnabled,
 		},
 		"reject": {
@@ -122,7 +122,7 @@ APPROVE approves a user's vhost request.`,
 REJECT rejects a user's vhost request, optionally giving them a reason
 for the rejection.`,
 			helpShort: `$bREJECT$b rejects a user's vhost request.`,
-			capabs:    []string{"hostserv"},
+			capabs:    []string{"vhosts"},
 			enabled:   hostservEnabled,
 		},
 	}
@@ -133,13 +133,26 @@ func hsNotice(rb *ResponseBuffer, text string) {
 	rb.Add(nil, "HostServ", "NOTICE", rb.target.Nick(), text)
 }
 
+// hsNotifyChannel notifies the designated channel of new vhost activity
+func hsNotifyChannel(server *Server, message string) {
+	chname := server.AccountConfig().VHosts.UserRequests.Channel
+	channel := server.channels.Get(chname)
+	if channel == nil {
+		return
+	}
+	chname = channel.Name()
+	for _, client := range channel.Members() {
+		client.Send(nil, "HostServ", "PRIVMSG", chname, message)
+	}
+}
+
 func hsOnOffHandler(server *Server, client *Client, command, params string, rb *ResponseBuffer) {
 	enable := false
 	if command == "on" {
 		enable = true
 	}
 
-	err := server.accounts.VHostSetEnabled(client, enable)
+	_, err := server.accounts.VHostSetEnabled(client, enable)
 	if err != nil {
 		hsNotice(rb, client.t("An error occurred"))
 	} else if enable {
@@ -163,7 +176,7 @@ func hsRequestHandler(server *Server, client *Client, command, params string, rb
 		return
 	}
 	elapsed := time.Now().Sub(account.VHost.LastRequestTime)
-	remainingTime := server.AccountConfig().HostServ.Cooldown - elapsed
+	remainingTime := server.AccountConfig().VHosts.UserRequests.Cooldown - elapsed
 	// you can update your existing request, but if you were rejected,
 	// you can't spam a replacement request
 	if account.VHost.RequestedVHost == "" && remainingTime > 0 {
@@ -171,11 +184,13 @@ func hsRequestHandler(server *Server, client *Client, command, params string, rb
 		return
 	}
 
-	err = server.accounts.VHostRequest(accountName, vhost)
+	_, err = server.accounts.VHostRequest(accountName, vhost)
 	if err != nil {
 		hsNotice(rb, client.t("An error occurred"))
 	} else {
 		hsNotice(rb, fmt.Sprintf(client.t("Your vhost request will be reviewed by an administrator")))
+		chanMsg := fmt.Sprintf("Account %s requests vhost %s", accountName, vhost)
+		hsNotifyChannel(server, chanMsg)
 		// TODO send admins a snomask of some kind
 	}
 }
@@ -208,10 +223,10 @@ func hsStatusHandler(server *Server, client *Client, command, params string, rb
 
 func validateVhost(server *Server, vhost string, oper bool) error {
 	ac := server.AccountConfig()
-	if len(vhost) > ac.HostServ.MaxVHostLen {
+	if len(vhost) > ac.VHosts.MaxLength {
 		return errVHostTooLong
 	}
-	if !ac.HostServ.ValidRegexp.MatchString(vhost) {
+	if !ac.VHosts.ValidRegexp.MatchString(vhost) {
 		return errVHostBadCharacters
 	}
 	return nil
@@ -235,7 +250,7 @@ func hsSetHandler(server *Server, client *Client, command, params string, rb *Re
 		return
 	}
 
-	err := server.accounts.VHostSet(user, vhost)
+	_, err := server.accounts.VHostSet(user, vhost)
 	if err != nil {
 		hsNotice(rb, client.t("An error occurred"))
 	} else if vhost != "" {
@@ -260,11 +275,13 @@ func hsApproveHandler(server *Server, client *Client, command, params string, rb
 		return
 	}
 
-	err := server.accounts.VHostApprove(user)
+	vhostInfo, err := server.accounts.VHostApprove(user)
 	if err != nil {
 		hsNotice(rb, client.t("An error occurred"))
 	} else {
 		hsNotice(rb, fmt.Sprintf(client.t("Successfully approved vhost request for %s"), user))
+		chanMsg := fmt.Sprintf("Oper %s approved vhost %s for account %s", client.Nick(), vhostInfo.ApprovedVHost, user)
+		hsNotifyChannel(server, chanMsg)
 		for _, client := range server.accounts.AccountToClients(user) {
 			client.Notice(client.t("Your vhost request was approved by an administrator"))
 		}
@@ -279,11 +296,13 @@ func hsRejectHandler(server *Server, client *Client, command, params string, rb
 	}
 	reason := strings.TrimSpace(params)
 
-	err := server.accounts.VHostReject(user, reason)
+	vhostInfo, err := server.accounts.VHostReject(user, reason)
 	if err != nil {
 		hsNotice(rb, client.t("An error occurred"))
 	} else {
 		hsNotice(rb, fmt.Sprintf(client.t("Successfully rejected vhost request for %s"), user))
+		chanMsg := fmt.Sprintf("Oper %s rejected vhost %s for account %s, with the reason: %v", client.Nick(), vhostInfo.RejectedVHost, user, reason)
+		hsNotifyChannel(server, chanMsg)
 		for _, client := range server.accounts.AccountToClients(user) {
 			if reason == "" {
 				client.Notice("Your vhost request was rejected by an administrator")

irc/server.go

@@ -858,8 +858,8 @@ func (server *Server) applyConfig(config *Config, initial bool) error {
 		server.accounts.buildNickToAccountIndex()
 	}
 
-	hsPreviouslyDisabled := oldAccountConfig != nil && !oldAccountConfig.HostServ.Enabled
+	hsPreviouslyDisabled := oldAccountConfig != nil && !oldAccountConfig.VHosts.Enabled
-	hsNowEnabled := config.Accounts.HostServ.Enabled
+	hsNowEnabled := config.Accounts.VHosts.Enabled
 	if hsPreviouslyDisabled && hsNowEnabled {
 		server.accounts.initVHostRequestQueue()
 	}

oragono.yaml

@@ -198,21 +198,36 @@ accounts:
         # rename-prefix - this is the prefix to use when renaming clients (e.g. Guest-AB54U31)
         rename-prefix: Guest-
 
-    hostserv:
+    # vhosts controls the assignment of vhosts (strings displayed in place of the user's
-        # is hostserv enabled at all?
+    # hostname/IP) by the HostServ service
+    vhosts:
+        # are vhosts enabled at all?
         enabled: true
-        # can users request vhosts from operators? if this is false, operators with the
+
-        # "hostserv" capability can still assign vhosts manually:
-        user-requests-enabled: false
-        # to prevent users from spamming requests, they must wait at least this long between
-        # distinct requests:
-        cooldown: 1h
         # maximum length of a vhost
-        max-vhost-len: 64
+        max-length: 64
+
         # regexp for testing the validity of a vhost
         # (make sure any changes you make here are RFC-compliant)
         valid-regexp: '^[0-9A-Za-z.\-_/]+$'
 
+        # options controlling users requesting vhosts:
+        user-requests:
+            # can users request vhosts at all? if this is false, operators with the
+            # 'vhosts' capability can still assign vhosts manually
+            enabled: false
+
+            # if uncommented, all new vhost requests will be dumped into the given
+            # channel, so opers can review them as they are sent in. ensure that you
+            # have registered and restricted the channel appropriately before you
+            # uncomment this.
+            #channel: "#vhosts"
+
+            # after a user's vhost has been approved or rejected, they need to wait
+            # this long (starting from the time of their original request)
+            # before they can request a new one.
+            cooldown: 168h
+
 # channel options
 channels:
     # modes that are set when new channels are created
@@ -267,7 +282,7 @@ oper-classes:
             - "oper:die"
             - "unregister"
             - "samode"
-            - "hostserv"
+            - "vhosts"
 
 # ircd operators
 opers: