Skip to content
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

Key() method invoked with wrong siteID #1499

Closed
daniloarcidiacono opened this issue Sep 28, 2022 · 1 comment · Fixed by #1500
Closed

Key() method invoked with wrong siteID #1499

daniloarcidiacono opened this issue Sep 28, 2022 · 1 comment · Fixed by #1500
Milestone

Comments

@daniloarcidiacono
Copy link

Hello,
I noticed that in some places the Key() method of admin Store is called with an incorrect site id, precisely:

https://github.com/umputun/remark42/blob/master/backend/app/store/service/service.go#L986

func (s *DataStore) getSecret(siteID string) (secret string, err error) {
	if secret, err = s.AdminStore.Key("any"); err != nil {

https://github.com/umputun/remark42/blob/master/backend/app/cmd/server.go#L1110

SecretReader: token.SecretFunc(func(aud string) (string, error) { // get secret per site
	return admns.Key("")
}),

This is not a problem for the "shared" implementation since it supports only one set of data for all sites, however that's not the case for my custom RPC backend... moreover, the fix seems trivial in both cases:

https://github.com/umputun/remark42/blob/master/backend/app/store/service/service.go#L986

func (s *DataStore) getSecret(siteID string) (secret string, err error) {
	if secret, err = s.AdminStore.Key(siteID); err != nil {

https://github.com/umputun/remark42/blob/master/backend/app/cmd/server.go#L1110

SecretReader: token.SecretFunc(func(aud string) (string, error) { // get secret per site
        // go-pkgz/auth.go says:
        //       "SecretReader   token.Secret        // reader returns secret for given site id (aud), required"
	return admns.Key(aud)
}),

Thanks

@umputun
Copy link
Owner

umputun commented Sep 28, 2022

This is not a bug but rather a documented limitation with a comment describing this behavior.

// get secret for given siteID
// Note: secret shared across sites, but some sites can be disabled.
func (s *DataStore) getSecret(siteID string) (secret string, err error) {

I do agree with the proposal. Looks like it won't break StaticStore.Key as it ignores aud anyway

This should be made clear somehow in comments what passed key won't affect the default store

paskal added a commit that referenced this issue Sep 28, 2022
Previously it was set to fixed strings, likely a test artefact.

Resolves #1499.
umputun pushed a commit that referenced this issue Sep 28, 2022
Previously it was set to fixed strings, likely a test artefact.

Resolves #1499.
@paskal paskal added this to the v1.11.0 milestone Jan 9, 2023
itzomen pushed a commit to traleor/comments that referenced this issue Apr 16, 2023
Previously it was set to fixed strings, likely a test artefact.

Resolves umputun#1499.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants