Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

[CloudSaas] - Send reactivate account link for locked accounts #33810

Merged
merged 12 commits into from
Apr 13, 2022
11 changes: 5 additions & 6 deletions client/web/src/auth/UnlockAccount.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { getReturnTo } from './SignInSignUpCommon'

import unlockAccountStyles from './SignInSignUpCommon.module.scss'

interface UnlockAccountPageProps extends RouteComponentProps<{ token: string; userId: string }> {
interface UnlockAccountPageProps extends RouteComponentProps<{ token: string }> {
location: H.Location
authenticatedUser: AuthenticatedUser | null
context: Pick<
Expand All @@ -29,7 +29,7 @@ interface UnlockAccountPageProps extends RouteComponentProps<{ token: string; us
export const UnlockAccountPage: React.FunctionComponent<UnlockAccountPageProps> = props => {
const [error, setError] = useState<Error | null>(null)
const [loading, setLoading] = useState(true)
const { token, userId } = props.match.params
const { token } = props.match.params

const unlockAccount = React.useCallback(async (): Promise<void> => {
try {
Expand All @@ -44,22 +44,21 @@ export const UnlockAccountPage: React.FunctionComponent<UnlockAccountPageProps>
},
body: JSON.stringify({
token,
userId: Number(userId),
}),
})

if (!response.ok) {
throw new Error('The url you provided is either expired or invalid.')
}

eventLogger.log('OkUnlockAccount', { userId, token })
eventLogger.log('OkUnlockAccount', { token })
} catch (error) {
setError(error)
eventLogger.log('KoUnlockAccount', { userId, token })
eventLogger.log('KoUnlockAccount', { token })
pietrorosa77 marked this conversation as resolved.
Show resolved Hide resolved
} finally {
setLoading(false)
}
}, [userId, token, props.context.xhrHeaders])
}, [token, props.context.xhrHeaders])

useEffect(() => {
if (props.authenticatedUser) {
Expand Down
2 changes: 1 addition & 1 deletion client/web/src/routes.constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export enum PageRoutes {
SearchNotebook = '/search/notebook',
SignIn = '/sign-in',
SignUp = '/sign-up',
UnlockAccount = '/unlock-account/:token/:userId',
UnlockAccount = '/unlock-account/:token',
Welcome = '/welcome',
Settings = '/settings',
User = '/user',
Expand Down
2 changes: 1 addition & 1 deletion cmd/frontend/internal/app/ui/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func newRouter() *muxtrace.Router {
r.Path("/search/console").Methods("GET").Name(routeSearchConsole)
r.Path("/sign-in").Methods("GET").Name(uirouter.RouteSignIn)
r.Path("/sign-up").Methods("GET").Name(uirouter.RouteSignUp)
r.Path("/unlock-account/{token}/{userId}").Methods("GET").Name(uirouter.RouteUnlockAccount)
r.Path("/unlock-account/{token}").Methods("GET").Name(uirouter.RouteUnlockAccount)
r.Path("/welcome").Methods("GET").Name(routeWelcome)
r.PathPrefix("/insights").Methods("GET").Name(routeInsights)
r.PathPrefix("/batch-changes").Methods("GET").Name(routeBatchChanges)
Expand Down
35 changes: 17 additions & 18 deletions cmd/frontend/internal/auth/userpasswd/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ type credentials struct {
}

type unlockAccountInfo struct {
Token string `json:"token"`
UserID int32 `json:"userId"`
Token string `json:"token"`
}

// HandleSignUp handles submission of the user signup form.
Expand Down Expand Up @@ -287,17 +286,19 @@ func HandleSignIn(db database.DB, store LockoutStore) func(w http.ResponseWriter
user = *u

if reason, locked := store.IsLockedOut(user.ID); locked {
recipient, verified, err := db.UserEmails().GetPrimaryEmail(ctx, user.ID)
if !verified || err != nil {
// log the error and proceed
log15.Error(fmt.Sprintf("Impossible to get primary email address for userId %d. Unlock account email can't be sent", user.ID), err)
}

err = store.SendUnlockAccountEmail(ctx, user.ID, recipient)

if err != nil {
// log the error and proceed
log15.Error(fmt.Sprintf("Impossible to send unlock account email for userId %d", user.ID), err)
if conf.CanSendEmail() {
recipient, verified, err := db.UserEmails().GetPrimaryEmail(ctx, user.ID)
if !verified || err != nil {
// log the error and proceed
log15.Error(fmt.Sprintf("Impossible to get primary email address for userId %d. Unlock account email can't be sent", user.ID), err)
}

err = store.SendUnlockAccountEmail(ctx, user.ID, recipient)
Copy link
Member

Choose a reason for hiding this comment

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

What is the meaning of having an invalid recipient here (when we logged and proceeded with an error)?


if err != nil {
// log the error and proceed
log15.Error(fmt.Sprintf("Impossible to send unlock account email for userId %d", user.ID), err)
Comment on lines +293 to +300
Copy link
Member

Choose a reason for hiding this comment

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

log15 is structural logging, so you would actually want:

log15.Error("Impossible to get primary email address for. Unlock account email can't be sent", "userID", user.ID, "error", err)

Copy link
Member

Choose a reason for hiding this comment

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

The current usage is actually causing server panics when the logging happens, will include the fix in a PR I'm currently working on.

}
}

httpLogAndError(w, fmt.Sprintf("Account has been locked out due to %q", reason), http.StatusUnprocessableEntity)
Expand Down Expand Up @@ -346,12 +347,12 @@ func HandleUnlockAccount(db database.DB, store LockoutStore) func(w http.Respons
return
}

if unlockAccountInfo.UserID == 0 || unlockAccountInfo.Token == "" {
http.Error(w, "Bad request: missing token or user id", http.StatusBadRequest)
if unlockAccountInfo.Token == "" {
http.Error(w, "Bad request: missing token", http.StatusBadRequest)
return
}

valid, error := store.VerifyUnlockAccountToken(unlockAccountInfo.UserID, unlockAccountInfo.Token)
valid, error := store.VerifyUnlockAccountToken(unlockAccountInfo.Token)

if !valid || error != nil {
err := "invalid token provided"
Expand All @@ -361,8 +362,6 @@ func HandleUnlockAccount(db database.DB, store LockoutStore) func(w http.Respons
httpLogAndError(w, err, http.StatusUnauthorized)
return
}

store.Reset(unlockAccountInfo.UserID)
}
}

Expand Down
6 changes: 3 additions & 3 deletions cmd/frontend/internal/auth/userpasswd/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,13 +186,13 @@ func TestHandleAccount_Unlock(t *testing.T) {
resp := httptest.NewRecorder()
h(resp, req)
assert.Equal(t, http.StatusBadRequest, resp.Code)
assert.Equal(t, "Bad request: missing token or user id\n", resp.Body.String())
assert.Equal(t, "Bad request: missing token\n", resp.Body.String())
}

// Getting error for invalid token
{
lockout.VerifyUnlockAccountTokenFunc.SetDefaultReturn(false, errors.Newf("invalid token provided"))
req, err := http.NewRequest(http.MethodPost, "/", strings.NewReader(`{ "userId": 1, "token": "abcd" }`))
req, err := http.NewRequest(http.MethodPost, "/", strings.NewReader(`{ "token": "abcd" }`))
require.NoError(t, err)

resp := httptest.NewRecorder()
Expand All @@ -205,7 +205,7 @@ func TestHandleAccount_Unlock(t *testing.T) {
// ok result
{
lockout.VerifyUnlockAccountTokenFunc.SetDefaultReturn(true, nil)
req, err := http.NewRequest(http.MethodPost, "/", strings.NewReader(`{ "userId": 1, "token": "eyJhbGciOiJIUzUxMiIsInR5cCI6IkpXVCJ9.eyJ1c2VyX2lkIjoxLCJpc3MiOiJodHRwczovL3NvdXJjZWdyYXBoLnRlc3Q6MzQ0MyIsInN1YiI6IjEiLCJleHAiOjE2NDk3NzgxNjl9.cm_giwkSviVRXGRCie9iii-ytJD3iAuNdtk9XmBZMrj7HHlH6vfky4ftjudAZ94HBp867cjxkuNc6OJ2uaEJFg" }`))
req, err := http.NewRequest(http.MethodPost, "/", strings.NewReader(`{ "token": "eyJhbGciOiJIUzUxMiIsInR5cCI6IkpXVCJ9.eyJ1c2VyX2lkIjoxLCJpc3MiOiJodHRwczovL3NvdXJjZWdyYXBoLnRlc3Q6MzQ0MyIsInN1YiI6IjEiLCJleHAiOjE2NDk3NzgxNjl9.cm_giwkSviVRXGRCie9iii-ytJD3iAuNdtk9XmBZMrj7HHlH6vfky4ftjudAZ94HBp867cjxkuNc6OJ2uaEJFg" }`))
require.NoError(t, err)

resp := httptest.NewRecorder()
Expand Down
31 changes: 17 additions & 14 deletions cmd/frontend/internal/auth/userpasswd/lockout.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type LockoutStore interface {
// Creates the unlock account url with the signet unlock token
GenerateUnlockAccountUrl(userID int32) (string, string, error)
// Verifies the provided unlock token is valid
VerifyUnlockAccountToken(userID int32, urlToken string) (bool, error)
VerifyUnlockAccountToken(urlToken string) (bool, error)
// Sends an email to the locked account email providing a temporary unlock link
SendUnlockAccountEmail(ctx context.Context, userID int32, userEmail string) error
}
Expand Down Expand Up @@ -79,10 +79,10 @@ func (s *lockoutStore) GenerateUnlockAccountUrl(userID int32) (string, string, e
signingKey := conf.SiteConfig().AuthUnlockAccountLinkSigningKey

if signingKey == "" {
return "", "", errors.Newf("signing key not provided, cannot validate JWT on invitation URL. Please add AuthUnlockAccountLinkSigningKey to site configuration.")
return "", "", errors.Newf("signing key not provided, cannot validate JWT on unlock account URL. Please add AuthUnlockAccountLinkSigningKey to site configuration.")
}

defaultExpiryMinutes := 5
defaultExpiryMinutes := 30
if conf.SiteConfig().AuthUnlockAccountLinkExpiry > 0 {
defaultExpiryMinutes = conf.SiteConfig().AuthUnlockAccountLinkExpiry
}
Expand Down Expand Up @@ -111,7 +111,7 @@ func (s *lockoutStore) GenerateUnlockAccountUrl(userID int32) (string, string, e

s.unlockToken.Set(key, []byte(tokenString))

path := fmt.Sprintf("/unlock-account/%s/%d", tokenString, userID)
path := fmt.Sprintf("/unlock-account/%s", tokenString)

return globals.ExternalURL().ResolveReference(&url.URL{Path: path}).String(), tokenString, nil
}
Expand All @@ -136,18 +136,11 @@ func (s *lockoutStore) SendUnlockAccountEmail(ctx context.Context, userID int32,
})
}

func (s *lockoutStore) VerifyUnlockAccountToken(userID int32, urlToken string) (bool, error) {
func (s *lockoutStore) VerifyUnlockAccountToken(urlToken string) (bool, error) {
signingKey := conf.SiteConfig().AuthUnlockAccountLinkSigningKey

if signingKey == "" {
return false, errors.Newf("signing key not provided, cannot validate JWT on invitation URL. Please add AuthUnlockAccountLinkSigningKey to site configuration.")
}

userIdKey := strconv.Itoa(int(userID))
storedToken, ok := s.unlockToken.Get(userIdKey)

if !ok {
return false, errors.Newf("No token exists for the specified user")
return false, errors.Newf("signing key not provided, cannot validate JWT on account reset URL. Please add AuthUnlockAccountLinkSigningKey to site configuration.")
}

token, err := jwt.ParseWithClaims(urlToken, &unlockAccountClaims{}, func(token *jwt.Token) (interface{}, error) {
Expand All @@ -158,7 +151,15 @@ func (s *lockoutStore) VerifyUnlockAccountToken(userID int32, urlToken string) (
return false, err
}

if claims, ok := token.Claims.(*unlockAccountClaims); ok && token.Valid && userID == claims.UserID && string(storedToken) == urlToken {
if claims, ok := token.Claims.(*unlockAccountClaims); ok && token.Valid {
userIdKey := strconv.Itoa(int(claims.UserID))
storedToken, found := s.unlockToken.Get(userIdKey)

if !found || string(storedToken) != urlToken {
return false, errors.Newf("No previously generated token exists for the specified user")
}

s.Reset(claims.UserID)
pietrorosa77 marked this conversation as resolved.
Show resolved Hide resolved
return true, nil
}

Expand All @@ -169,6 +170,7 @@ func (s *lockoutStore) Reset(userID int32) {
key := strconv.Itoa(int(userID))
s.lockouts.Delete(key)
s.failedAttempts.Delete(key)
s.unlockToken.Delete(key)
}

var emailTemplates = txemail.MustValidate(txtypes.Templates{
Expand Down Expand Up @@ -212,6 +214,7 @@ Sourcegraph, 981 Mission St, San Francisco, CA 94103, USA
<p class="mtxl">
Please, follow this link in your browser to unlock your account and try to sign in again: <a class="btn mtm" href="{{.UnlockAccountUrl}}">Unlock your Account</a>
</p>
<p class="smaller">Or visit this link in your browser: <a href="{{.UnlockAccountUrl}}">{{.UnlockAccountUrl}}</a></p>
<small>
<p class="mtl">
This link will expire in {{.ExpiryMinutes}} minutes.
Expand Down
10 changes: 6 additions & 4 deletions cmd/frontend/internal/auth/userpasswd/lockout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func TestLockoutStore(t *testing.T) {

path, _, err := s.GenerateUnlockAccountUrl(1)

assert.EqualError(t, err, "signing key not provided, cannot validate JWT on invitation URL. Please add AuthUnlockAccountLinkSigningKey to site configuration.")
assert.EqualError(t, err, "signing key not provided, cannot validate JWT on unlock account URL. Please add AuthUnlockAccountLinkSigningKey to site configuration.")
assert.Empty(t, path)

})
Expand Down Expand Up @@ -172,7 +172,7 @@ func TestLockoutStore(t *testing.T) {

assert.Empty(t, err)

valid, error := s.VerifyUnlockAccountToken(1, token)
valid, error := s.VerifyUnlockAccountToken(token)

assert.Empty(t, error)

Expand All @@ -194,9 +194,11 @@ func TestLockoutStore(t *testing.T) {

assert.Empty(t, err)

valid, error := s.VerifyUnlockAccountToken(2, token)
s.Reset(1)

valid, error := s.VerifyUnlockAccountToken(token)

assert.EqualError(t, error, "No token exists for the specified user")
assert.EqualError(t, error, "No previously generated token exists for the specified user")
assert.False(t, valid)
})
}
31 changes: 14 additions & 17 deletions cmd/frontend/internal/auth/userpasswd/mocks.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions doc/cli/references/search.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,6 @@ Other tips:

Query syntax: https://about.sourcegraph.com/docs/search/query-syntax/

Be careful with search strings including negation: a search with an initial
negated term may be parsed as a flag rather than as a search string. You can
use -- to ensure that src parses this correctly, eg:

$ src search -- '-repo:github.com/foo/bar error'


```