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

Append OIDC logins (success/failure) to security events.#44467

Merged
vrto merged 8 commits into
mainfrom
mv/security-events/oidc-logins
Nov 16, 2022
Merged

Append OIDC logins (success/failure) to security events.#44467
vrto merged 8 commits into
mainfrom
mv/security-events/oidc-logins

Conversation

@vrto

@vrto vrto commented Nov 16, 2022

Copy link
Copy Markdown
Contributor

Description

Expand security events with a new SecurityEventOIDCLoginSucceeded and SecurityEventOIDCLoginFailed pair and hook it up to the correct place.

Test plan

  • expanded the existing automated tests

@cla-bot cla-bot Bot added the cla-signed label Nov 16, 2022
@vrto vrto requested a review from willdollman November 16, 2022 08:10
@sourcegraph-bot

sourcegraph-bot commented Nov 16, 2022

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 6334795...fbdb723.

Notify File(s)
@unknwon enterprise/cmd/frontend/internal/auth/openidconnect/middleware.go
enterprise/cmd/frontend/internal/auth/openidconnect/middleware_test.go

Comment thread enterprise/cmd/frontend/internal/auth/openidconnect/middleware.go Outdated
db.SecurityEventLogs().LogEvent(r.Context(), &database.SecurityEvent{
Name: database.SecurityEventOIDCLoginFailed,
URL: r.URL.Path, // don't log OIDC query params
AnonymousUserID: fmt.Sprintf("unknown OIDC @ %s", time.Now()), // we don't know have a reliable user identify at the time of the failure

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
AnonymousUserID: fmt.Sprintf("unknown OIDC @ %s", time.Now()), // we don't know have a reliable user identify at the time of the failure
AnonymousUserID: fmt.Sprintf("unknown OIDC @ %s", time.Now()), // We don't have a reliable user identity at the time of the failure

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is time.Now() necessary as we also have a Timestamp field?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think no, but I wanted to make that unique (more or less); we'd normally use a cookie value for the AnonymousUserID, but we haven't got anything in this case.

Comment thread enterprise/cmd/frontend/internal/auth/openidconnect/middleware.go Outdated
Comment thread enterprise/cmd/frontend/internal/auth/openidconnect/middleware.go
@vrto vrto merged commit 776eb00 into main Nov 16, 2022
@vrto vrto deleted the mv/security-events/oidc-logins branch November 16, 2022 14:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants