diff --git a/internal/federationdomain/endpoints/auth/auth_handler.go b/internal/federationdomain/endpoints/auth/auth_handler.go index a06ad4a91..03b6b481f 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler.go +++ b/internal/federationdomain/endpoints/auth/auth_handler.go @@ -213,7 +213,23 @@ func (h *authorizeHandler) authorize( } } if err != nil { - // TODO: Consider an audit event here + // No specific audit event is emitted here in the case of an authorization error. + // There are currently seven possible cases: + // (1) OIDC with cli_password: + // - Rely on the "HTTP Request Completed" audit event with an error and error_description to indicate what went wrong. + // - There's no way to determine why the OIDC provider rejected the request. + // (2) OIDC with browser_authcode: this endpoint only redirects upstream + // (3) LDAP with cli_password: + // - Rely on the "HTTP Request Completed" audit event with an error and error_description to indicate what went wrong. + // - If we know that the LDAP provider rejected the request due to incorrect username or password, + // Pinniped will provide the "Incorrect Username Or Password" audit event. + // (4) LDAP with browser_authcode: this endpoint only redirects to the /login page + // (5) Active Directory with cli_password: + // - Rely on the "HTTP Request Completed" audit event with an error and error_description to indicate what went wrong. + // - If we know that the Active Directory provider rejected the request due to incorrect username or password, + // Pinniped will provide the "Incorrect Username Or Password" audit event. + // (6) Active Directory with browser_authcode: this endpoint only redirects to the /login page + // (7) GitHub with browser_authcode (cli_password is not supported): this endpoint only redirects upstream oidc.WriteAuthorizeError(r, w, oauthHelper, authorizeRequester, err, requestedBrowserlessFlow) } } diff --git a/internal/testutil/log_lines.go b/internal/testutil/log_lines.go index da776f201..73b28cca4 100644 --- a/internal/testutil/log_lines.go +++ b/internal/testutil/log_lines.go @@ -45,14 +45,14 @@ func WantAuditIDOnEveryAuditLog(wantedAuditLogs []WantedAuditLog, wantAuditID st } func GetStateParam(t *testing.T, fullURL string) stateparam.Encoded { - var encodedStateParam stateparam.Encoded - if fullURL != "" { - path, err := url.Parse(fullURL) - require.NoError(t, err) - encodedStateParam = stateparam.Encoded(path.Query().Get("state")) + if fullURL == "" { + var empty stateparam.Encoded + return empty } - return encodedStateParam + path, err := url.Parse(fullURL) + require.NoError(t, err) + return stateparam.Encoded(path.Query().Get("state")) } func CompareAuditLogs(t *testing.T, wantAuditLogs []WantedAuditLog, actualAuditLogsOneLiner string) {