mirror of https://github.com/coder/coder.git
fix(audit): audit login/logout for new 3rd-party auth (#6733)
* fix(audit): audit login/logout for new 3rd-party auth * no longer auditing unknown users
This commit is contained in:
parent
df31636e72
commit
25e92fd2f4
|
@ -154,9 +154,7 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
|
|||
if ResourceID(req.Old) == uuid.Nil && ResourceID(req.New) == uuid.Nil {
|
||||
// If the request action is a login or logout, we always want to audit it even if
|
||||
// there is no diff. This is so we can capture events where an API Key is never created
|
||||
// because an unknown user fails to login.
|
||||
// TODO: introduce the concept of an anonymous user so we always have a userID even
|
||||
// when dealing with a mystery user. https://github.com/coder/coder/issues/6054
|
||||
// because a known user fails to login.
|
||||
if req.params.Action != database.AuditActionLogin && req.params.Action != database.AuditActionLogout {
|
||||
return
|
||||
}
|
||||
|
@ -185,8 +183,13 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
|
|||
key, ok := httpmw.APIKeyOptional(p.Request)
|
||||
if ok {
|
||||
userID = key.UserID
|
||||
} else {
|
||||
} else if req.UserID != uuid.Nil {
|
||||
userID = req.UserID
|
||||
} else {
|
||||
// if we do not have a user associated with the audit action
|
||||
// we do not want to audit
|
||||
// (this pertains to logins; we don't want to capture non-user login attempts)
|
||||
return
|
||||
}
|
||||
|
||||
ip := parseIP(p.Request.RemoteAddr)
|
||||
|
|
|
@ -424,7 +424,6 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
|
|||
})
|
||||
return
|
||||
}
|
||||
aReq.UserID = user.ID
|
||||
|
||||
cookie, key, err := api.oauthLogin(r, oauthLoginParams{
|
||||
User: user,
|
||||
|
@ -453,6 +452,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
|
|||
return
|
||||
}
|
||||
aReq.New = key
|
||||
aReq.UserID = key.UserID
|
||||
|
||||
http.SetCookie(rw, cookie)
|
||||
|
||||
|
@ -714,7 +714,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
|
|||
})
|
||||
return
|
||||
}
|
||||
aReq.UserID = user.ID
|
||||
|
||||
cookie, key, err := api.oauthLogin(r, oauthLoginParams{
|
||||
User: user,
|
||||
|
@ -745,6 +744,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
|
|||
return
|
||||
}
|
||||
aReq.New = key
|
||||
aReq.UserID = key.UserID
|
||||
|
||||
http.SetCookie(rw, cookie)
|
||||
|
||||
|
|
|
@ -12,6 +12,7 @@ import (
|
|||
"github.com/coreos/go-oidc/v3/oidc"
|
||||
"github.com/golang-jwt/jwt"
|
||||
"github.com/google/go-github/v43/github"
|
||||
"github.com/google/uuid"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/oauth2"
|
||||
|
@ -64,9 +65,7 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||
|
||||
t.Run("NotInAllowedOrganization", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
client := coderdtest.New(t, &coderdtest.Options{
|
||||
Auditor: auditor,
|
||||
GithubOAuth2Config: &coderd.GithubOAuth2Config{
|
||||
OAuth2Config: &testutil.OAuth2Config{},
|
||||
ListOrganizationMemberships: func(ctx context.Context, client *http.Client) ([]*github.Membership, error) {
|
||||
|
@ -79,19 +78,13 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||
},
|
||||
},
|
||||
})
|
||||
numLogs := len(auditor.AuditLogs)
|
||||
|
||||
resp := oauth2Callback(t, client)
|
||||
numLogs++ // add an audit log for login
|
||||
require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
t.Run("NotInAllowedTeam", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
client := coderdtest.New(t, &coderdtest.Options{
|
||||
Auditor: auditor,
|
||||
GithubOAuth2Config: &coderd.GithubOAuth2Config{
|
||||
AllowOrganizations: []string{"coder"},
|
||||
AllowTeams: []coderd.GithubOAuth2Team{{"another", "something"}, {"coder", "frontend"}},
|
||||
|
@ -114,20 +107,13 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||
},
|
||||
},
|
||||
})
|
||||
numLogs := len(auditor.AuditLogs)
|
||||
|
||||
resp := oauth2Callback(t, client)
|
||||
numLogs++ // add an audit log for login
|
||||
|
||||
require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
t.Run("UnverifiedEmail", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
client := coderdtest.New(t, &coderdtest.Options{
|
||||
Auditor: auditor,
|
||||
GithubOAuth2Config: &coderd.GithubOAuth2Config{
|
||||
OAuth2Config: &testutil.OAuth2Config{},
|
||||
AllowOrganizations: []string{"coder"},
|
||||
|
@ -150,23 +136,16 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||
},
|
||||
},
|
||||
})
|
||||
numLogs := len(auditor.AuditLogs)
|
||||
|
||||
_ = coderdtest.CreateFirstUser(t, client)
|
||||
numLogs++ // add an audit log for user create
|
||||
|
||||
resp := oauth2Callback(t, client)
|
||||
numLogs++ // add an audit log for login
|
||||
|
||||
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
t.Run("BlockSignups", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
client := coderdtest.New(t, &coderdtest.Options{
|
||||
Auditor: auditor,
|
||||
GithubOAuth2Config: &coderd.GithubOAuth2Config{
|
||||
OAuth2Config: &testutil.OAuth2Config{},
|
||||
AllowOrganizations: []string{"coder"},
|
||||
|
@ -190,20 +169,14 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||
},
|
||||
},
|
||||
})
|
||||
numLogs := len(auditor.AuditLogs)
|
||||
|
||||
resp := oauth2Callback(t, client)
|
||||
numLogs++ // add an audit log for login
|
||||
|
||||
require.Equal(t, http.StatusForbidden, resp.StatusCode)
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
t.Run("MultiLoginNotAllowed", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
client := coderdtest.New(t, &coderdtest.Options{
|
||||
Auditor: auditor,
|
||||
GithubOAuth2Config: &coderd.GithubOAuth2Config{
|
||||
OAuth2Config: &testutil.OAuth2Config{},
|
||||
AllowOrganizations: []string{"coder"},
|
||||
|
@ -227,20 +200,15 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||
},
|
||||
},
|
||||
})
|
||||
numLogs := len(auditor.AuditLogs)
|
||||
|
||||
// Creates the first user with login_type 'password'.
|
||||
_ = coderdtest.CreateFirstUser(t, client)
|
||||
numLogs++ // add an audit log for user create
|
||||
|
||||
// Attempting to login should give us a 403 since the user
|
||||
// already has a login_type of 'password'.
|
||||
resp := oauth2Callback(t, client)
|
||||
numLogs++ // add an audit log for login
|
||||
|
||||
require.Equal(t, http.StatusForbidden, resp.StatusCode)
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
t.Run("Signup", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
@ -290,6 +258,7 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||
require.Equal(t, "/hello-world", user.AvatarURL)
|
||||
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.NotEqual(t, auditor.AuditLogs[numLogs-1].UserID, uuid.Nil)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
t.Run("SignupAllowedTeam", func(t *testing.T) {
|
||||
|
@ -480,9 +449,7 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||
})
|
||||
t.Run("SignupFailedInactiveInOrg", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
client := coderdtest.New(t, &coderdtest.Options{
|
||||
Auditor: auditor,
|
||||
GithubOAuth2Config: &coderd.GithubOAuth2Config{
|
||||
AllowSignups: true,
|
||||
AllowOrganizations: []string{"coder"},
|
||||
|
@ -513,14 +480,10 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||
},
|
||||
},
|
||||
})
|
||||
numLogs := len(auditor.AuditLogs)
|
||||
|
||||
resp := oauth2Callback(t, client)
|
||||
numLogs++ // add an audit log for login
|
||||
|
||||
require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
}
|
||||
|
||||
|
@ -711,6 +674,7 @@ func TestUserOIDC(t *testing.T) {
|
|||
require.Equal(t, tc.Username, user.Username)
|
||||
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.NotEqual(t, auditor.AuditLogs[numLogs-1].UserID, uuid.Nil)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
}
|
||||
|
||||
|
@ -784,33 +748,24 @@ func TestUserOIDC(t *testing.T) {
|
|||
|
||||
t.Run("NoIDToken", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
client := coderdtest.New(t, &coderdtest.Options{
|
||||
Auditor: auditor,
|
||||
OIDCConfig: &coderd.OIDCConfig{
|
||||
OAuth2Config: &testutil.OAuth2Config{},
|
||||
},
|
||||
})
|
||||
numLogs := len(auditor.AuditLogs)
|
||||
|
||||
resp := oidcCallback(t, client, "asdf")
|
||||
numLogs++ // add an audit log for login
|
||||
|
||||
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
|
||||
t.Run("BadVerify", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
verifier := oidc.NewVerifier("", &oidc.StaticKeySet{
|
||||
PublicKeys: []crypto.PublicKey{},
|
||||
}, &oidc.Config{})
|
||||
provider := &oidc.Provider{}
|
||||
|
||||
client := coderdtest.New(t, &coderdtest.Options{
|
||||
Auditor: auditor,
|
||||
OIDCConfig: &coderd.OIDCConfig{
|
||||
OAuth2Config: &testutil.OAuth2Config{
|
||||
Token: (&oauth2.Token{
|
||||
|
@ -823,14 +778,10 @@ func TestUserOIDC(t *testing.T) {
|
|||
Verifier: verifier,
|
||||
},
|
||||
})
|
||||
numLogs := len(auditor.AuditLogs)
|
||||
|
||||
resp := oidcCallback(t, client, "asdf")
|
||||
numLogs++ // add an audit log for login
|
||||
|
||||
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
@ -92,10 +92,7 @@ func TestPostLogin(t *testing.T) {
|
|||
t.Parallel()
|
||||
t.Run("InvalidUser", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
|
||||
numLogs := len(auditor.AuditLogs)
|
||||
|
||||
client := coderdtest.New(t, nil)
|
||||
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
|
||||
defer cancel()
|
||||
|
||||
|
@ -103,13 +100,9 @@ func TestPostLogin(t *testing.T) {
|
|||
Email: "my@email.org",
|
||||
Password: "password",
|
||||
})
|
||||
numLogs++ // add an audit log for login
|
||||
var apiErr *codersdk.Error
|
||||
require.ErrorAs(t, err, &apiErr)
|
||||
require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode())
|
||||
|
||||
require.Len(t, auditor.AuditLogs, numLogs)
|
||||
require.Equal(t, database.AuditActionLogin, auditor.AuditLogs[numLogs-1].Action)
|
||||
})
|
||||
|
||||
t.Run("BadPassword", func(t *testing.T) {
|
||||
|
|
|
@ -4,7 +4,6 @@ import {
|
|||
MockWorkspaceCreateAuditLogForDifferentOwner,
|
||||
MockAuditLogSuccessfulLogin,
|
||||
MockAuditLogUnsuccessfulLoginKnownUser,
|
||||
MockAuditLogUnsuccessfulLoginUnknownUser,
|
||||
} from "testHelpers/entities"
|
||||
import { AuditLogDescription } from "./AuditLogDescription"
|
||||
import { AuditLogRow } from "../AuditLogRow"
|
||||
|
@ -101,25 +100,6 @@ describe("AuditLogDescription", () => {
|
|||
),
|
||||
).toBeInTheDocument()
|
||||
|
||||
const statusPill = screen.getByRole("status")
|
||||
expect(statusPill).toHaveTextContent("401")
|
||||
})
|
||||
it("renders the correct string for unsuccessful login for an unknown user", async () => {
|
||||
render(<AuditLogRow auditLog={MockAuditLogUnsuccessfulLoginUnknownUser} />)
|
||||
|
||||
expect(
|
||||
screen.getByText(
|
||||
t("auditLog:table.logRow.description.unlinkedAuditDescription", {
|
||||
truncatedDescription: "an unknown user logged in",
|
||||
target: "",
|
||||
onBehalfOf: undefined,
|
||||
})
|
||||
.replace(/<[^>]*>/g, " ")
|
||||
.replace(/\s{2,}/g, " ")
|
||||
.trim(),
|
||||
),
|
||||
).toBeInTheDocument()
|
||||
|
||||
const statusPill = screen.getByRole("status")
|
||||
expect(statusPill).toHaveTextContent("401")
|
||||
})
|
||||
|
|
|
@ -11,7 +11,7 @@ export const AuditLogDescription: FC<{ auditLog: AuditLog }> = ({
|
|||
const { t } = useTranslation("auditLog")
|
||||
|
||||
let target = auditLog.resource_target.trim()
|
||||
const user = auditLog.user ? auditLog.user.username.trim() : "an unknown user"
|
||||
const user = auditLog.user?.username.trim()
|
||||
|
||||
if (auditLog.resource_type === "workspace_build") {
|
||||
return <BuildAuditDescription auditLog={auditLog} />
|
||||
|
@ -30,7 +30,7 @@ export const AuditLogDescription: FC<{ auditLog: AuditLog }> = ({
|
|||
const onBehalfOf =
|
||||
auditLog.additional_fields.workspace_owner &&
|
||||
auditLog.additional_fields.workspace_owner !== "unknown" &&
|
||||
auditLog.additional_fields.workspace_owner !== auditLog.user?.username
|
||||
auditLog.additional_fields.workspace_owner.trim() !== user
|
||||
? `on behalf of ${auditLog.additional_fields.workspace_owner}`
|
||||
: ""
|
||||
|
||||
|
|
|
@ -1384,12 +1384,6 @@ export const MockAuditLogUnsuccessfulLoginKnownUser: TypesGen.AuditLog = {
|
|||
status_code: 401,
|
||||
}
|
||||
|
||||
export const MockAuditLogUnsuccessfulLoginUnknownUser: TypesGen.AuditLog = {
|
||||
...MockAuditLogSuccessfulLogin,
|
||||
status_code: 401,
|
||||
user: undefined,
|
||||
}
|
||||
|
||||
export const MockWorkspaceQuota: TypesGen.WorkspaceQuota = {
|
||||
credits_consumed: 0,
|
||||
budget: 100,
|
||||
|
|
Loading…
Reference in New Issue