Conversation
paskal
left a comment
There was a problem hiding this comment.
Thanks a ton! Tested this with GitHub OAuth endpoints on a live instance — works as expected. A few things:
-
built-in provider names aren't reserved (
server.go:1023). onlyemailandanonymousare blocked, butAUTH_CUSTOM_NAME=githuborAUTH_CUSTOM_NAME=telegramwould silently register a competing route. should add the full list:google,github,facebook,yandex,microsoft,patreon,discord,telegram,dev,apple. docs have the same gap — says "should not beemailoranonymous" but doesn't mention built-in provider names. -
user ID hashing uses SHA-256 (
server.go:1043) while all built-in providers use SHA-1. no comment explaining the choice. if intentional, add a note — otherwise it'll confuse anyone who switches from custom to a native provider later (IDs won't match, existing comments become disassociated). -
no happy-path test — only the partial-config error case is covered. worth adding a test that registers a fully configured custom provider and checks it appears in the provider list, similar to
TestServerApp_DevMode.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2006 +/- ##
==========================================
+ Coverage 62.03% 62.07% +0.03%
==========================================
Files 132 132
Lines 3037 3040 +3
Branches 788 791 +3
==========================================
+ Hits 1884 1887 +3
Misses 1039 1039
Partials 114 114 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I was just checking the docs and was looking for this feature. I would like to authenticate users with Codeberg (Foregejo). I'll try to test a custom provider with https://remark42.com/docs/contributing/backend/ ... |
fix: reserve built-in custom provider names
|
I’ve addressed the review feedback:
Could you please take another look when you have time? Thanks! |
umputun
left a comment
There was a problem hiding this comment.
nice feature, looks solid overall. few things I noticed:
-
"twitter"is missing fromreservedCustomProviderNames(server.go:505). it's still registered as a provider at line 1019, soAUTH_CUSTOM_NAME=twitterwould create a route conflict -
custom provider name isn't validated for URL-safe characters (
server.go:1038). it gets used directly in route paths (/auth/<name>/login), so names with spaces or special chars would break routing. docs say "must be URL-safe" but code doesn't enforce it - a simple regex like^[a-z0-9][a-z0-9_-]*$would close the gap -
if all user info fields (ID, email, name) come back empty from the provider,
sourceIDis""and all such users hash to the same ID (server.go:1050-1057). worth at least logging a warning -
docs don't mention only one custom provider is supported. users might expect to configure multiple
…ma233/remark42 into feat/custom-oauth2-provider
|
@umputun thanks, addressed all four points:
Please take another look when you have a chance. |
|
I am thinking that adding just one doesn't make much sense, adding more would be better. Three approaches from top of my mind: Option A: Fixed numbered slots (recommended) Custom1 CustomAuthGroup `group:"custom1" namespace:"custom1" env-namespace:"CUSTOM1"`
Custom2 CustomAuthGroup `group:"custom2" namespace:"custom2" env-namespace:"CUSTOM2"`
Custom3 CustomAuthGroup `group:"custom3" namespace:"custom3" env-namespace:"CUSTOM3"`Env vars:
Option B: Manual env scanning
Option C: JSON in a single env var
I'll think more about it after the merge and before we cut v1.16.0. Would love to hear your thoughts on that too, Alex. |
Thanks, that makes sense. I agree multiple custom providers would be better long-term. For this PR, I’d still prefer to keep the scope to a single custom provider and get the minimal version merged first. It already solves the main integration use case with a small, easy-to-review change set. If we add multi-provider support next, I’d lean toward Option A. It fits Remark42’s existing go-flags/env style best, keeps help text and validation straightforward, and is much friendlier for docker-compose users than JSON-in-env or manual env parsing. So my vote would be: merge this one as-is, then do multi-provider support in a follow-up PR. |
Custom OAuth2 integration on both frontend and backend.