Skip to content
This repository has been archived by the owner on Dec 21, 2019. It is now read-only.

Add option to specify secure cookies in the session store #33

Open
turnkey-commerce opened this issue Jul 3, 2016 · 2 comments · May be fixed by #34
Open

Add option to specify secure cookies in the session store #33

turnkey-commerce opened this issue Jul 3, 2016 · 2 comments · May be fixed by #34

Comments

@turnkey-commerce
Copy link
Contributor

turnkey-commerce commented Jul 3, 2016

There should be an option to make sure the cookie storage requires secure cookies for sites that have https available. It needs to be optional so that it would be supported in dev/testing environments that don't support https.

One possibility would be to add another argument to the NewAuthorizer:

func NewAuthorizer(backend AuthBackend, secureCookie bool, key []byte, defaultRole string, roles map[string]Role) (Authorizer, error) {
    var a Authorizer
    a.cookiejar = sessions.NewCookieStore([]byte(key))
    a.cookiejar.Options.Secure = secureCookie
...
}

or make it secure by default and require calling a Method to make it insecure (best practice):

func NewAuthorizer(backend AuthBackend, key []byte, defaultRole string, roles map[string]Role) (Authorizer, error) {
    var a Authorizer
    a.cookiejar = sessions.NewCookieStore([]byte(key))
    a.cookiejar.Options.Secure = true
...
}

func (a Authorizer) AllowNonHttpsCookie() {
    a.cookiejar.Options.Secure = false
}

One related issue to cover is that currently a login seems to fail silently if a.cookiejar.Options.Secure is set to true and it is on a site that does not support https.

@apexskier
Copy link
Owner

I think I'd prefer your latter suggestion: making cookies secure by default with a fallback method/option to allow non secure cookies. I'm very much open to PRs implementing this feature.

@turnkey-commerce
Copy link
Contributor Author

Thanks for the confirm, I'll work on that and issue a PR.

@turnkey-commerce turnkey-commerce linked a pull request Jul 23, 2016 that will close this issue
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants