Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit25e92fd

Browse files
authored
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
1 parentdf31636 commit25e92fd

File tree

7 files changed

+15
-94
lines changed

7 files changed

+15
-94
lines changed

‎coderd/audit/request.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,7 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
154154
ifResourceID(req.Old)==uuid.Nil&&ResourceID(req.New)==uuid.Nil {
155155
// If the request action is a login or logout, we always want to audit it even if
156156
// there is no diff. This is so we can capture events where an API Key is never created
157-
// because an unknown user fails to login.
158-
// TODO: introduce the concept of an anonymous user so we always have a userID even
159-
// when dealing with a mystery user. https://github.com/coder/coder/issues/6054
157+
// because a known user fails to login.
160158
ifreq.params.Action!=database.AuditActionLogin&&req.params.Action!=database.AuditActionLogout {
161159
return
162160
}
@@ -185,8 +183,13 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
185183
key,ok:=httpmw.APIKeyOptional(p.Request)
186184
ifok {
187185
userID=key.UserID
188-
}else {
186+
}elseifreq.UserID!=uuid.Nil{
189187
userID=req.UserID
188+
}else {
189+
// if we do not have a user associated with the audit action
190+
// we do not want to audit
191+
// (this pertains to logins; we don't want to capture non-user login attempts)
192+
return
190193
}
191194

192195
ip:=parseIP(p.Request.RemoteAddr)

‎coderd/userauth.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -424,7 +424,6 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
424424
})
425425
return
426426
}
427-
aReq.UserID=user.ID
428427

429428
cookie,key,err:=api.oauthLogin(r,oauthLoginParams{
430429
User:user,
@@ -453,6 +452,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
453452
return
454453
}
455454
aReq.New=key
455+
aReq.UserID=key.UserID
456456

457457
http.SetCookie(rw,cookie)
458458

@@ -714,7 +714,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
714714
})
715715
return
716716
}
717-
aReq.UserID=user.ID
718717

719718
cookie,key,err:=api.oauthLogin(r,oauthLoginParams{
720719
User:user,
@@ -745,6 +744,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
745744
return
746745
}
747746
aReq.New=key
747+
aReq.UserID=key.UserID
748748

749749
http.SetCookie(rw,cookie)
750750

‎coderd/userauth_test.go

Lines changed: 3 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"github.com/coreos/go-oidc/v3/oidc"
1313
"github.com/golang-jwt/jwt"
1414
"github.com/google/go-github/v43/github"
15+
"github.com/google/uuid"
1516
"github.com/stretchr/testify/assert"
1617
"github.com/stretchr/testify/require"
1718
"golang.org/x/oauth2"
@@ -64,9 +65,7 @@ func TestUserOAuth2Github(t *testing.T) {
6465

6566
t.Run("NotInAllowedOrganization",func(t*testing.T) {
6667
t.Parallel()
67-
auditor:=audit.NewMock()
6868
client:=coderdtest.New(t,&coderdtest.Options{
69-
Auditor:auditor,
7069
GithubOAuth2Config:&coderd.GithubOAuth2Config{
7170
OAuth2Config:&testutil.OAuth2Config{},
7271
ListOrganizationMemberships:func(ctx context.Context,client*http.Client) ([]*github.Membership,error) {
@@ -79,19 +78,13 @@ func TestUserOAuth2Github(t *testing.T) {
7978
},
8079
},
8180
})
82-
numLogs:=len(auditor.AuditLogs)
8381

8482
resp:=oauth2Callback(t,client)
85-
numLogs++// add an audit log for login
8683
require.Equal(t,http.StatusUnauthorized,resp.StatusCode)
87-
require.Len(t,auditor.AuditLogs,numLogs)
88-
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
8984
})
9085
t.Run("NotInAllowedTeam",func(t*testing.T) {
9186
t.Parallel()
92-
auditor:=audit.NewMock()
9387
client:=coderdtest.New(t,&coderdtest.Options{
94-
Auditor:auditor,
9588
GithubOAuth2Config:&coderd.GithubOAuth2Config{
9689
AllowOrganizations: []string{"coder"},
9790
AllowTeams: []coderd.GithubOAuth2Team{{"another","something"}, {"coder","frontend"}},
@@ -114,20 +107,13 @@ func TestUserOAuth2Github(t *testing.T) {
114107
},
115108
},
116109
})
117-
numLogs:=len(auditor.AuditLogs)
118110

119111
resp:=oauth2Callback(t,client)
120-
numLogs++// add an audit log for login
121-
122112
require.Equal(t,http.StatusUnauthorized,resp.StatusCode)
123-
require.Len(t,auditor.AuditLogs,numLogs)
124-
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
125113
})
126114
t.Run("UnverifiedEmail",func(t*testing.T) {
127115
t.Parallel()
128-
auditor:=audit.NewMock()
129116
client:=coderdtest.New(t,&coderdtest.Options{
130-
Auditor:auditor,
131117
GithubOAuth2Config:&coderd.GithubOAuth2Config{
132118
OAuth2Config:&testutil.OAuth2Config{},
133119
AllowOrganizations: []string{"coder"},
@@ -150,23 +136,16 @@ func TestUserOAuth2Github(t *testing.T) {
150136
},
151137
},
152138
})
153-
numLogs:=len(auditor.AuditLogs)
154139

155140
_=coderdtest.CreateFirstUser(t,client)
156-
numLogs++// add an audit log for user create
157141

158142
resp:=oauth2Callback(t,client)
159-
numLogs++// add an audit log for login
160143

161144
require.Equal(t,http.StatusBadRequest,resp.StatusCode)
162-
require.Len(t,auditor.AuditLogs,numLogs)
163-
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
164145
})
165146
t.Run("BlockSignups",func(t*testing.T) {
166147
t.Parallel()
167-
auditor:=audit.NewMock()
168148
client:=coderdtest.New(t,&coderdtest.Options{
169-
Auditor:auditor,
170149
GithubOAuth2Config:&coderd.GithubOAuth2Config{
171150
OAuth2Config:&testutil.OAuth2Config{},
172151
AllowOrganizations: []string{"coder"},
@@ -190,20 +169,14 @@ func TestUserOAuth2Github(t *testing.T) {
190169
},
191170
},
192171
})
193-
numLogs:=len(auditor.AuditLogs)
194172

195173
resp:=oauth2Callback(t,client)
196-
numLogs++// add an audit log for login
197174

198175
require.Equal(t,http.StatusForbidden,resp.StatusCode)
199-
require.Len(t,auditor.AuditLogs,numLogs)
200-
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
201176
})
202177
t.Run("MultiLoginNotAllowed",func(t*testing.T) {
203178
t.Parallel()
204-
auditor:=audit.NewMock()
205179
client:=coderdtest.New(t,&coderdtest.Options{
206-
Auditor:auditor,
207180
GithubOAuth2Config:&coderd.GithubOAuth2Config{
208181
OAuth2Config:&testutil.OAuth2Config{},
209182
AllowOrganizations: []string{"coder"},
@@ -227,20 +200,15 @@ func TestUserOAuth2Github(t *testing.T) {
227200
},
228201
},
229202
})
230-
numLogs:=len(auditor.AuditLogs)
231203

232204
// Creates the first user with login_type 'password'.
233205
_=coderdtest.CreateFirstUser(t,client)
234-
numLogs++// add an audit log for user create
235206

236207
// Attempting to login should give us a 403 since the user
237208
// already has a login_type of 'password'.
238209
resp:=oauth2Callback(t,client)
239-
numLogs++// add an audit log for login
240210

241211
require.Equal(t,http.StatusForbidden,resp.StatusCode)
242-
require.Len(t,auditor.AuditLogs,numLogs)
243-
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
244212
})
245213
t.Run("Signup",func(t*testing.T) {
246214
t.Parallel()
@@ -290,6 +258,7 @@ func TestUserOAuth2Github(t *testing.T) {
290258
require.Equal(t,"/hello-world",user.AvatarURL)
291259

292260
require.Len(t,auditor.AuditLogs,numLogs)
261+
require.NotEqual(t,auditor.AuditLogs[numLogs-1].UserID,uuid.Nil)
293262
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
294263
})
295264
t.Run("SignupAllowedTeam",func(t*testing.T) {
@@ -480,9 +449,7 @@ func TestUserOAuth2Github(t *testing.T) {
480449
})
481450
t.Run("SignupFailedInactiveInOrg",func(t*testing.T) {
482451
t.Parallel()
483-
auditor:=audit.NewMock()
484452
client:=coderdtest.New(t,&coderdtest.Options{
485-
Auditor:auditor,
486453
GithubOAuth2Config:&coderd.GithubOAuth2Config{
487454
AllowSignups:true,
488455
AllowOrganizations: []string{"coder"},
@@ -513,14 +480,10 @@ func TestUserOAuth2Github(t *testing.T) {
513480
},
514481
},
515482
})
516-
numLogs:=len(auditor.AuditLogs)
517483

518484
resp:=oauth2Callback(t,client)
519-
numLogs++// add an audit log for login
520485

521486
require.Equal(t,http.StatusUnauthorized,resp.StatusCode)
522-
require.Len(t,auditor.AuditLogs,numLogs)
523-
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
524487
})
525488
}
526489

@@ -711,6 +674,7 @@ func TestUserOIDC(t *testing.T) {
711674
require.Equal(t,tc.Username,user.Username)
712675

713676
require.Len(t,auditor.AuditLogs,numLogs)
677+
require.NotEqual(t,auditor.AuditLogs[numLogs-1].UserID,uuid.Nil)
714678
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
715679
}
716680

@@ -784,33 +748,24 @@ func TestUserOIDC(t *testing.T) {
784748

785749
t.Run("NoIDToken",func(t*testing.T) {
786750
t.Parallel()
787-
auditor:=audit.NewMock()
788751
client:=coderdtest.New(t,&coderdtest.Options{
789-
Auditor:auditor,
790752
OIDCConfig:&coderd.OIDCConfig{
791753
OAuth2Config:&testutil.OAuth2Config{},
792754
},
793755
})
794-
numLogs:=len(auditor.AuditLogs)
795756

796757
resp:=oidcCallback(t,client,"asdf")
797-
numLogs++// add an audit log for login
798-
799758
require.Equal(t,http.StatusBadRequest,resp.StatusCode)
800-
require.Len(t,auditor.AuditLogs,numLogs)
801-
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
802759
})
803760

804761
t.Run("BadVerify",func(t*testing.T) {
805762
t.Parallel()
806-
auditor:=audit.NewMock()
807763
verifier:=oidc.NewVerifier("",&oidc.StaticKeySet{
808764
PublicKeys: []crypto.PublicKey{},
809765
},&oidc.Config{})
810766
provider:=&oidc.Provider{}
811767

812768
client:=coderdtest.New(t,&coderdtest.Options{
813-
Auditor:auditor,
814769
OIDCConfig:&coderd.OIDCConfig{
815770
OAuth2Config:&testutil.OAuth2Config{
816771
Token: (&oauth2.Token{
@@ -823,14 +778,10 @@ func TestUserOIDC(t *testing.T) {
823778
Verifier:verifier,
824779
},
825780
})
826-
numLogs:=len(auditor.AuditLogs)
827781

828782
resp:=oidcCallback(t,client,"asdf")
829-
numLogs++// add an audit log for login
830783

831784
require.Equal(t,http.StatusBadRequest,resp.StatusCode)
832-
require.Len(t,auditor.AuditLogs,numLogs)
833-
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
834785
})
835786
}
836787

‎coderd/users_test.go

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -92,24 +92,17 @@ func TestPostLogin(t *testing.T) {
9292
t.Parallel()
9393
t.Run("InvalidUser",func(t*testing.T) {
9494
t.Parallel()
95-
auditor:=audit.NewMock()
96-
client:=coderdtest.New(t,&coderdtest.Options{Auditor:auditor})
97-
numLogs:=len(auditor.AuditLogs)
98-
95+
client:=coderdtest.New(t,nil)
9996
ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitLong)
10097
defercancel()
10198

10299
_,err:=client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
103100
Email:"my@email.org",
104101
Password:"password",
105102
})
106-
numLogs++// add an audit log for login
107103
varapiErr*codersdk.Error
108104
require.ErrorAs(t,err,&apiErr)
109105
require.Equal(t,http.StatusUnauthorized,apiErr.StatusCode())
110-
111-
require.Len(t,auditor.AuditLogs,numLogs)
112-
require.Equal(t,database.AuditActionLogin,auditor.AuditLogs[numLogs-1].Action)
113106
})
114107

115108
t.Run("BadPassword",func(t*testing.T) {

‎site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.test.tsx

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import {
44
MockWorkspaceCreateAuditLogForDifferentOwner,
55
MockAuditLogSuccessfulLogin,
66
MockAuditLogUnsuccessfulLoginKnownUser,
7-
MockAuditLogUnsuccessfulLoginUnknownUser,
87
}from"testHelpers/entities"
98
import{AuditLogDescription}from"./AuditLogDescription"
109
import{AuditLogRow}from"../AuditLogRow"
@@ -101,25 +100,6 @@ describe("AuditLogDescription", () => {
101100
),
102101
).toBeInTheDocument()
103102

104-
conststatusPill=screen.getByRole("status")
105-
expect(statusPill).toHaveTextContent("401")
106-
})
107-
it("renders the correct string for unsuccessful login for an unknown user",async()=>{
108-
render(<AuditLogRowauditLog={MockAuditLogUnsuccessfulLoginUnknownUser}/>)
109-
110-
expect(
111-
screen.getByText(
112-
t("auditLog:table.logRow.description.unlinkedAuditDescription",{
113-
truncatedDescription:"an unknown user logged in",
114-
target:"",
115-
onBehalfOf:undefined,
116-
})
117-
.replace(/<[^>]*>/g," ")
118-
.replace(/\s{2,}/g," ")
119-
.trim(),
120-
),
121-
).toBeInTheDocument()
122-
123103
conststatusPill=screen.getByRole("status")
124104
expect(statusPill).toHaveTextContent("401")
125105
})

‎site/src/components/AuditLogRow/AuditLogDescription/AuditLogDescription.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export const AuditLogDescription: FC<{ auditLog: AuditLog }> = ({
1111
const{ t}=useTranslation("auditLog")
1212

1313
lettarget=auditLog.resource_target.trim()
14-
constuser=auditLog.user ?auditLog.user.username.trim() :"an unknown user"
14+
constuser=auditLog.user?.username.trim()
1515

1616
if(auditLog.resource_type==="workspace_build"){
1717
return<BuildAuditDescriptionauditLog={auditLog}/>
@@ -30,7 +30,7 @@ export const AuditLogDescription: FC<{ auditLog: AuditLog }> = ({
3030
constonBehalfOf=
3131
auditLog.additional_fields.workspace_owner&&
3232
auditLog.additional_fields.workspace_owner!=="unknown"&&
33-
auditLog.additional_fields.workspace_owner!==auditLog.user?.username
33+
auditLog.additional_fields.workspace_owner.trim()!==user
3434
?`on behalf of${auditLog.additional_fields.workspace_owner}`
3535
:""
3636

‎site/src/testHelpers/entities.ts

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1384,12 +1384,6 @@ export const MockAuditLogUnsuccessfulLoginKnownUser: TypesGen.AuditLog = {
13841384
status_code:401,
13851385
}
13861386

1387-
exportconstMockAuditLogUnsuccessfulLoginUnknownUser:TypesGen.AuditLog={
1388-
...MockAuditLogSuccessfulLogin,
1389-
status_code:401,
1390-
user:undefined,
1391-
}
1392-
13931387
exportconstMockWorkspaceQuota:TypesGen.WorkspaceQuota={
13941388
credits_consumed:0,
13951389
budget:100,

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp