umputun / remark42

comment engine
https://remark42.com
MIT License
4.84k stars 376 forks source link

Do not allow commenting from disabled auth provider #1660

Closed cowsay1 closed 1 year ago

cowsay1 commented 1 year ago

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.

umputun commented 1 year ago

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 https://github.com/go-pkgz/auth/pull/176, which will check user id and will reject users with invalidated providers right away.

umputun commented 1 year ago

@paskal Initially, I've tried to add this verification as a part of token.ValidatorFunc with smth like this:

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.

umputun commented 1 year ago

@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

cowsay1 commented 1 year ago

Looks like all good :+1:

umputun commented 1 year ago

tagged as v1.21.1