Update Authorization Webhook Response Format and Handling #1037

chacha912 · 2024-10-17T01:28:41Z

What this PR does / why we need it:

Add HTTP status codes to the authorization webhook response while maintaining the existing response body format of {allowed: bool, reason: string}.

Added response status codes in HTTP header:

200: OK (Allowed)
401: Unauthorized (Invalid or missing token)
403: Forbidden (Permission denied)

This change allows for a more granular distinction between authentication and authorization issues:

Forbidden (403): Now strictly used for cases where the user lacks the necessary permissions for the requested action.
Unauthorized (401): Used when the token is invalid or expired, allowing the client to handle token refresh scenarios.

The token refresh interface needs to be updated to accommodate this new distinction.(Provide a way to refresh token for Auth Webhook #996)

Which issue(s) this PR fixes:

Fixes #205

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Additional documentation:

Checklist:

Added relevant tests or not required
Didn't break anything

Summary by CodeRabbit

Release Notes

New Features
- Introduced structured error metadata handling for authentication webhooks.
- Enhanced response handling with new status codes for authentication results.
- Added functionality for attaching metadata to errors, improving error reporting.
- Added a method for updating client authentication tokens and storing RPC server address.
Bug Fixes
- Improved clarity and specificity in error reporting for permission and authentication errors.
Tests
- Updated test cases to reflect new error handling and response structures.
- Added new tests for various authentication scenarios and improved error assertions.
Documentation
- Enhanced comments and documentation for clarity regarding new error handling mechanisms.

github-actions

Go Benchmark

Benchmark suite	Current: `e68cb71`	Previous: `e79cd11`	Ratio
`BenchmarkDocument/constructor_test`	`1498` ns/op 1337 B/op 24 allocs/op	`1477` ns/op 1337 B/op 24 allocs/op	`1.01`
`BenchmarkDocument/constructor_test - ns/op`	`1498` ns/op	`1477` ns/op	`1.01`
`BenchmarkDocument/constructor_test - B/op`	`1337` B/op	`1337` B/op	`1`
`BenchmarkDocument/constructor_test - allocs/op`	`24` allocs/op	`24` allocs/op	`1`
`BenchmarkDocument/status_test`	`953.2` ns/op 1305 B/op 22 allocs/op	`983.7` ns/op 1305 B/op 22 allocs/op	`0.97`
`BenchmarkDocument/status_test - ns/op`	`953.2` ns/op	`983.7` ns/op	`0.97`
`BenchmarkDocument/status_test - B/op`	`1305` B/op	`1305` B/op	`1`
`BenchmarkDocument/status_test - allocs/op`	`22` allocs/op	`22` allocs/op	`1`
`BenchmarkDocument/equals_test`	`7799` ns/op 7529 B/op 134 allocs/op	`7568` ns/op 7273 B/op 132 allocs/op	`1.03`
`BenchmarkDocument/equals_test - ns/op`	`7799` ns/op	`7568` ns/op	`1.03`
`BenchmarkDocument/equals_test - B/op`	`7529` B/op	`7273` B/op	`1.04`
`BenchmarkDocument/equals_test - allocs/op`	`134` allocs/op	`132` allocs/op	`1.02`
`BenchmarkDocument/nested_update_test`	`19162` ns/op 12395 B/op 264 allocs/op	`16809` ns/op 12138 B/op 262 allocs/op	`1.14`
`BenchmarkDocument/nested_update_test - ns/op`	`19162` ns/op	`16809` ns/op	`1.14`
`BenchmarkDocument/nested_update_test - B/op`	`12395` B/op	`12138` B/op	`1.02`
`BenchmarkDocument/nested_update_test - allocs/op`	`264` allocs/op	`262` allocs/op	`1.01`
`BenchmarkDocument/delete_test`	`23218` ns/op 15923 B/op 347 allocs/op	`22564` ns/op 15363 B/op 341 allocs/op	`1.03`
`BenchmarkDocument/delete_test - ns/op`	`23218` ns/op	`22564` ns/op	`1.03`
`BenchmarkDocument/delete_test - B/op`	`15923` B/op	`15363` B/op	`1.04`
`BenchmarkDocument/delete_test - allocs/op`	`347` allocs/op	`341` allocs/op	`1.02`
`BenchmarkDocument/object_test`	`8741` ns/op 7073 B/op 122 allocs/op	`10060` ns/op 6817 B/op 120 allocs/op	`0.87`
`BenchmarkDocument/object_test - ns/op`	`8741` ns/op	`10060` ns/op	`0.87`
`BenchmarkDocument/object_test - B/op`	`7073` B/op	`6817` B/op	`1.04`
`BenchmarkDocument/object_test - allocs/op`	`122` allocs/op	`120` allocs/op	`1.02`
`BenchmarkDocument/array_test`	`30184` ns/op 12203 B/op 278 allocs/op	`29873` ns/op 11947 B/op 276 allocs/op	`1.01`
`BenchmarkDocument/array_test - ns/op`	`30184` ns/op	`29873` ns/op	`1.01`
`BenchmarkDocument/array_test - B/op`	`12203` B/op	`11947` B/op	`1.02`
`BenchmarkDocument/array_test - allocs/op`	`278` allocs/op	`276` allocs/op	`1.01`
`BenchmarkDocument/text_test`	`32486` ns/op 15326 B/op 492 allocs/op	`31304` ns/op 14779 B/op 486 allocs/op	`1.04`
`BenchmarkDocument/text_test - ns/op`	`32486` ns/op	`31304` ns/op	`1.04`
`BenchmarkDocument/text_test - B/op`	`15326` B/op	`14779` B/op	`1.04`
`BenchmarkDocument/text_test - allocs/op`	`492` allocs/op	`486` allocs/op	`1.01`
`BenchmarkDocument/text_composition_test`	`30406` ns/op 18716 B/op 504 allocs/op	`30336` ns/op 18463 B/op 502 allocs/op	`1.00`
`BenchmarkDocument/text_composition_test - ns/op`	`30406` ns/op	`30336` ns/op	`1.00`
`BenchmarkDocument/text_composition_test - B/op`	`18716` B/op	`18463` B/op	`1.01`
`BenchmarkDocument/text_composition_test - allocs/op`	`504` allocs/op	`502` allocs/op	`1.00`
`BenchmarkDocument/rich_text_test`	`84562` ns/op 40180 B/op 1183 allocs/op	`85762` ns/op 38709 B/op 1165 allocs/op	`0.99`
`BenchmarkDocument/rich_text_test - ns/op`	`84562` ns/op	`85762` ns/op	`0.99`
`BenchmarkDocument/rich_text_test - B/op`	`40180` B/op	`38709` B/op	`1.04`
`BenchmarkDocument/rich_text_test - allocs/op`	`1183` allocs/op	`1165` allocs/op	`1.02`
`BenchmarkDocument/counter_test`	`18442` ns/op 11874 B/op 258 allocs/op	`17928` ns/op 10722 B/op 244 allocs/op	`1.03`
`BenchmarkDocument/counter_test - ns/op`	`18442` ns/op	`17928` ns/op	`1.03`
`BenchmarkDocument/counter_test - B/op`	`11874` B/op	`10722` B/op	`1.11`
`BenchmarkDocument/counter_test - allocs/op`	`258` allocs/op	`244` allocs/op	`1.06`
`BenchmarkDocument/text_edit_gc_100`	`1313215` ns/op 872560 B/op 17281 allocs/op	`1312018` ns/op 871973 B/op 17276 allocs/op	`1.00`
`BenchmarkDocument/text_edit_gc_100 - ns/op`	`1313215` ns/op	`1312018` ns/op	`1.00`
`BenchmarkDocument/text_edit_gc_100 - B/op`	`872560` B/op	`871973` B/op	`1.00`
`BenchmarkDocument/text_edit_gc_100 - allocs/op`	`17281` allocs/op	`17276` allocs/op	`1.00`
`BenchmarkDocument/text_edit_gc_1000`	`49797088` ns/op 50546855 B/op 186742 allocs/op	`50235130` ns/op 50546306 B/op 186739 allocs/op	`0.99`
`BenchmarkDocument/text_edit_gc_1000 - ns/op`	`49797088` ns/op	`50235130` ns/op	`0.99`
`BenchmarkDocument/text_edit_gc_1000 - B/op`	`50546855` B/op	`50546306` B/op	`1.00`
`BenchmarkDocument/text_edit_gc_1000 - allocs/op`	`186742` allocs/op	`186739` allocs/op	`1.00`
`BenchmarkDocument/text_split_gc_100`	`1954922` ns/op 1589077 B/op 15951 allocs/op	`1961532` ns/op 1588486 B/op 15945 allocs/op	`1.00`
`BenchmarkDocument/text_split_gc_100 - ns/op`	`1954922` ns/op	`1961532` ns/op	`1.00`
`BenchmarkDocument/text_split_gc_100 - B/op`	`1589077` B/op	`1588486` B/op	`1.00`
`BenchmarkDocument/text_split_gc_100 - allocs/op`	`15951` allocs/op	`15945` allocs/op	`1.00`
`BenchmarkDocument/text_split_gc_1000`	`116344438` ns/op 141482127 B/op 186153 allocs/op	`117538603` ns/op 141482699 B/op 186140 allocs/op	`0.99`
`BenchmarkDocument/text_split_gc_1000 - ns/op`	`116344438` ns/op	`117538603` ns/op	`0.99`
`BenchmarkDocument/text_split_gc_1000 - B/op`	`141482127` B/op	`141482699` B/op	`1.00`
`BenchmarkDocument/text_split_gc_1000 - allocs/op`	`186153` allocs/op	`186140` allocs/op	`1.00`
`BenchmarkDocument/text_delete_all_10000`	`16295468` ns/op 10214883 B/op 55690 allocs/op	`18230599` ns/op 10215651 B/op 55687 allocs/op	`0.89`
`BenchmarkDocument/text_delete_all_10000 - ns/op`	`16295468` ns/op	`18230599` ns/op	`0.89`
`BenchmarkDocument/text_delete_all_10000 - B/op`	`10214883` B/op	`10215651` B/op	`1.00`
`BenchmarkDocument/text_delete_all_10000 - allocs/op`	`55690` allocs/op	`55687` allocs/op	`1.00`
`BenchmarkDocument/text_delete_all_100000`	`287944685` ns/op 142987740 B/op 561707 allocs/op	`298869720` ns/op 142985272 B/op 561733 allocs/op	`0.96`
`BenchmarkDocument/text_delete_all_100000 - ns/op`	`287944685` ns/op	`298869720` ns/op	`0.96`
`BenchmarkDocument/text_delete_all_100000 - B/op`	`142987740` B/op	`142985272` B/op	`1.00`
`BenchmarkDocument/text_delete_all_100000 - allocs/op`	`561707` allocs/op	`561733` allocs/op	`1.00`
`BenchmarkDocument/text_100`	`219042` ns/op 120490 B/op 5182 allocs/op	`231831` ns/op 120235 B/op 5180 allocs/op	`0.94`
`BenchmarkDocument/text_100 - ns/op`	`219042` ns/op	`231831` ns/op	`0.94`
`BenchmarkDocument/text_100 - B/op`	`120490` B/op	`120235` B/op	`1.00`
`BenchmarkDocument/text_100 - allocs/op`	`5182` allocs/op	`5180` allocs/op	`1.00`
`BenchmarkDocument/text_1000`	`2371133` ns/op 1171279 B/op 51086 allocs/op	`2552019` ns/op 1171026 B/op 51084 allocs/op	`0.93`
`BenchmarkDocument/text_1000 - ns/op`	`2371133` ns/op	`2552019` ns/op	`0.93`
`BenchmarkDocument/text_1000 - B/op`	`1171279` B/op	`1171026` B/op	`1.00`
`BenchmarkDocument/text_1000 - allocs/op`	`51086` allocs/op	`51084` allocs/op	`1.00`
`BenchmarkDocument/array_1000`	`1207856` ns/op 1091655 B/op 11834 allocs/op	`1291774` ns/op 1091389 B/op 11831 allocs/op	`0.94`
`BenchmarkDocument/array_1000 - ns/op`	`1207856` ns/op	`1291774` ns/op	`0.94`
`BenchmarkDocument/array_1000 - B/op`	`1091655` B/op	`1091389` B/op	`1.00`
`BenchmarkDocument/array_1000 - allocs/op`	`11834` allocs/op	`11831` allocs/op	`1.00`
`BenchmarkDocument/array_10000`	`13443312` ns/op 9799411 B/op 120296 allocs/op	`13470218` ns/op 9798892 B/op 120292 allocs/op	`1.00`
`BenchmarkDocument/array_10000 - ns/op`	`13443312` ns/op	`13470218` ns/op	`1.00`
`BenchmarkDocument/array_10000 - B/op`	`9799411` B/op	`9798892` B/op	`1.00`
`BenchmarkDocument/array_10000 - allocs/op`	`120296` allocs/op	`120292` allocs/op	`1.00`
`BenchmarkDocument/array_gc_100`	`147579` ns/op 133281 B/op 1266 allocs/op	`154681` ns/op 132716 B/op 1260 allocs/op	`0.95`
`BenchmarkDocument/array_gc_100 - ns/op`	`147579` ns/op	`154681` ns/op	`0.95`
`BenchmarkDocument/array_gc_100 - B/op`	`133281` B/op	`132716` B/op	`1.00`
`BenchmarkDocument/array_gc_100 - allocs/op`	`1266` allocs/op	`1260` allocs/op	`1.00`
`BenchmarkDocument/array_gc_1000`	`1380678` ns/op 1159712 B/op 12883 allocs/op	`1475811` ns/op 1159072 B/op 12876 allocs/op	`0.94`
`BenchmarkDocument/array_gc_1000 - ns/op`	`1380678` ns/op	`1475811` ns/op	`0.94`
`BenchmarkDocument/array_gc_1000 - B/op`	`1159712` B/op	`1159072` B/op	`1.00`
`BenchmarkDocument/array_gc_1000 - allocs/op`	`12883` allocs/op	`12876` allocs/op	`1.00`
`BenchmarkDocument/counter_1000`	`200335` ns/op 193335 B/op 5773 allocs/op	`218246` ns/op 193080 B/op 5771 allocs/op	`0.92`
`BenchmarkDocument/counter_1000 - ns/op`	`200335` ns/op	`218246` ns/op	`0.92`
`BenchmarkDocument/counter_1000 - B/op`	`193335` B/op	`193080` B/op	`1.00`
`BenchmarkDocument/counter_1000 - allocs/op`	`5773` allocs/op	`5771` allocs/op	`1.00`
`BenchmarkDocument/counter_10000`	`2171760` ns/op 2088265 B/op 59780 allocs/op	`2321812` ns/op 2087998 B/op 59778 allocs/op	`0.94`
`BenchmarkDocument/counter_10000 - ns/op`	`2171760` ns/op	`2321812` ns/op	`0.94`
`BenchmarkDocument/counter_10000 - B/op`	`2088265` B/op	`2087998` B/op	`1.00`
`BenchmarkDocument/counter_10000 - allocs/op`	`59780` allocs/op	`59778` allocs/op	`1.00`
`BenchmarkDocument/object_1000`	`1369045` ns/op 1428398 B/op 9851 allocs/op	`1487361` ns/op 1428131 B/op 9849 allocs/op	`0.92`
`BenchmarkDocument/object_1000 - ns/op`	`1369045` ns/op	`1487361` ns/op	`0.92`
`BenchmarkDocument/object_1000 - B/op`	`1428398` B/op	`1428131` B/op	`1.00`
`BenchmarkDocument/object_1000 - allocs/op`	`9851` allocs/op	`9849` allocs/op	`1.00`
`BenchmarkDocument/object_10000`	`15147044` ns/op 12166501 B/op 100565 allocs/op	`16488644` ns/op 12166587 B/op 100562 allocs/op	`0.92`
`BenchmarkDocument/object_10000 - ns/op`	`15147044` ns/op	`16488644` ns/op	`0.92`
`BenchmarkDocument/object_10000 - B/op`	`12166501` B/op	`12166587` B/op	`1.00`
`BenchmarkDocument/object_10000 - allocs/op`	`100565` allocs/op	`100562` allocs/op	`1.00`
`BenchmarkDocument/tree_100`	`1016017` ns/op 943956 B/op 6103 allocs/op	`1086704` ns/op 943703 B/op 6101 allocs/op	`0.93`
`BenchmarkDocument/tree_100 - ns/op`	`1016017` ns/op	`1086704` ns/op	`0.93`
`BenchmarkDocument/tree_100 - B/op`	`943956` B/op	`943703` B/op	`1.00`
`BenchmarkDocument/tree_100 - allocs/op`	`6103` allocs/op	`6101` allocs/op	`1.00`
`BenchmarkDocument/tree_1000`	`73373557` ns/op 86460502 B/op 60116 allocs/op	`81480724` ns/op 86460429 B/op 60114 allocs/op	`0.90`
`BenchmarkDocument/tree_1000 - ns/op`	`73373557` ns/op	`81480724` ns/op	`0.90`
`BenchmarkDocument/tree_1000 - B/op`	`86460502` B/op	`86460429` B/op	`1.00`
`BenchmarkDocument/tree_1000 - allocs/op`	`60116` allocs/op	`60114` allocs/op	`1.00`
`BenchmarkDocument/tree_10000`	`9537394321` ns/op 8580654992 B/op 600240 allocs/op	`9874931789` ns/op 8580661712 B/op 600233 allocs/op	`0.97`
`BenchmarkDocument/tree_10000 - ns/op`	`9537394321` ns/op	`9874931789` ns/op	`0.97`
`BenchmarkDocument/tree_10000 - B/op`	`8580654992` B/op	`8580661712` B/op	`1.00`
`BenchmarkDocument/tree_10000 - allocs/op`	`600240` allocs/op	`600233` allocs/op	`1.00`
`BenchmarkDocument/tree_delete_all_1000`	`75133805` ns/op 87510773 B/op 75272 allocs/op	`81211966` ns/op 87509285 B/op 75263 allocs/op	`0.93`
`BenchmarkDocument/tree_delete_all_1000 - ns/op`	`75133805` ns/op	`81211966` ns/op	`0.93`
`BenchmarkDocument/tree_delete_all_1000 - B/op`	`87510773` B/op	`87509285` B/op	`1.00`
`BenchmarkDocument/tree_delete_all_1000 - allocs/op`	`75272` allocs/op	`75263` allocs/op	`1.00`
`BenchmarkDocument/tree_edit_gc_100`	`3730002` ns/op 4147272 B/op 15146 allocs/op	`4031767` ns/op 4147774 B/op 15141 allocs/op	`0.93`
`BenchmarkDocument/tree_edit_gc_100 - ns/op`	`3730002` ns/op	`4031767` ns/op	`0.93`
`BenchmarkDocument/tree_edit_gc_100 - B/op`	`4147272` B/op	`4147774` B/op	`1.00`
`BenchmarkDocument/tree_edit_gc_100 - allocs/op`	`15146` allocs/op	`15141` allocs/op	`1.00`
`BenchmarkDocument/tree_edit_gc_1000`	`300022321` ns/op 383746524 B/op 154855 allocs/op	`326622951` ns/op 383745592 B/op 154864 allocs/op	`0.92`
`BenchmarkDocument/tree_edit_gc_1000 - ns/op`	`300022321` ns/op	`326622951` ns/op	`0.92`
`BenchmarkDocument/tree_edit_gc_1000 - B/op`	`383746524` B/op	`383745592` B/op	`1.00`
`BenchmarkDocument/tree_edit_gc_1000 - allocs/op`	`154855` allocs/op	`154864` allocs/op	`1.00`
`BenchmarkDocument/tree_split_gc_100`	`2490330` ns/op 2413047 B/op 11131 allocs/op	`2701394` ns/op 2412475 B/op 11125 allocs/op	`0.92`
`BenchmarkDocument/tree_split_gc_100 - ns/op`	`2490330` ns/op	`2701394` ns/op	`0.92`
`BenchmarkDocument/tree_split_gc_100 - B/op`	`2413047` B/op	`2412475` B/op	`1.00`
`BenchmarkDocument/tree_split_gc_100 - allocs/op`	`11131` allocs/op	`11125` allocs/op	`1.00`
`BenchmarkDocument/tree_split_gc_1000`	`179082247` ns/op 222253781 B/op 122004 allocs/op	`200004660` ns/op 222253289 B/op 122010 allocs/op	`0.90`
`BenchmarkDocument/tree_split_gc_1000 - ns/op`	`179082247` ns/op	`200004660` ns/op	`0.90`
`BenchmarkDocument/tree_split_gc_1000 - B/op`	`222253781` B/op	`222253289` B/op	`1.00`
`BenchmarkDocument/tree_split_gc_1000 - allocs/op`	`122004` allocs/op	`122010` allocs/op	`1.00`
`BenchmarkRPC/client_to_server`	`418830134` ns/op 19727816 B/op 224688 allocs/op	`346600874` ns/op 16591733 B/op 169214 allocs/op	`1.21`
`BenchmarkRPC/client_to_server - ns/op`	`418830134` ns/op	`346600874` ns/op	`1.21`
`BenchmarkRPC/client_to_server - B/op`	`19727816` B/op	`16591733` B/op	`1.19`
`BenchmarkRPC/client_to_server - allocs/op`	`224688` allocs/op	`169214` allocs/op	`1.33`
`BenchmarkRPC/client_to_client_via_server`	`767743140` ns/op 46737280 B/op 476328 allocs/op	`626104207` ns/op 34385416 B/op 323844 allocs/op	`1.23`
`BenchmarkRPC/client_to_client_via_server - ns/op`	`767743140` ns/op	`626104207` ns/op	`1.23`
`BenchmarkRPC/client_to_client_via_server - B/op`	`46737280` B/op	`34385416` B/op	`1.36`
`BenchmarkRPC/client_to_client_via_server - allocs/op`	`476328` allocs/op	`323844` allocs/op	`1.47`
`BenchmarkRPC/attach_large_document`	`1760833929` ns/op 3033425240 B/op 13754 allocs/op	`1394507433` ns/op 1907794320 B/op 8792 allocs/op	`1.26`
`BenchmarkRPC/attach_large_document - ns/op`	`1760833929` ns/op	`1394507433` ns/op	`1.26`
`BenchmarkRPC/attach_large_document - B/op`	`3033425240` B/op	`1907794320` B/op	`1.59`
`BenchmarkRPC/attach_large_document - allocs/op`	`13754` allocs/op	`8792` allocs/op	`1.56`
`BenchmarkRPC/adminCli_to_server`	`533871047` ns/op 35993412 B/op 289703 allocs/op	`536211172` ns/op 35960656 B/op 289553 allocs/op	`1.00`
`BenchmarkRPC/adminCli_to_server - ns/op`	`533871047` ns/op	`536211172` ns/op	`1.00`
`BenchmarkRPC/adminCli_to_server - B/op`	`35993412` B/op	`35960656` B/op	`1.00`
`BenchmarkRPC/adminCli_to_server - allocs/op`	`289703` allocs/op	`289553` allocs/op	`1.00`
`BenchmarkLocker`	`68.98` ns/op 16 B/op 1 allocs/op	`64.39` ns/op 16 B/op 1 allocs/op	`1.07`
`BenchmarkLocker - ns/op`	`68.98` ns/op	`64.39` ns/op	`1.07`
`BenchmarkLocker - B/op`	`16` B/op	`16` B/op	`1`
`BenchmarkLocker - allocs/op`	`1` allocs/op	`1` allocs/op	`1`
`BenchmarkLockerParallel`	`40.35` ns/op 0 B/op 0 allocs/op	`39.17` ns/op 0 B/op 0 allocs/op	`1.03`
`BenchmarkLockerParallel - ns/op`	`40.35` ns/op	`39.17` ns/op	`1.03`
`BenchmarkLockerParallel - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkLockerParallel - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkLockerMoreKeys`	`158.8` ns/op 15 B/op 0 allocs/op	`154.4` ns/op 15 B/op 0 allocs/op	`1.03`
`BenchmarkLockerMoreKeys - ns/op`	`158.8` ns/op	`154.4` ns/op	`1.03`
`BenchmarkLockerMoreKeys - B/op`	`15` B/op	`15` B/op	`1`
`BenchmarkLockerMoreKeys - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkChange/Push_10_Changes`	`4553404` ns/op 144091 B/op 1570 allocs/op	`3806029` ns/op 117167 B/op 1215 allocs/op	`1.20`
`BenchmarkChange/Push_10_Changes - ns/op`	`4553404` ns/op	`3806029` ns/op	`1.20`
`BenchmarkChange/Push_10_Changes - B/op`	`144091` B/op	`117167` B/op	`1.23`
`BenchmarkChange/Push_10_Changes - allocs/op`	`1570` allocs/op	`1215` allocs/op	`1.29`
`BenchmarkChange/Push_100_Changes`	`16225010` ns/op 724254 B/op 8191 allocs/op	`15394081` ns/op 570185 B/op 6585 allocs/op	`1.05`
`BenchmarkChange/Push_100_Changes - ns/op`	`16225010` ns/op	`15394081` ns/op	`1.05`
`BenchmarkChange/Push_100_Changes - B/op`	`724254` B/op	`570185` B/op	`1.27`
`BenchmarkChange/Push_100_Changes - allocs/op`	`8191` allocs/op	`6585` allocs/op	`1.24`
`BenchmarkChange/Push_1000_Changes`	`128299218` ns/op 7142028 B/op 77175 allocs/op	`126296826` ns/op 5284779 B/op 63077 allocs/op	`1.02`
`BenchmarkChange/Push_1000_Changes - ns/op`	`128299218` ns/op	`126296826` ns/op	`1.02`
`BenchmarkChange/Push_1000_Changes - B/op`	`7142028` B/op	`5284779` B/op	`1.35`
`BenchmarkChange/Push_1000_Changes - allocs/op`	`77175` allocs/op	`63077` allocs/op	`1.22`
`BenchmarkChange/Pull_10_Changes`	`3659072` ns/op 123812 B/op 1403 allocs/op	`2993126` ns/op 103049 B/op 1019 allocs/op	`1.22`
`BenchmarkChange/Pull_10_Changes - ns/op`	`3659072` ns/op	`2993126` ns/op	`1.22`
`BenchmarkChange/Pull_10_Changes - B/op`	`123812` B/op	`103049` B/op	`1.20`
`BenchmarkChange/Pull_10_Changes - allocs/op`	`1403` allocs/op	`1019` allocs/op	`1.38`
`BenchmarkChange/Pull_100_Changes`	`5190118` ns/op 351134 B/op 4948 allocs/op	`4547983` ns/op 267840 B/op 3489 allocs/op	`1.14`
`BenchmarkChange/Pull_100_Changes - ns/op`	`5190118` ns/op	`4547983` ns/op	`1.14`
`BenchmarkChange/Pull_100_Changes - B/op`	`351134` B/op	`267840` B/op	`1.31`
`BenchmarkChange/Pull_100_Changes - allocs/op`	`4948` allocs/op	`3489` allocs/op	`1.42`
`BenchmarkChange/Pull_1000_Changes`	`10611710` ns/op 2220144 B/op 42673 allocs/op	`8749542` ns/op 1493758 B/op 29869 allocs/op	`1.21`
`BenchmarkChange/Pull_1000_Changes - ns/op`	`10611710` ns/op	`8749542` ns/op	`1.21`
`BenchmarkChange/Pull_1000_Changes - B/op`	`2220144` B/op	`1493758` B/op	`1.49`
`BenchmarkChange/Pull_1000_Changes - allocs/op`	`42673` allocs/op	`29869` allocs/op	`1.43`
`BenchmarkSnapshot/Push_3KB_snapshot`	`19158299` ns/op 851165 B/op 8194 allocs/op	`18224553` ns/op 706976 B/op 6586 allocs/op	`1.05`
`BenchmarkSnapshot/Push_3KB_snapshot - ns/op`	`19158299` ns/op	`18224553` ns/op	`1.05`
`BenchmarkSnapshot/Push_3KB_snapshot - B/op`	`851165` B/op	`706976` B/op	`1.20`
`BenchmarkSnapshot/Push_3KB_snapshot - allocs/op`	`8194` allocs/op	`6586` allocs/op	`1.24`
`BenchmarkSnapshot/Push_30KB_snapshot`	`131762820` ns/op 7617682 B/op 80133 allocs/op	`129566296` ns/op 5446883 B/op 63078 allocs/op	`1.02`
`BenchmarkSnapshot/Push_30KB_snapshot - ns/op`	`131762820` ns/op	`129566296` ns/op	`1.02`
`BenchmarkSnapshot/Push_30KB_snapshot - B/op`	`7617682` B/op	`5446883` B/op	`1.40`
`BenchmarkSnapshot/Push_30KB_snapshot - allocs/op`	`80133` allocs/op	`63078` allocs/op	`1.27`
`BenchmarkSnapshot/Pull_3KB_snapshot`	`7410573` ns/op 1139932 B/op 19416 allocs/op	`6753592` ns/op 923370 B/op 15523 allocs/op	`1.10`
`BenchmarkSnapshot/Pull_3KB_snapshot - ns/op`	`7410573` ns/op	`6753592` ns/op	`1.10`
`BenchmarkSnapshot/Pull_3KB_snapshot - B/op`	`1139932` B/op	`923370` B/op	`1.23`
`BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op`	`19416` allocs/op	`15523` allocs/op	`1.25`
`BenchmarkSnapshot/Pull_30KB_snapshot`	`19165541` ns/op 9321935 B/op 187585 allocs/op	`15210485` ns/op 7153442 B/op 150118 allocs/op	`1.26`
`BenchmarkSnapshot/Pull_30KB_snapshot - ns/op`	`19165541` ns/op	`15210485` ns/op	`1.26`
`BenchmarkSnapshot/Pull_30KB_snapshot - B/op`	`9321935` B/op	`7153442` B/op	`1.30`
`BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op`	`187585` allocs/op	`150118` allocs/op	`1.25`
`BenchmarkSplayTree/stress_test_100000`	`0.1935` ns/op 0 B/op 0 allocs/op	`0.196` ns/op 0 B/op 0 allocs/op	`0.99`
`BenchmarkSplayTree/stress_test_100000 - ns/op`	`0.1935` ns/op	`0.196` ns/op	`0.99`
`BenchmarkSplayTree/stress_test_100000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/stress_test_100000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/stress_test_200000`	`0.3972` ns/op 0 B/op 0 allocs/op	`0.3743` ns/op 0 B/op 0 allocs/op	`1.06`
`BenchmarkSplayTree/stress_test_200000 - ns/op`	`0.3972` ns/op	`0.3743` ns/op	`1.06`
`BenchmarkSplayTree/stress_test_200000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/stress_test_200000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/stress_test_300000`	`0.5942` ns/op 0 B/op 0 allocs/op	`0.59` ns/op 0 B/op 0 allocs/op	`1.01`
`BenchmarkSplayTree/stress_test_300000 - ns/op`	`0.5942` ns/op	`0.59` ns/op	`1.01`
`BenchmarkSplayTree/stress_test_300000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/stress_test_300000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/random_access_100000`	`0.01291` ns/op 0 B/op 0 allocs/op	`0.01273` ns/op 0 B/op 0 allocs/op	`1.01`
`BenchmarkSplayTree/random_access_100000 - ns/op`	`0.01291` ns/op	`0.01273` ns/op	`1.01`
`BenchmarkSplayTree/random_access_100000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/random_access_100000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/random_access_200000`	`0.03153` ns/op 0 B/op 0 allocs/op	`0.02864` ns/op 0 B/op 0 allocs/op	`1.10`
`BenchmarkSplayTree/random_access_200000 - ns/op`	`0.03153` ns/op	`0.02864` ns/op	`1.10`
`BenchmarkSplayTree/random_access_200000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/random_access_200000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/random_access_300000`	`0.0433` ns/op 0 B/op 0 allocs/op	`0.03691` ns/op 0 B/op 0 allocs/op	`1.17`
`BenchmarkSplayTree/random_access_300000 - ns/op`	`0.0433` ns/op	`0.03691` ns/op	`1.17`
`BenchmarkSplayTree/random_access_300000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/random_access_300000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/editing_trace_bench`	`0.002162` ns/op 0 B/op 0 allocs/op	`0.002352` ns/op 0 B/op 0 allocs/op	`0.92`
`BenchmarkSplayTree/editing_trace_bench - ns/op`	`0.002162` ns/op	`0.002352` ns/op	`0.92`
`BenchmarkSplayTree/editing_trace_bench - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/editing_trace_bench - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSync/memory_sync_10_test`	`8444` ns/op 3765 B/op 69 allocs/op	`7034` ns/op 1286 B/op 38 allocs/op	`1.20`
`BenchmarkSync/memory_sync_10_test - ns/op`	`8444` ns/op	`7034` ns/op	`1.20`
`BenchmarkSync/memory_sync_10_test - B/op`	`3765` B/op	`1286` B/op	`2.93`
`BenchmarkSync/memory_sync_10_test - allocs/op`	`69` allocs/op	`38` allocs/op	`1.82`
`BenchmarkSync/memory_sync_100_test`	`54760` ns/op 11117 B/op 303 allocs/op	`52507` ns/op 8619 B/op 272 allocs/op	`1.04`
`BenchmarkSync/memory_sync_100_test - ns/op`	`54760` ns/op	`52507` ns/op	`1.04`
`BenchmarkSync/memory_sync_100_test - B/op`	`11117` B/op	`8619` B/op	`1.29`
`BenchmarkSync/memory_sync_100_test - allocs/op`	`303` allocs/op	`272` allocs/op	`1.11`
`BenchmarkSync/memory_sync_1000_test`	`584274` ns/op 77174 B/op 2161 allocs/op	`583316` ns/op 74448 B/op 2126 allocs/op	`1.00`
`BenchmarkSync/memory_sync_1000_test - ns/op`	`584274` ns/op	`583316` ns/op	`1.00`
`BenchmarkSync/memory_sync_1000_test - B/op`	`77174` B/op	`74448` B/op	`1.04`
`BenchmarkSync/memory_sync_1000_test - allocs/op`	`2161` allocs/op	`2126` allocs/op	`1.02`
`BenchmarkSync/memory_sync_10000_test`	`7549759` ns/op 763337 B/op 20566 allocs/op	`7557690` ns/op 735829 B/op 20229 allocs/op	`1.00`
`BenchmarkSync/memory_sync_10000_test - ns/op`	`7549759` ns/op	`7557690` ns/op	`1.00`
`BenchmarkSync/memory_sync_10000_test - B/op`	`763337` B/op	`735829` B/op	`1.04`
`BenchmarkSync/memory_sync_10000_test - allocs/op`	`20566` allocs/op	`20229` allocs/op	`1.02`
`BenchmarkTextEditing`	`5081288124` ns/op 3982609488 B/op 20647718 allocs/op	`4945220217` ns/op 3903650032 B/op 19608556 allocs/op	`1.03`
`BenchmarkTextEditing - ns/op`	`5081288124` ns/op	`4945220217` ns/op	`1.03`
`BenchmarkTextEditing - B/op`	`3982609488` B/op	`3903650032` B/op	`1.02`
`BenchmarkTextEditing - allocs/op`	`20647718` allocs/op	`19608556` allocs/op	`1.05`
`BenchmarkTree/10000_vertices_to_protobuf`	`4038651` ns/op 6262982 B/op 70025 allocs/op	`3490789` ns/op 6263051 B/op 70025 allocs/op	`1.16`
`BenchmarkTree/10000_vertices_to_protobuf - ns/op`	`4038651` ns/op	`3490789` ns/op	`1.16`
`BenchmarkTree/10000_vertices_to_protobuf - B/op`	`6262982` B/op	`6263051` B/op	`1.00`
`BenchmarkTree/10000_vertices_to_protobuf - allocs/op`	`70025` allocs/op	`70025` allocs/op	`1`
`BenchmarkTree/10000_vertices_from_protobuf`	`221803333` ns/op 442299708 B/op 290039 allocs/op	`156547615` ns/op 442172702 B/op 290039 allocs/op	`1.42`
`BenchmarkTree/10000_vertices_from_protobuf - ns/op`	`221803333` ns/op	`156547615` ns/op	`1.42`
`BenchmarkTree/10000_vertices_from_protobuf - B/op`	`442299708` B/op	`442172702` B/op	`1.00`
`BenchmarkTree/10000_vertices_from_protobuf - allocs/op`	`290039` allocs/op	`290039` allocs/op	`1`
`BenchmarkTree/20000_vertices_to_protobuf`	`9012996` ns/op 12716931 B/op 140028 allocs/op	`7982396` ns/op 12716926 B/op 140028 allocs/op	`1.13`
`BenchmarkTree/20000_vertices_to_protobuf - ns/op`	`9012996` ns/op	`7982396` ns/op	`1.13`
`BenchmarkTree/20000_vertices_to_protobuf - B/op`	`12716931` B/op	`12716926` B/op	`1.00`
`BenchmarkTree/20000_vertices_to_protobuf - allocs/op`	`140028` allocs/op	`140028` allocs/op	`1`
`BenchmarkTree/20000_vertices_from_protobuf`	`865626680` ns/op 1697263568 B/op 580041 allocs/op	`700685034` ns/op 1697272044 B/op 580044 allocs/op	`1.24`
`BenchmarkTree/20000_vertices_from_protobuf - ns/op`	`865626680` ns/op	`700685034` ns/op	`1.24`
`BenchmarkTree/20000_vertices_from_protobuf - B/op`	`1697263568` B/op	`1697272044` B/op	`1.00`
`BenchmarkTree/20000_vertices_from_protobuf - allocs/op`	`580041` allocs/op	`580044` allocs/op	`1.00`
`BenchmarkTree/30000_vertices_to_protobuf`	`14114183` ns/op 19318489 B/op 210030 allocs/op	`12821671` ns/op 19318254 B/op 210030 allocs/op	`1.10`
`BenchmarkTree/30000_vertices_to_protobuf - ns/op`	`14114183` ns/op	`12821671` ns/op	`1.10`
`BenchmarkTree/30000_vertices_to_protobuf - B/op`	`19318489` B/op	`19318254` B/op	`1.00`
`BenchmarkTree/30000_vertices_to_protobuf - allocs/op`	`210030` allocs/op	`210030` allocs/op	`1`
`BenchmarkTree/30000_vertices_from_protobuf`	`1956664037` ns/op 3752043640 B/op 870044 allocs/op	`1638786215` ns/op 3752682616 B/op 870046 allocs/op	`1.19`
`BenchmarkTree/30000_vertices_from_protobuf - ns/op`	`1956664037` ns/op	`1638786215` ns/op	`1.19`
`BenchmarkTree/30000_vertices_from_protobuf - B/op`	`3752043640` B/op	`3752682616` B/op	`1.00`
`BenchmarkTree/30000_vertices_from_protobuf - allocs/op`	`870044` allocs/op	`870046` allocs/op	`1.00`

This comment was automatically generated by workflow using github-action-benchmark.

codecov · 2024-10-17T02:06:33Z

Codecov Report

Attention: Patch coverage is 28.57143% with 55 lines in your changes missing coverage. Please review.

Project coverage is 46.83%. Comparing base (bd2ebe9) to head (e68cb71).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
server/rpc/connecthelper/status.go	0.00%	34 Missing ⚠️
api/converter/errors.go	0.00%	13 Missing ⚠️
internal/metaerrors/metaerrors.go	84.21%	2 Missing and 1 partial ⚠️
client/auth.go	0.00%	2 Missing ⚠️
client/client.go	60.00%	2 Missing ⚠️
internal/validation/validation.go	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1037      +/-   ##
==========================================
+ Coverage   46.44%   46.83%   +0.39%     
==========================================
  Files          81       84       +3     
  Lines       11887    12108     +221     
==========================================
+ Hits         5521     5671     +150     
- Misses       5806     5873      +67     
- Partials      560      564       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…r Auth webhook

coderabbitai · 2024-10-23T01:02:41Z

Warning

Rate limit exceeded

@hackerwins has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between c481be9 and e68cb71.

Walkthrough

The changes in this pull request introduce several modifications across various files, primarily focusing on enhancing error handling and validation mechanisms. A new function ErrorMetadataOf is added to the converter package to extract metadata from errors. The verifyAccess function in the webhook.go file has been updated for better error reporting, including the introduction of new error variables. The status.go file has been modified to improve error mapping to connectRPC status codes. Additionally, tests have been updated to reflect these changes, ensuring comprehensive coverage of new error handling scenarios.

Changes

File	Change Summary
`api/converter/errors.go`	Added function `ErrorMetadataOf(err error) map[string]string` to extract error metadata.
`server/rpc/auth/webhook.go`	Renamed `ErrNotAllowed` to `ErrPermissionDenied`, added `ErrUnexpectedResponse`, updated response checks in `verifyAccess`.
`server/rpc/connecthelper/status.go`	Updated error mappings, added functions `metaErrorToConnectError` and `formErrorToConnectError`, removed `structErrorToConnectError`.
`test/integration/auth_webhook_test.go`	Updated response handling logic, refactored tests, added new test cases for various scenarios.
`api/types/updatable_project_fields_test.go`	Renamed variable `structError` to `formErr` in validation tests.
`api/types/user_fields_test.go`	Renamed variable `structError` to `formErr` in password validation tests.
`internal/validation/validation.go`	Renamed `StructError` to `FormError`, updated validation logic to use the new type.
`internal/validation/validation_test.go`	Updated tests to use `FormError` instead of `StructError`.
`internal/metaerrors/metaerrors.go`	Introduced `metaerrors` package with `MetaError` type for enhanced error reporting.
`internal/metaerrors/metaerrors_test.go`	Added unit tests for `metaerrors` package functionalities.
`client/client.go`	Added method `SetToken(token string) error` to update client authentication token.
`client/options.go`	Added field `RPCAddress string` to `Options` struct for storing RPC server address.

Assessment against linked issues

Objective	Addressed	Explanation
Add `PermissionDenied` error response to webhook (205)	✅

Possibly related PRs

Detach documents when client is deactivated #1036: This PR is related as it introduces functionality to detach documents from a client when that client is deactivated, which aligns with the changes made in the main PR that involves error handling related to errors in the converter package. Both PRs deal with error handling and document management in the context of client operations.

Suggested labels

enhancement 🌟

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (7)

internal/richerror/richerror.go (2)
17-18: Enhance package documentation with more details.

The current package documentation is minimal. Consider adding:

Detailed description of use cases

Examples of how to use RichError

Integration patterns with authorization webhook

Common metadata keys and their purposes

Example enhancement:
-// Package richerror provides a rich error type that can be used to wrap errors
+// Package richerror provides enhanced error handling capabilities by wrapping errors
+// with additional metadata. It is primarily used in the authorization webhook to
+// provide detailed error information for authentication and authorization failures.
+//
+// Example usage:
+//
+//	return RichError{
+//		Err: fmt.Errorf("permission denied"),
+//		Metadata: map[string]string{
+//			"code": "403",
+//			"reason": "insufficient_permissions",
+//		},
+//	}
17-18: Document integration with authorization webhook.

Since this package is crucial for the new authorization webhook response format, consider adding documentation about:

How the metadata maps to response codes (200, 401, 403)

Standard metadata keys used in authorization context

Examples specific to authentication/authorization scenarios

Example documentation:
// Package richerror provides enhanced error handling for the authorization webhook.
//
// Standard metadata keys for authorization:
// - "code": HTTP status code (200, 401, 403)
// - "message": Human-readable error message
//
// Authentication example:
//
//	return richerror.New(
//		errors.New("invalid token"),
//		map[string]string{
//			"code": "401",
//			"message": "Token has expired",
//		},
//	)
//
// Authorization example:
//
//	return richerror.New(
//		errors.New("permission denied"),
//		map[string]string{
//			"code": "403",
//			"message": "User lacks required permissions",
//		},
//	)
api/converter/errors.go (1)
29-46: LGTM! Consider enhancing documentation.

The implementation looks good and aligns well with the PR objectives. However, consider adding documentation that describes:

The expected structure of the metadata map

Common metadata keys that consumers should expect (e.g., 'code', 'message')

Example usage with the new authorization response codes (200, 401, 403)

Add documentation like this:
 // ErrorMetadataOf returns the error metadata of the given error.
+//
+// The returned metadata map may contain the following keys:
+// - "code": The error code (e.g., "200" for OK, "401" for Unauthenticated, "403" for Permission Denied)
+// - "message": A descriptive message about the error
+//
+// Returns nil if the error cannot be converted to a connect.Error or if no metadata is found.
 func ErrorMetadataOf(err error) map[string]string {
api/types/auth_webhook.go (2)
129-144: Consider enhancing the Code type with additional utility methods.

The new Code type and constants are well-defined and properly documented. The use of standard HTTP status codes (200, 401, 403) is a good choice.

Consider adding these utility methods to improve the type's usability:
type Code int

+// String returns the string representation of the Code.
+func (c Code) String() string {
+    switch c {
+    case CodeOK:
+        return "OK"
+    case CodeUnauthenticated:
+        return "Unauthenticated"
+    case CodePermissionDenied:
+        return "PermissionDenied"
+    default:
+        return fmt.Sprintf("Unknown(%d)", c)
+    }
+}
+
+// IsValid returns true if the Code is a valid response code.
+func (c Code) IsValid() bool {
+    switch c {
+    case CodeOK, CodeUnauthenticated, CodePermissionDenied:
+        return true
+    default:
+        return false
+    }
+}
147-150: Add validation in NewAuthWebhookResponse.

The updated response structure looks good, but the constructor should validate the response codes.

Add validation in the NewAuthWebhookResponse function:
func NewAuthWebhookResponse(reader io.Reader) (*AuthWebhookResponse, error) {
	resp := &AuthWebhookResponse{}

	if err := json.NewDecoder(reader).Decode(resp); err != nil {
		return nil, fmt.Errorf("%s: %w", err.Error(), ErrInvalidWebhookResponse)
	}

+	// Validate the response code
+	if !resp.Code.IsValid() {
+		return nil, fmt.Errorf("invalid response code %d: %w", resp.Code, ErrInvalidWebhookResponse)
+	}

	return resp, nil
}
test/integration/auth_webhook_test.go (2)

Line range hint 99-127: Clarity improved by renaming to "successful authorization test"

Renaming the test enhances readability and clearly conveys the test's purpose, which is important for maintainability.

Line range hint 216-217: Avoid using time.Sleep to reduce test execution time

Using time.Sleep(projectInfoCacheTTL) slows down the test suite. Consider invalidating the project info cache programmatically or configuring a shorter TTL during tests to improve efficiency.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ee6fcf5 and 411d1c9.

📒 Files selected for processing (6)

api/converter/errors.go (1 hunks)
api/types/auth_webhook.go (1 hunks)
internal/richerror/richerror.go (1 hunks)
server/rpc/auth/webhook.go (4 hunks)
server/rpc/connecthelper/status.go (5 hunks)
test/integration/auth_webhook_test.go (14 hunks)

🧰 Additional context used

🔇 Additional comments (15)

api/converter/errors.go (1)
29-46: Verify error handling flow integration.

Let's verify how this function is used in the error handling flow, particularly with the new authorization response codes.

✅ Verification successful

Let me gather more information about the error handling integration, particularly focusing on the auth webhook test usage and the error metadata handling.

Based on the gathered information, I can now provide a final response as we have sufficient evidence about the error handling flow integration.

Error handling flow is properly integrated and verified

The verification shows that:

ErrorMetadataOf is correctly used in auth webhook tests to verify error metadata containing proper error codes and messages

Error metadata construction is consistently implemented across the codebase:

In connecthelper/status.go, errors are properly converted to connect errors with metadata

The metadata includes error codes and additional context when available

Integration tests validate the error flow for authentication scenarios (invalid token, no token)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check the usage of ErrorMetadataOf in the error handling flow
# Expected: Find calls to ErrorMetadataOf and verify integration with auth webhook

# Test 1: Find direct usage of ErrorMetadataOf
echo "=== Direct Usage of ErrorMetadataOf ==="
rg -l "ErrorMetadataOf"

# Test 2: Check auth webhook response handling
echo -e "\n=== Auth Webhook Response Handling ==="
rg -A 5 "type AuthWebhookResponse struct"

# Test 3: Check error conversion logic
echo -e "\n=== Error Conversion Logic ==="
rg -A 5 "func.*Error.*connect\.Error"
Length of output: 1879

Script:
#!/bin/bash
# Let's examine the auth webhook test implementation and error metadata usage

# Check the auth webhook test implementation
echo "=== Auth Webhook Test Implementation ==="
rg -A 10 "ErrorMetadataOf" test/integration/auth_webhook_test.go

# Check for error metadata construction
echo -e "\n=== Error Metadata Construction ==="
rg -A 5 "errdetails\.ErrorInfo" --type go

# Check for auth webhook error handling
echo -e "\n=== Auth Webhook Error Handling ==="
rg -A 5 "AuthWebhookResponse.*Error" --type go
Length of output: 2614
api/types/auth_webhook.go (1)
147-150: Verify all consumers of AuthWebhookResponse are updated.

This is a breaking change in the response format. Let's ensure all consumers are properly updated.

✅ Verification successful

Let me gather more information to verify the impact of this change thoroughly.

Based on the search results, I can now provide a final response about the impact of the changes to AuthWebhookResponse.

The changes to AuthWebhookResponse are properly handled throughout the codebase

The search results show that all usages of AuthWebhookResponse are already using the new format (Code and Message fields) consistently across the codebase:

The test files (test/integration/auth_webhook_test.go) are using the new fields correctly with various response codes (CodeOK, CodePermissionDenied, CodeUnauthenticated)

The server implementation (server/rpc/auth/webhook.go) properly handles the new response format

The backend caching mechanism (server/backend/backend.go) is compatible with the new structure

No references to the old fields (Allowed, Reason) were found in the codebase

The change appears to be part of a coordinated update where all consumers were properly modified to use the new response format.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for direct usage of the old fields (Allowed, Reason)
echo "Checking for potentially outdated field usage..."
rg -l "\.Allowed\b|\.Reason\b" --type go

# Search for AuthWebhookResponse construction or handling
echo -e "\nChecking AuthWebhookResponse usage..."
ast-grep --pattern 'AuthWebhookResponse{$$$}'
Length of output: 350

Script:
#!/bin/bash
# Let's look for any test files that might be using AuthWebhookResponse
echo "Checking test files for AuthWebhookResponse usage..."
rg -l "AuthWebhookResponse" --type go --type-add 'go:*_test.go'

# Let's see the actual implementation context where AuthWebhookResponse is used
echo -e "\nChecking context of AuthWebhookResponse usage..."
rg -B 5 -A 5 "AuthWebhookResponse" --type go

# Let's also check for any HTTP handlers or middleware that might be using this type
echo -e "\nChecking for HTTP handlers using this response..."
ast-grep --pattern 'func $_($_, $_) {
  $$$
  AuthWebhookResponse
  $$$
}'
Length of output: 10148
server/rpc/auth/webhook.go (4)
31-31: LGTM! Well-defined error variables with clear distinctions.

The new error variables and messages effectively distinguish between different authorization scenarios (authentication vs. permission issues), aligning well with the PR objectives.

Also applies to: 37-50

105-119: LGTM! Comprehensive response code handling.

The implementation properly distinguishes between different authorization scenarios:

200 (OK): Access granted

401 (Unauthenticated): Invalid/expired credentials

403 (Permission Denied): Insufficient permissions

Other codes: Unexpected response

The rich error handling for unauthenticated cases will help with token refresh scenarios.

140-140: LGTM! Improved error message clarity.

The error message now includes the status code, which will help with debugging unexpected webhook responses.

Line range hint 73-119: Verify test coverage for webhook response scenarios.

Please ensure that the test suite covers all possible webhook response scenarios:

Valid response with code 200

Invalid/expired token with code 401

Permission denied with code 403

Unexpected response codes

Cache behavior for different response types

✅ Verification successful

Let me gather more information about the test coverage by checking the implementation details of these test files.

Based on the test file content, I can now generate the final response as all the necessary information has been gathered.

All webhook response scenarios are properly tested

The test suite comprehensively covers all the required scenarios with dedicated test cases:

Valid response (200):

TestProjectAuthWebhook "successful authorization test"

Cache behavior tested in TestAuthWebhookCache

Invalid/expired token (401):

TestProjectAuthWebhook "unauthenticated response test"

Tests both missing token and invalid token cases

Permission denied (403):

TestProjectAuthWebhook "permission denied response test"

Cache behavior tested in TestAuthWebhookCache

Unexpected responses:

TestAuthWebhookErrorHandling covers unexpected status codes

TestAuthWebhookErrorHandling covers unexpected response codes

Tests retry behavior with unavailable servers

Tests timeout scenarios

Cache behavior:

TestAuthWebhookCache thoroughly tests caching for different response types

Verifies TTL behavior for both authorized and unauthorized responses

Confirms non-caching behavior for other response types
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check test coverage for webhook response scenarios

# Look for test cases covering different response codes
rg -A 10 "TestAuthWebhook|TestVerifyAccess" test/
Length of output: 1635

Script:
#!/bin/bash
# Check the complete implementation of auth webhook tests
cat test/integration/auth_webhook_test.go
Length of output: 17434
server/rpc/connecthelper/status.go (5)

29-29: Import richerror to enhance error handling

The import of github.com/yorkie-team/yorkie/internal/richerror allows for richer error information, aligning with the PR objectives to improve error response structures.

83-85: Map authentication errors to CodeUnauthenticated

The addition of auth.ErrUnexpectedResponse, auth.ErrUnauthenticated, and auth.ErrWebhookTimeout to the errorToConnectCode map with connect.CodeUnauthenticated correctly categorizes these errors as authentication issues.

88-90: Add ErrPermissionDenied to error mapping for authorization errors

Including auth.ErrPermissionDenied with connect.CodePermissionDenied accurately reflects the new distinction between authentication and authorization errors as per the updated response codes.

132-134: Update errorToCode mapping with new error codes

Adding "ErrPermissionDenied" and "ErrUnauthenticated" to the errorToCode map ensures that these errors have corresponding string representations for error metadata.

273-276: Prioritize rich error conversion in ToStatusError

By invoking richErrorToConnectError first in the ToStatusError function, you correctly give precedence to rich errors, allowing for enhanced error details to be included in the response.

test/integration/auth_webhook_test.go (4)

53-61: Correct handling of response codes in newAuthServer function

The logic for setting res.Code based on req.Token correctly aligns with the updated response structure, ensuring appropriate authorization responses.

70-83: Proper simulation of service unavailability in newUnavailableAuthServer

The test server accurately simulates temporary unavailability by returning ServiceUnavailable until recoveryCnt is reached, which is crucial for testing retry mechanisms.

Line range hint 238-394: Comprehensive testing of error handling in TestAuthWebhookErrorHandling

Adding extensive test cases for various error scenarios significantly improves test coverage and ensures robust error handling.

Line range hint 396-596: Effective caching mechanism tests in TestAuthWebhookCache

The tests thoroughly verify the caching behavior for authorized and unauthorized responses, ensuring the cache operates as expected under different scenarios.

internal/richerror/richerror.go

server/rpc/auth/webhook.go

server/rpc/connecthelper/status.go

test/integration/auth_webhook_test.go

…resh-auth-token

coderabbitai

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (2)

internal/detailederror/detailederror.go (1)
17-18: Enhance package documentation.

Consider expanding the package documentation to include:

A more detailed explanation of its purpose and use cases

Examples of how to use DetailedError

Documentation of how it integrates with the new error code system

Example improvement:
-// Package detailederror provides a metadata that can be used to wrap errors.
+// Package detailederror provides functionality for wrapping errors with additional metadata.
+//
+// It is primarily used to enhance error handling by attaching structured metadata to errors,
+// such as error codes, messages, and other contextual information. This is particularly
+// useful for authentication and authorization scenarios where detailed error information
+// needs to be propagated through the system.
+//
+// Example usage:
+//
+//	err := &DetailedError{
+//		Err: fmt.Errorf("permission denied"),
+//		Metadata: map[string]string{
+//			"code": "403",
+//			"message": "User lacks required permissions",
+//		},
+//	}
internal/validation/validation.go (1)
122-123: Consider renaming ValidationError to Error to avoid package name stuttering.

The type name will be used as validation.ValidationError by other packages, which is redundant. Since this is already in the validation package, consider renaming it to just Error.

Apply this diff to improve the naming:
-// ValidationError is the error returned by the validation of struct.
-type ValidationError struct {
+// Error is the error returned by the validation of struct.
+type Error struct {
🧰 Tools

🪛 golangci-lint

[warning] 123-123: exported: type name will be used as validation.ValidationError by other packages, and that stutters; consider calling this Error

(revive)

🪛 GitHub Check: build

[failure] 123-123:
exported: type name will be used as validation.ValidationError by other packages, and that stutters; consider calling this Error (revive)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 411d1c9 and ad2d9a6.

📒 Files selected for processing (8)

api/types/auth_webhook.go (1 hunks)
api/types/updatable_project_fields_test.go (3 hunks)
api/types/user_fields_test.go (2 hunks)
internal/detailederror/detailederror.go (1 hunks)
internal/validation/validation.go (2 hunks)
internal/validation/validation_test.go (1 hunks)
server/rpc/auth/webhook.go (4 hunks)
server/rpc/connecthelper/status.go (6 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

api/types/auth_webhook.go
server/rpc/auth/webhook.go

🧰 Additional context used

🪛 golangci-lint

internal/validation/validation.go

[warning] 123-123: exported: type name will be used as validation.ValidationError by other packages, and that stutters; consider calling this Error

(revive)

🪛 GitHub Check: build

internal/validation/validation.go

[failure] 123-123:
exported: type name will be used as validation.ValidationError by other packages, and that stutters; consider calling this Error (revive)

🔇 Additional comments (16)

internal/detailederror/detailederror.go (1)
1-28: Verify integration with authorization webhook.

The DetailedError package provides a good foundation for the new error handling system. However, we should verify its integration with the authorization webhook implementation.

✅ Verification successful

Let's gather more specific information about the webhook implementation and its error handling.

This will help us understand how DetailedError is being used in the webhook implementation and verify the error handling patterns.

Based on the webhook implementation analysis, I can now provide a final response:

DetailedError is properly integrated with authorization webhook

The DetailedError implementation is correctly integrated with the authorization webhook, specifically for handling unauthenticated responses. The implementation:

Uses DetailedError to wrap ErrUnauthenticated with additional message metadata

Properly handles different response codes (OK, PermissionDenied, Unauthenticated)

Maintains consistent error handling patterns with proper error wrapping
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check if DetailedError is properly integrated with the authorization webhook

# Test 1: Look for webhook response handling code
echo "Checking webhook response handling..."
rg -l "DetailedError" "server/rpc/auth"

# Test 2: Look for specific error codes mentioned in PR
echo "Checking error code usage..."
rg -A 5 "(401|403)" "server/rpc/auth"
Length of output: 254

Script:
#!/bin/bash
# Let's examine the webhook.go file content and look for DetailedError usage
echo "Checking webhook.go implementation..."
cat server/rpc/auth/webhook.go

# Also check for any HTTP status code handling patterns
echo -e "\nChecking HTTP status code patterns..."
rg -A 3 "http\.Status" server/rpc/auth/webhook.go
Length of output: 5963
api/types/user_fields_test.go (3)
45-45: LGTM: Consistent error type assertions.

The error assertions have been consistently updated across all test cases to use the new validationErr variable of type *validation.ValidationError. The test coverage remains comprehensive, validating various password criteria.

Also applies to: 52-52, 59-59, 66-66, 73-73, 80-80, 87-87

Line range hint 29-87: Clarify the relationship to PR objectives.

These validation-related changes appear unrelated to the stated PR objectives of updating the authorization webhook response format. Could you please clarify:

How these validation changes support the webhook response format updates, or

If these changes should be in a separate PR focused on validation refactoring?

29-29: Verify the validation package changes.

The change from StructError to ValidationError type appears to be part of a broader refactoring. While the change itself is straightforward, we should verify the validation package changes are complete.

✅ Verification successful

Let me gather more information about the validation error usage to ensure the refactoring is complete.

This will help us verify:

The context of ValidationError usage across the codebase

Test coverage and consistency in test files

Whether there are any missed references or inconsistencies

Based on the results, I can now generate the final response as we have comprehensive information about the ValidationError usage across the codebase.

The ValidationError type change is properly implemented and consistent

The validation package changes are complete and consistent across the codebase:

The ValidationError type is properly defined in internal/validation/validation.go

No remaining references to StructError were found

The type is consistently used across both implementation and test files

The change is properly reflected in error handling, including RPC error conversion
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify that all references to StructError have been updated to ValidationError

# Test 1: Check for any remaining references to StructError
echo "Checking for remaining StructError references..."
rg "StructError" -l

# Test 2: Verify the new ValidationError type exists and is properly defined
echo "Verifying ValidationError definition..."
ast-grep --pattern 'type ValidationError struct { $$$ }'
Length of output: 444

Script:
#!/bin/bash
# Check for all usages of ValidationError to ensure consistent usage
echo "Checking ValidationError usages..."
rg "ValidationError" -A 2 -B 2

# Check for any test files using validation errors
echo "Checking test files using validation..."
fd ".*_test.go" --exec rg "validation\." {} -l
Length of output: 3638
internal/validation/validation_test.go (2)
64-65: Verify test coverage for new error codes.

While the type change from StructError to ValidationError is appropriate, the test suite should be expanded to cover the new HTTP status codes (401, 403) mentioned in the PR objectives.

Let's verify the test coverage:
#!/bin/bash
# Search for test cases covering the new error codes
rg -A 5 "(401|403).*(Unauthenticated|Permission)" --type go
Would you like me to help generate additional test cases to cover the new error codes and response format?

64-65: Verify test location for webhook response format.

The changes to ValidationError look good, but we should ensure that the new webhook response format (codes 200, 401, 403) is tested in a more appropriate location, likely in an API or webhook-specific test file.

Let's check for webhook-related test files:

✅ Verification successful

Let me try different search patterns to locate webhook and response-related test files.

Let's analyze the test coverage for webhook-related functionality and HTTP response codes.

Let me check the webhook test implementation to verify the response format testing.

This will help us verify if the webhook response format and status codes are properly tested.

Webhook response format and status codes are properly tested

The test coverage for webhook response format and status codes is comprehensive in test/integration/auth_webhook_test.go. The tests include:

Success responses (200/OK)

Authentication failures (401/Unauthenticated)

Permission denied responses (403/Forbidden)

Error handling for unexpected status codes

Response format validation

Response caching behavior

The test implementation properly verifies all the scenarios mentioned in the review comment, including:

Response codes (200, 401, 403)

Error messages and codes in the response format

Edge cases like invalid tokens and missing tokens
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for webhook-related test files
fd -e go -e test webhook

# Search for existing webhook response format tests
rg -A 5 "webhook.*response.*format" --type go
Length of output: 188

Script:
#!/bin/bash
# Search for webhook-related files
fd webhook

# Search for response status code tests
rg -A 5 "(401|403|200).*response" --type go

# Search for test files containing HTTP status codes
rg -A 5 "http\.Status(OK|Unauthorized|Forbidden)" --type go --type-add 'go:*.go'

# Search for webhook handler tests
rg -A 5 "func.*Test.*Webhook" --type go
Length of output: 4272

Script:
#!/bin/bash
# Check the webhook test implementation
cat test/integration/auth_webhook_test.go

# Look for response status code assertions in webhook tests
rg -A 3 "assert.*Status.*response" test/integration/auth_webhook_test.go
rg -A 3 "w\.WriteHeader" test/integration/auth_webhook_test.go
Length of output: 17770
api/types/updatable_project_fields_test.go (4)
29-29: LGTM! Clear and consistent error variable declaration.

The renamed variable validationErr better describes its purpose and aligns with the updated validation error type system.

85-115: LGTM! Consistent error handling across test cases.

The error assertions are consistently updated throughout the test file, maintaining a uniform approach to validation error handling.

Line range hint 29-115: Verify test coverage for new auth webhook response format.

While the error handling updates look good, the test suite might need additional coverage for the new authorization webhook response format ({code: number, message: string}) and the specific response codes (200, 401, 403) mentioned in the PR objectives.
#!/bin/bash
# Search for tests related to auth webhook response handling
echo "Searching for auth webhook response tests..."
rg -l --type go --glob "*test.go" "AuthWebhook.*Response|code.*message"

# Search for specific response code tests
echo "Searching for response code tests..."
rg -l --type go --glob "*test.go" "401.*Unauthenticated|403.*Permission.*Denied"
71-71: 🛠️ Refactor suggestion

Consider expanding auth webhook method validation tests.

Given that this PR modifies the authorization webhook response format, consider adding test cases for the new response codes (200, 401, 403) in the auth webhook methods validation.
internal/validation/validation.go (1)

Line range hint 128-137: LGTM! Clean and efficient error message formatting.

The implementation efficiently uses strings.Builder for concatenation and properly handles formatting with newlines and whitespace trimming.

🧰 Tools

🪛 golangci-lint

[warning] 123-123: exported: type name will be used as validation.ValidationError by other packages, and that stutters; consider calling this Error

(revive)

🪛 GitHub Check: build

[failure] 123-123:
exported: type name will be used as validation.ValidationError by other packages, and that stutters; consider calling this Error (revive)
server/rpc/connecthelper/status.go (5)
132-134: LGTM: Error string mappings are consistent

The error string mappings correctly correspond to the new auth error codes.

216-218: Prevent metadata key collisions

The metadata merging could overwrite existing keys, including the important "code" key.

Line range hint 227-279: LGTM: Well-structured validation error handling

The validation error handling implementation:

Properly converts validation errors to connect.Error with CodeInvalidArgument

Correctly transforms field violations into BadRequest details

Maintains good separation of concerns

273-281: Verify error handling precedence

The error handling precedence has been updated to:

Detailed errors

Validation errors

Standard errors

Please verify this order aligns with the expected error handling requirements.

Let's check the error handling flow:
#!/bin/bash
# Description: Verify error handling usage patterns
# Expect: Consistent error handling patterns across the codebase

# Search for ToStatusError usage patterns
rg "ToStatusError\(" --type go -B 2 -A 2
83-90: LGTM: Error code mappings align with new auth response format

The updated error mappings correctly differentiate between authentication and authorization errors, properly mapping:

Authentication errors (401) to connect.CodeUnauthenticated

Permission errors (403) to connect.CodePermissionDenied

Let's verify that all auth-related errors are properly mapped:

✅ Verification successful

Let me gather more information about auth error definitions to ensure complete coverage.

This will help us verify if all defined auth errors are properly mapped in the status.go file.

Let me try a different approach to find error definitions.

This should help us find error definitions regardless of how they are declared or initialized.

Based on the search results, I can now generate the final response as we have found all auth-related error definitions and their mappings.

✓ Error mappings are complete and correctly implemented

The error mappings in status.go properly handle all auth-related errors defined in the codebase:

Authentication errors (all mapped to connect.CodeUnauthenticated):

ErrUnexpectedStatusCode

ErrUnexpectedResponse

ErrWebhookTimeout

ErrUnauthenticated

Authorization errors (mapped to connect.CodePermissionDenied):

ErrPermissionDenied

The only additional auth error found was ErrUnexpectedSigningMethod from the token manager, which is an internal error and appropriately not exposed in the RPC layer mappings.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all auth-related errors are properly mapped to connect codes
# Expect: No unmapped auth errors

# Search for auth error definitions
rg -l "type.*Error.*struct" server/rpc/auth/
# Search for their usage in error mappings
rg "auth\.Err\w+" server/rpc/connecthelper/status.go
Length of output: 2618

Script:
#!/bin/bash
# Search for error definitions in auth package
rg "var Err\w+" server/rpc/auth/ -A 1

# Search for custom error type definitions
rg "type.*Error" server/rpc/auth/
Length of output: 75

Script:
#!/bin/bash
# Search for error-related code in auth package with more context
rg "var|const|type.*[Ee]rr" server/rpc/auth/ -B 2 -A 2

# Also search for error initialization patterns
rg "errors\.New|fmt\.Errorf" server/rpc/auth/ -B 2 -A 2
Length of output: 6419

internal/detailederror/detailederror.go

internal/validation/validation.go

coderabbitai

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (4)

internal/metaerrors/metaerrors.go (2)
17-18: Enhance package documentation

The package documentation could be more comprehensive. Consider adding:

Examples of usage

Common use cases

Details about metadata format expectations
-// Package metaerrors provides a way to attach metadata to errors.
+// Package metaerrors provides functionality to attach structured metadata to errors.
+//
+// This package is particularly useful when additional context needs to be passed
+// along with errors, such as in authentication/authorization scenarios. The metadata
+// can include information like error codes, request IDs, or any other key-value pairs
+// that provide context about the error.
+//
+// Example usage:
+//
+//	err := someOperation()
+//	if err != nil {
+//	    return metaerrors.New(err, map[string]string{
+//	        "code": "401",
+//	        "request_id": "abc123",
+//	    })
+//	}
1-59: Add authorization-specific constants and helpers

Since this package is being introduced as part of the authorization webhook response format update, consider adding specific constants and helper functions for authorization errors:
+// Common metadata keys for authorization errors
+const (
+    KeyStatusCode = "status_code"
+    KeyErrorType  = "error_type"
+)
+
+// NewAuthError creates a new MetaError with authorization-specific metadata
+func NewAuthError(err error, code int, errorType string) *MetaError {
+    return New(err, map[string]string{
+        KeyStatusCode: strconv.Itoa(code),
+        KeyErrorType:  errorType,
+    })
+}
+
+// IsAuthError checks if the given error is an authorization error
+func IsAuthError(err error) bool {
+    var metaErr *MetaError
+    if errors.As(err, &metaErr) {
+        _, hasCode := metaErr.Metadata[KeyStatusCode]
+        _, hasType := metaErr.Metadata[KeyErrorType]
+        return hasCode && hasType
+    }
+    return false
+}
internal/metaerrors/metaerrors_test.go (1)
29-57: Consider adding more test cases for edge scenarios.

While the current test cases cover the basic functionality well, consider adding tests for:

Empty metadata map (map[string]string{})

Multiple levels of error wrapping

Special characters in metadata values that might need escaping

Example test case:
t.Run("test meta error with special characters", func(t *testing.T) {
    err := errors.New("error message")
    metaErr := metaerrors.New(err, map[string]string{
        "key": "value with [brackets]",
        "path": "/some/path",
    })
    assert.Equal(t, `error message [key=value with [brackets],path=/some/path]`, metaErr.Error())
})
server/rpc/connecthelper/status.go (1)
273-281: Consider adding documentation for the error conversion precedence.

The error conversion chain is well-structured, but it would be helpful to document why metaErrorToConnectError takes precedence over the other conversions.

Add a comment before the ToStatusError function:
// ToStatusError converts errors to connect.Error using the following precedence:
// 1. Rich errors with metadata (metaErrorToConnectError)
// 2. Pre-mapped errors (errorToConnectError)
// 3. Form validation errors (formErrorToConnectError)
// 4. Falls back to internal error if no specific conversion applies

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 148ac37 and 1bdbe91.

📒 Files selected for processing (5)

internal/metaerrors/metaerrors.go (1 hunks)
internal/metaerrors/metaerrors_test.go (1 hunks)
internal/validation/validation.go (3 hunks)
server/rpc/auth/webhook.go (4 hunks)
server/rpc/connecthelper/status.go (6 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

internal/validation/validation.go
server/rpc/auth/webhook.go

🔇 Additional comments (3)

internal/metaerrors/metaerrors_test.go (2)

1-27: LGTM: Package structure and license header are well-organized.

The file follows Go best practices with proper license header, package naming convention, and clean imports.

29-57: Verify integration with new error response format.

Since this package will be used to support the new authorization webhook response format, please verify that the metadata handling aligns with the new requirements.

✅ Verification successful

Let me check the webhook implementation to understand how metaerrors is being used.

Let me check the auth webhook response types to complete the verification.

MetaErrors package correctly supports the auth webhook response format

The implementation aligns perfectly with the auth webhook requirements:

The metaerrors package is used in webhook.go specifically for the CodeUnauthenticated (401) case, where it properly attaches the error message as metadata
The error format matches the webhook response structure, where:
- Base errors are defined as constants (ErrUnauthenticated)
- Additional context is added via metadata
- The resulting format supports both the error message and supplementary information

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of metaerrors in auth webhook implementation
rg -l "metaerrors" --type go | grep -i "auth"

# Search for HTTP status code constants that might be used with metaerrors
ast-grep --pattern 'const (
  $$$
  StatusCode$_ = $_
  $$$
)'

Length of output: 135

Script:

#!/bin/bash
# Check the webhook implementation
cat server/rpc/auth/webhook.go

# Also check if there are any other error-related constants or types
rg -g '*.go' 'type.*Error' -A 5

Length of output: 6098

Script:

#!/bin/bash
# Check the auth webhook response types
cat api/types/auth_webhook.go

# Also check if there are any error codes defined
rg -g '*.go' 'Code[A-Z][a-zA-Z]+\s*=' -A 2

Length of output: 4938

server/rpc/connecthelper/status.go (1)

83-89: LGTM! Error code mappings align with the new auth response format.

The changes correctly implement the new authorization response format by:

Mapping authentication failures (invalid/expired tokens) to CodeUnauthenticated (401)
Mapping permission issues to CodePermissionDenied (403)

Let's verify that all auth-related errors are properly mapped:

✅ Verification successful

All auth-related errors are properly mapped in the error code mappings

After verification, I can confirm that all error types defined in the auth package are correctly mapped in the status.go file:

ErrUnexpectedSigningMethod -> Not mapped (internal error, defaults to Internal)
ErrPermissionDenied -> CodePermissionDenied
ErrUnexpectedStatusCode -> CodeUnauthenticated
ErrUnexpectedResponse -> CodeUnauthenticated
ErrWebhookTimeout -> CodeUnauthenticated
ErrUnauthenticated -> CodeUnauthenticated

The mappings are complete and correctly categorize authentication failures vs permission issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all auth-related errors are mapped to appropriate connect codes
# Expected: All auth package errors should be mapped in errorToConnectCode

# Find all error declarations in the auth package
rg -t go '(var|const)\s+Err[^\s]+\s+=\s+' server/rpc/auth/

# Compare with mappings in status.go
rg -t go 'auth\.Err[^:]+:\s+connect\.Code' server/rpc/connecthelper/status.go

Length of output: 445

Script:

#!/bin/bash
# Let's try a different approach to find error declarations in auth package
# Using ast-grep to find error variable declarations

ast-grep --pattern 'var Err$_ = errors.New($_)'  server/rpc/auth/
ast-grep --pattern 'var Err$_ = fmt.Errorf($_)'  server/rpc/auth/

Length of output: 133

Script:

#!/bin/bash
# Let's try using ripgrep to find error declarations with more context
rg -t go -A 2 -B 2 'var\s+Err[A-Z][a-zA-Z]*\s*=' server/rpc/auth/

Length of output: 67

Script:

#!/bin/bash
# Let's check the entire auth package content to understand error definitions
rg -t go --no-filename '^[^/]*var\s+|^[^/]*const\s+|^[^/]*type\s+Error' server/rpc/auth/

Length of output: 208

Script:

#!/bin/bash
# Let's check the content of all go files in auth package to find error definitions
find server/rpc/auth/ -name "*.go" -exec cat {} \; | grep -A 2 -B 2 'Err[A-Z][a-zA-Z]*\s*='

Length of output: 1070

internal/metaerrors/metaerrors.go

server/rpc/connecthelper/status.go

coderabbitai

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)

test/integration/auth_webhook_test.go (2)

Line range hint 44-85: Consider improving test constants and error messages

The auth server implementation could be enhanced for better maintainability and clarity:

+const (
+    validToken = "valid_token"
+    notAllowedToken = "not_allowed_token"
+    invalidToken = "invalid_token"
+)

 func newAuthServer(t *testing.T) (*httptest.Server, string) {
-    token := xid.New().String()
+    token := validToken

     return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
         req, err := types.NewAuthWebhookRequest(r.Body)
         assert.NoError(t, err)

         var res types.AuthWebhookResponse
         if req.Token == token {
             res.Code = types.CodeOK
-        } else if req.Token == "not allowed token" {
+        } else if req.Token == notAllowedToken {
             res.Code = types.CodePermissionDenied
+            res.Message = "user lacks required permissions"
         } else if req.Token == "" {
             res.Code = types.CodeUnauthenticated
             res.Message = "no token"
         } else {
             res.Code = types.CodeUnauthenticated
             res.Message = "invalid token"
         }

Line range hint 396-597: Extract magic numbers into named constants

The cache tests use magic numbers for expected request counts. These should be constants for better maintainability and clarity.

+const (
+    // Expected request counts for cache tests
+    expectedCachedRequests = 2    // One initial request + one after TTL
+    expectedUncachedRequests = 6  // All requests go through when caching is disabled
+)

 // Usage in tests:
-assert.Equal(t, 2, reqCnt)  // In cached response tests
+assert.Equal(t, expectedCachedRequests, reqCnt)

-assert.Equal(t, 6, reqCnt)  // In uncached response test
+assert.Equal(t, expectedUncachedRequests, reqCnt)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1bdbe91 and 13efe8f.

📒 Files selected for processing (3)

server/rpc/auth/webhook.go (4 hunks)
server/rpc/connecthelper/status.go (6 hunks)
test/integration/auth_webhook_test.go (14 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

server/rpc/auth/webhook.go

🔇 Additional comments (5)

server/rpc/connecthelper/status.go (3)

82-92: LGTM! Error code mappings align with PR objectives.

The new error mappings correctly implement the distinction between authentication and authorization scenarios:

ErrUnauthenticated maps to 401 for invalid/expired tokens
ErrPermissionDenied maps to 403 for permission issues
Internal errors properly mapped to 500

191-227: Skip comment about metadata collision.

A previous review already identified the potential issue with metadata key collisions.

Line range hint 229-265: LGTM! Proper form error handling implementation.

The function correctly converts form validation errors to connect errors with appropriate field violation details.

test/integration/auth_webhook_test.go (2)

Line range hint 1-597: Implementation successfully meets PR objectives

The test implementation thoroughly covers:

New response format with proper status codes
Clear distinction between authentication and authorization errors
Comprehensive error handling scenarios
Proper caching behavior for different response types

While there are some minor improvements suggested above, the overall implementation is solid and well-tested.

133-142: 🛠️ Refactor suggestion

Extract duplicated project update code into a helper function

The project update code is repeated across multiple test cases. This duplication was previously flagged in past reviews and should be addressed.

+func updateProjectWebhookURL(ctx context.Context, t *testing.T, adminCli *client.Client, project *types.Project, webhookURL string) {
+    project.AuthWebhookURL = webhookURL
+    _, err := adminCli.UpdateProject(
+        ctx,
+        project.ID.String(),
+        &types.UpdatableProjectFields{
+            AuthWebhookURL: &project.AuthWebhookURL,
+        },
+    )
+    assert.NoError(t, err)
+}

 // Usage in tests:
-project.AuthWebhookURL = authServer.URL
-_, err := adminCli.UpdateProject(
-    ctx,
-    project.ID.String(),
-    &types.UpdatableProjectFields{
-        AuthWebhookURL: &project.AuthWebhookURL,
-    },
-)
-assert.NoError(t, err)
+updateProjectWebhookURL(ctx, t, adminCli, project, authServer.URL)

Also applies to: 174-181, 312-321, 341-347, 505-512

coderabbitai · 2024-10-31T07:27:35Z

server/rpc/connecthelper/status.go

+	if err, ok := metaErrorToConnectError(err); ok {
+		return err
+	}
+
 	if err, ok := errorToConnectError(err); ok {
 		return err
 	}

-	if err, ok := structErrorToConnectError(err); ok {
+	if err, ok := formErrorToConnectError(err); ok {


💡 Codebase verification

Test coverage needed for error handling priority

Based on the test file content, the current test coverage is insufficient for the error handling priority chain. The tests only cover errorToConnectError with basic cases, while missing coverage for:

metaErrorToConnectError conversion

formErrorToConnectError conversion

Priority order verification when an error could match multiple converters

Edge cases and fallback to internal error

Specific gaps in test coverage:

No test cases for ToStatusError function's conversion priority

Missing tests for metaErrorToConnectError conversion of rich errors

Missing tests for formErrorToConnectError validation errors

No verification of the fallback to internal error

🔗 Analysis chain

Verify error handling priority in tests.

The error handling flow has been updated with a clear priority:

Rich errors (with metadata)

Standard errors

Form validation errors

Internal error fallback

Please ensure test coverage verifies this priority, especially for errors that could match multiple converters.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash # Description: Check test coverage for error handling priority # Test: Look for test cases covering error handling priority rg -l "TestToStatusError|TestErrorHandling" --type go # Test: Check for specific error type assertions in tests rg "errors\.As.*MetaError|errors\.As.*FormError" --type go

Length of output: 116

Script:

#!/bin/bash # Let's try a broader search for test files and error handling patterns # Find test files in the connecthelper directory fd ".*_test\.go$" server/rpc/connecthelper/ # Look for error conversion functions and their tests rg -A 5 "(metaErrorToConnectError|errorToConnectError|formErrorToConnectError)" server/rpc/connecthelper/ # Look for ToStatusError tests with different context rg -A 10 "ToStatusError" server/rpc/connecthelper/

Length of output: 3974

Script:

#!/bin/bash # Let's check the test file content to see how errors are tested # Show the full content of status_test.go cat server/rpc/connecthelper/status_test.go # Look for any test helper functions or test setup related to errors rg -B 2 -A 5 "func Test.*Error" server/rpc/connecthelper/status_test.go

Length of output: 1283

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)

test/integration/auth_webhook_test.go (4)
54-55: Consider extracting magic strings into constants.

The string "not allowed token" is used directly in the code. Consider extracting it into a constant for better maintainability and reusability.
+const (
+    notAllowedToken = "not allowed token"
+)

-		} else if req.Token == "not allowed token" {
+		} else if req.Token == notAllowedToken {
152-153: Enhance error metadata assertions.

The error metadata assertions could be more robust by checking all fields in the metadata map. Consider adding assertions for any additional metadata fields that might be present.
-assert.Equal(t, map[string]string{"code": connecthelper.CodeOf(auth.ErrUnauthenticated), "message": "no token"}, converter.ErrorMetadataOf(err))
+metadata := converter.ErrorMetadataOf(err)
+assert.Equal(t, connecthelper.CodeOf(auth.ErrUnauthenticated), metadata["code"])
+assert.Equal(t, "no token", metadata["message"])
+assert.Len(t, metadata, 2, "unexpected additional metadata fields")
Also applies to: 164-165

239-243: Make retry configuration more explicit.

The retry configuration could be more explicit about its purpose and constraints.
-	var recoveryCnt uint64 = 4
+	const (
+		// Maximum number of retries before considering the webhook unavailable
+		// Must be less than server's maximum allowed retries
+		maxRetryAttempts uint64 = 4
+	)

-	conf.Backend.AuthWebhookMaxRetries = recoveryCnt
+	conf.Backend.AuthWebhookMaxRetries = maxRetryAttempts
Line range hint 401-412: Extract request counting logic into a reusable helper.

The request counting logic is duplicated across test cases. Consider extracting it into a helper type that provides a more robust way to verify cache behavior.
type requestCounter struct {
    count   int32
    methods map[types.Method]bool
}

func newRequestCounter() *requestCounter {
    return &requestCounter{
        methods: make(map[types.Method]bool),
    }
}

func (rc *requestCounter) handleRequest(w http.ResponseWriter, r *http.Request) {
    req, err := types.NewAuthWebhookRequest(r.Body)
    if err != nil {
        http.Error(w, err.Error(), http.StatusBadRequest)
        return
    }
    
    atomic.AddInt32(&rc.count, 1)
    rc.methods[req.Method] = true
}

func (rc *requestCounter) getCount() int {
    return int(atomic.LoadInt32(&rc.count))
}
Also applies to: 477-489

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 13efe8f and bb02c70.

📒 Files selected for processing (3)

server/rpc/auth/webhook.go (4 hunks)
server/rpc/connecthelper/status.go (6 hunks)
test/integration/auth_webhook_test.go (14 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

server/rpc/auth/webhook.go

🔇 Additional comments (4)

server/rpc/connecthelper/status.go (3)

82-91: LGTM: Error mappings align with webhook response format changes.

The updated error mappings properly distinguish between authentication and authorization scenarios:

ErrUnauthenticated → CodeUnauthenticated (401)
ErrPermissionDenied → CodePermissionDenied (403)
Webhook-related errors → CodeInternal (500)

Also applies to: 134-139

275-283: LGTM: Clear error handling priority established.

The updated ToStatusError function implements a well-defined error handling priority:

Rich errors with metadata
Standard mapped errors
Form validation errors
Fallback to internal error

191-227: Verify error handling consistency across the codebase.

The new metaErrorToConnectError function introduces rich error handling. Let's verify its usage and consistency.

✅ Verification successful

Let me gather more information about error handling patterns to ensure a comprehensive verification.

Error handling implementation is consistent and well-structured

The verification reveals a consistent error handling pattern across the codebase:

Error conversion is centralized in connecthelper package with dedicated conversion functions
Rich error handling with metadata is properly implemented in metaErrorToConnectError
Error types (ErrPermissionDenied, ErrUnauthenticated) are consistently used in auth package
Metadata handling follows a uniform pattern across error conversions

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent error handling across the codebase

# Test: Check for MetaError usage patterns
rg "metaerrors\.New|metaerrors\.Wrap" --type go

# Test: Look for potential error type conversions
rg "errors\.As.*MetaError" --type go

# Test: Check error handling in webhook-related code
rg "ErrPermissionDenied|ErrUnauthenticated" server/rpc/auth/

Length of output: 1486

Script:

#!/bin/bash
# Check for error code mapping and conversion patterns
ast-grep --pattern 'var errorToConnectCode = map[$_]connect.Code{'

# Look for other error conversion functions
rg "func.*Error.*To.*Error" --type go

# Check for error metadata handling patterns
rg "Metadata\[.*\]" --type go -A 2

Length of output: 569

test/integration/auth_webhook_test.go (1)

Line range hint 214-215: Improve test reliability by avoiding hard-coded sleep.

The hard-coded sleep for cache TTL could make the test flaky. Consider using a test helper that verifies the cache was actually cleared or implementing a more deterministic approach.

Consider implementing a helper function that ensures cache invalidation:

func waitForCacheInvalidation(ttl time.Duration) {
    // Add small buffer to ensure cache is cleared
    time.Sleep(ttl + 100*time.Millisecond)
}

coderabbitai

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (6)

client/options.go (1)
44-45: Improve documentation for the RPCAddress field.

The current documentation is minimal. Consider adding more context about:

The expected format of the address (e.g., "host:port")

Whether this is required or optional

The default value if not specified
-	// RPCAddress is the address of the RPC server.
+	// RPCAddress is the address of the RPC server in the format "host:port".
+	// If not specified, the default address will be used.
server/rpc/auth/webhook.go (1)

Line range hint 53-53: Consider adding documentation about the response format change.

Since this is a significant change in the webhook response format, consider:

Adding code comments describing the expected response format

Updating any relevant API documentation

Adding migration guidelines for webhook implementations
test/integration/auth_webhook_test.go (2)
606-648: Token refresh handling needs additional test cases

While the basic token refresh scenario is tested, consider adding test cases for:

Multiple token refresh attempts

Token refresh with expired tokens

Token refresh with rate limiting

Example test case:
t.Run("multiple token refresh attempts test", func(t *testing.T) {
    // Test implementation
})

t.Run("token refresh with rate limiting test", func(t *testing.T) {
    // Test implementation
})
Line range hint 486-491: Inconsistent response handling

The test sets Allowed = false after setting StatusForbidden, which is redundant and potentially confusing with the new response format.

Update the response to use only the new format:
 			w.WriteHeader(http.StatusForbidden)
 			var res types.AuthWebhookResponse
-			res.Allowed = false
+			res.Code = 403
+			res.Message = "Permission Denied"
client/client.go (2)
209-222: LGTM: Token refresh implementation with suggested improvements

The implementation correctly handles token updates while maintaining existing configurations. It properly manages connection lifecycle by closing idle connections before reconnecting.

Consider adding token validation before updating:
 func (c *Client) SetToken(token string) error {
+    if token == "" {
+        return errors.New("token cannot be empty")
+    }
     newClientOptions := []connect.ClientOption{
209-209: Documentation: Add error scenarios

Consider enhancing the method documentation to describe possible error scenarios, such as connection failures or invalid tokens.

Add error documentation:
-// SetToken updates the client's token for reauthentication purposes.
+// SetToken updates the client's token for reauthentication purposes.
+//
+// Returns an error if:
+// - The token is empty
+// - Failed to establish connection with the server
+// - The server rejected the new token

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between fd41769 and 035634b.

📒 Files selected for processing (4)

client/client.go (2 hunks)
client/options.go (1 hunks)
server/rpc/auth/webhook.go (5 hunks)
test/integration/auth_webhook_test.go (14 hunks)

🔇 Additional comments (10)

client/options.go (1)

44-46: Verify the relationship with PR objectives.

The addition of the RPCAddress field appears unrelated to the PR's main objective of updating the authorization webhook response format. Please clarify:

How does this change support the new authentication/authorization flow?
Is this part of a broader architectural change not mentioned in the PR description?

✅ Verification successful

Let me gather more context about the usage of RPCAddress and its relationship with authentication.

Let me check for any changes related to this PR by looking at the git history and diff.

Let me check for the broader context of authentication and RPC usage in the codebase.

Let me check for any recent changes related to RPCAddress and its usage in authentication/authorization.

Based on the git history and code context, I can now provide a final response:

The RPCAddress field addition is part of the authentication infrastructure

The RPCAddress field was added to support client reconnection during token updates. This is evidenced by:

The field is used in SetToken method which handles reauthentication
When updating authentication tokens, the client needs to reconnect using the stored RPC address
This change is directly related to authentication as it enables seamless token updates while maintaining the connection

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for related changes in authentication/authorization code
rg -l "RPCAddress" | xargs rg -l "auth|token"

Length of output: 80

Script:

#!/bin/bash
# Check the content of client/client.go and client/options.go for RPCAddress usage
echo "=== client/client.go ==="
rg -A 5 -B 5 "RPCAddress" client/client.go

echo -e "\n=== client/options.go ==="
rg -A 5 -B 5 "RPCAddress" client/options.go

Length of output: 977

Script:

#!/bin/bash
# Check git diff and history for RPCAddress changes
echo "=== Git diff for RPCAddress ==="
git diff HEAD^ HEAD -- client/options.go client/client.go

echo -e "\n=== PR description and commits ==="
gh pr view --json title,body,commits

Length of output: 310

Script:

#!/bin/bash
# Look for authentication/authorization related code near RPC usage
echo "=== Auth context in client.go ==="
rg -A 10 -B 10 "auth|token" client/client.go

echo -e "\n=== Check for recent changes ==="
git log -p --grep="auth\|webhook\|rpc" -i -- client/

Length of output: 135690

Script:

#!/bin/bash
# Check recent commits and changes related to RPCAddress
git log -p --since="6 months ago" -- client/options.go client/client.go | grep -A 5 -B 5 "RPCAddress\|auth"

Length of output: 2402

server/rpc/auth/webhook.go (4)

37-48: LGTM: Error variables clearly define authentication states.

The new error variables provide clear distinction between authentication failures (ErrUnauthenticated) and permission issues (ErrPermissionDenied), aligning well with the PR objectives.

122-125: Duplicate of existing comment about caching logic.

141-141: LGTM: Improved error reporting with status code.

Including the status code in the error message will help with debugging integration issues.

Line range hint 96-120: Consider handling inconsistent status code and allowed flag combinations.

The current implementation assumes specific combinations of status codes and the allowed flag. However, there might be edge cases where they don't align as expected. For example:

200 OK with allowed=false
403 Forbidden with allowed=true
401 Unauthorized with allowed=true

Consider adding logging for these unexpected combinations to help debug integration issues.

test/integration/auth_webhook_test.go (4)

Line range hint 242-398: Comprehensive error handling test coverage

The error handling test cases are well-structured and cover important scenarios:

Unexpected status codes
Unexpected webhook responses
Server unavailability and retry mechanism
Recovery after temporary unavailability

This aligns well with robust error handling requirements.

Line range hint 400-604: Well-implemented caching test cases

The caching tests thoroughly verify:

Caching of authorized responses
Caching of permission denied responses
Non-caching of other responses
Cache TTL behavior

The implementation includes proper request counting and timing verification.

139-146: Refactor repeated project update code

The code for updating the project's AuthWebhookURL is repeated across multiple tests.

Also applies to: 174-184, 312-323, 341-351, 505-517

32-40: Consider organizing imports

The imports are not grouped by standard library, external packages, and internal packages.

client/client.go (1)

186-186: LGTM: Storing RPC address for token refresh support

The addition of storing the RPC address is necessary to support token refresh functionality, allowing reconnection with updated tokens.

client/options.go

test/integration/auth_webhook_test.go

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)

test/integration/auth_webhook_test.go (2)
Line range hint 242-398: Consider adding edge case tests

While the error handling is comprehensive, consider adding tests for:

Malformed response bodies

Network timeouts

Rate limiting scenarios

Line range hint 400-604: Add concurrent access tests for cache

The cache implementation tests are thorough for single-threaded scenarios. Consider adding tests for concurrent access patterns to verify thread safety:

Multiple clients accessing cache simultaneously

Race conditions during cache updates
func TestConcurrentCacheAccess(t *testing.T) {
    const numGoroutines = 10
    var wg sync.WaitGroup
    for i := 0; i < numGoroutines; i++ {
        wg.Add(1)
        go func() {
            defer wg.Done()
            // Concurrent cache access logic
        }()
    }
    wg.Wait()
}

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 035634b and c481be9.

📒 Files selected for processing (1)

test/integration/auth_webhook_test.go (14 hunks)

🔇 Additional comments (3)

test/integration/auth_webhook_test.go (3)

53-65: Response format needs updating

Line range hint 133-197: Well-structured test coverage for authentication scenarios

The test cases effectively cover the distinction between authentication and authorization scenarios, validating both error codes and messages.

606-648: Enhance token refresh test coverage

Consider adding tests for:

Token refresh during active operations
Multiple token refreshes
Invalid token formats

Also, verify the impact on existing connections when refreshing tokens:

hackerwins

Thanks for your contribution.

This commit replaces static authentication tokens with a dynamic token injection interface, improving auth lifecycle management. Key features include: 1. Dynamic Token Management - New authTokenInjector function replaces static token configuration - Automatic token refresh on 401 (Unauthorized) responses - Proper error handling for 403 (Forbidden) responses 2. Authentication Error Handling - Added auth-error event subscription for monitoring - Automatic retry for syncLoop and watchLoop operations - Manual retry for activate/deactivate/attach/detach operations Example usage: const client = new yorkie.Client(YorkieAPIAddr, { apiKey, authTokenInjector: async (reason) => { if (reason === 'token expired') { return await refreshAccessToken(); } return accessToken; }, }); Related to yorkie-team/yorkie#1037 --------- Co-authored-by: Youngteac Hong <[email protected]>

Add Code type for authentication webhook response

cdd952e

chacha912 requested a review from hackerwins October 17, 2024 01:28

Modify tests to handle new Code type in auth webhook response

b73e95b

chacha912 force-pushed the refresh-auth-token branch from e79cd11 to b73e95b Compare October 17, 2024 01:59

github-actions bot reviewed Oct 17, 2024

View reviewed changes

Introduce RichError struct to extend error responses with metadata fo…

411d1c9

…r Auth webhook

chacha912 marked this pull request as ready for review October 23, 2024 01:02

coderabbitai bot reviewed Oct 23, 2024

View reviewed changes

Merge branch 'main' of https://github.com/yorkie-team/yorkie into ref…

5e54e52

…resh-auth-token

chacha912 mentioned this pull request Oct 24, 2024

Add dynamic token refresh and authentication error handling yorkie-team/yorkie-js-sdk#911

Merged

5 tasks

coderabbitai bot reviewed Oct 24, 2024

View reviewed changes

hackerwins force-pushed the refresh-auth-token branch from ad2d9a6 to 5e54e52 Compare October 24, 2024 05:01

hackerwins added 2 commits October 31, 2024 15:23

Rename StructError to FormError

148ac37

Rename RichError to MetaError

1bdbe91

coderabbitai bot reviewed Oct 31, 2024

View reviewed changes

internal/metaerrors/metaerrors.go Show resolved Hide resolved

internal/metaerrors/metaerrors.go Show resolved Hide resolved

internal/metaerrors/metaerrors.go Show resolved Hide resolved

server/rpc/connecthelper/status.go Show resolved Hide resolved

coderabbitai bot reviewed Oct 31, 2024

View reviewed changes

hackerwins force-pushed the refresh-auth-token branch from 13efe8f to bb02c70 Compare October 31, 2024 09:34

coderabbitai bot reviewed Oct 31, 2024

View reviewed changes

Adjust status codes of webhook errors

fd41769

hackerwins force-pushed the refresh-auth-token branch from bb02c70 to fd41769 Compare October 31, 2024 09:42

chacha912 added 2 commits November 1, 2024 15:45

Add SetToken to update token when token is invalid

51d0ada

Move authorized status codes to HTTP response header

035634b

coderabbitai bot reviewed Nov 1, 2024

View reviewed changes

client/options.go Outdated Show resolved Hide resolved

test/integration/auth_webhook_test.go Show resolved Hide resolved

coderabbitai bot reviewed Nov 1, 2024

View reviewed changes

hackerwins force-pushed the refresh-auth-token branch from c481be9 to 6c9985c Compare November 1, 2024 10:56

Clean up codes

e68cb71

hackerwins force-pushed the refresh-auth-token branch from 6c9985c to e68cb71 Compare November 1, 2024 10:59

hackerwins approved these changes Nov 1, 2024

View reviewed changes

hackerwins merged commit 5aaacfb into main Nov 1, 2024
5 checks passed

hackerwins deleted the refresh-auth-token branch November 1, 2024 11:09

devleejb mentioned this pull request Nov 2, 2024

Add Support for Authorization Webhook Status Code yorkie-team/codepair#395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Authorization Webhook Response Format and Handling #1037

Update Authorization Webhook Response Format and Handling #1037

chacha912 commented Oct 17, 2024 •

edited by coderabbitai bot

Loading

github-actions bot left a comment •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

coderabbitai bot commented Oct 23, 2024 •

edited

Loading

Rate limit exceeded

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot Oct 31, 2024

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

hackerwins left a comment

Update Authorization Webhook Response Format and Handling #1037

Update Authorization Webhook Response Format and Handling #1037

Conversation

chacha912 commented Oct 17, 2024 • edited by coderabbitai bot Loading

Summary by CodeRabbit

Summary by CodeRabbit

Release Notes

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Go Benchmark

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

coderabbitai bot commented Oct 23, 2024 • edited Loading

Rate limit exceeded

Walkthrough

Changes

Assessment against linked issues

Possibly related PRs

Suggested labels

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Oct 31, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

hackerwins left a comment

Choose a reason for hiding this comment

chacha912 commented Oct 17, 2024 •

edited by coderabbitai bot

Loading

github-actions bot left a comment •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

coderabbitai bot commented Oct 23, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)