-
-
Notifications
You must be signed in to change notification settings - Fork 384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not allow commenting from disabled auth provider #1660
Comments
There is a workaround to invalidate all tokens by changing the secret. This should work but can be too extreme as it will force relogin for all the users. Alternatively, you can wait until the cookie expires; eventually, the user with a turned-off provider will be kicked off. To address it properly, I have created go-pkgz/auth#176, which will check user id and will reject users with invalidated providers right away. |
@paskal Initially, I've tried to add this verification as a part of func (s *ServerCommand) isAllowedProvider(userID string) bool {
p := strings.Split(userID, "_")[0] // user id has "provider_" suffix
switch p {
case "anonymous":
return s.Auth.Anonymous
case "email":
return s.Auth.Email.Enable
case "telegram":
return s.Auth.Telegram
case "github":
return s.Auth.Github.CID != "" && s.Auth.Github.CSEC != ""
case "google":
return s.Auth.Google.CID != "" && s.Auth.Google.CSEC != ""
case "facebook":
return s.Auth.Facebook.CID != "" && s.Auth.Facebook.CSEC != ""
case "microsoft":
return s.Auth.Microsoft.CID != "" && s.Auth.Microsoft.CSEC != ""
case "yandex":
return s.Auth.Yandex.CID != "" && s.Auth.Yandex.CSEC != ""
case "twitter":
return s.Auth.Twitter.CID != "" && s.Auth.Twitter.CSEC != ""
case "patreon":
return s.Auth.Patreon.CID != "" && s.Auth.Patreon.CSEC != ""
case "apple":
return s.Auth.Apple.CID != "" && s.Auth.Apple.TID != "" && s.Auth.Apple.KID != ""
default:
return true
}
} This code is clear and easy to read; however, I didn't like it as it adds another place to maintain as we add a new provider. So, the final approach I picked is to add this check directly to auth library which is based on the list of all the active/registered providers. |
adopt tests for the mandatory provider check fix leftover test for the server
@cowsay1 The change I made to auth library was integrated. Pls, check the master image and let me know if there are any issues. If all good I'll release a hotfix version |
Looks like all good 👍 |
tagged as |
If the user logged in (worst case is anon), and after that we turn off auth provider that he used, he can still leave comments until he logged out.
The text was updated successfully, but these errors were encountered: