Repository navigation
Refactoring for config patterns - #159
Conversation
| } | ||
|
|
||
| admin, err := kolide.NewUser("admin", "admin", "admin@kolide.co", true, false) | ||
| admin, err := kolide.NewUser("admin", "admin", "admin@kolide.co", true, false, config.Auth.BcryptCost) |
There was a problem hiding this comment.
I'm deleting the kolide.NewUser method as new users should only be created through the Service interface.
Having it in two places causes confusion.
There was a problem hiding this comment.
Cool. When I rebase I'll deal with that conflict.
|
@marpaia @groob I have a bunch of questions about this sessions code that I'm trying to refactor. In which situations do we need to "validate" sessions, and in which do we not? Currently Per #158 (comment), are we in agreement that Can you help me get a general sense for what all the |
|
|
||
| func (orm gormDB) validateSession(session *kolide.Session) error { | ||
| sessionLifeSpan := viper.GetFloat64("session.expiration_seconds") | ||
| func (orm gormDB) validateSession(session *kolide.Session, sessionLifeSpan time.Duration) error { |
There was a problem hiding this comment.
Intending to kill this in favor of service.validateSession.
|
@marpaia Can you please take a look at my other questions above? #159 (comment) |
|
It looks like we are in agreement that |
| Session: SessionConfig{ | ||
| KeySize: 64, | ||
| Duration: 24 * 90 * time.Hour, | ||
| CookieName: "KolideSession", |
There was a problem hiding this comment.
We're not using cookies anymore, maybe drop this field altogether?
There was a problem hiding this comment.
Are we using localstorage now?
There was a problem hiding this comment.
Yep, with the token sent to the server using the Authorization Bearer token. See #131
|
@groob I've tried not to make changes in the areas that your PR is touching, but some already got in there before you warned me. When do you expect to merge that PR? If it won't be for a while, I may merge this beforehand. |
|
It's not a problem. I'll rebase once you merge. |
| JWTKey: "foobar", | ||
| ds, err := datastore.New("inmem", "") | ||
| if err != nil { | ||
| panic("Could not create datastore: " + err.Error()) |
There was a problem hiding this comment.
Can we avoid using panics in favor of returning an error, or at least logging it an calling os.Exit() instead?
|
@marpaia please take a look at this when you get a chance. There's some major refactoring, and I think it's pretty much ready to merge. |
| } | ||
|
|
||
| // initFatal prints an error message and exits with a non-zero status. | ||
| func initFatal(err error, message string) { |
There was a problem hiding this comment.
I think it's SO silly that we don't panic here, lest the go kids don't think we're cool.
There was a problem hiding this comment.
IMO (and I saw your and @groob's discussion on this in chat) rather than having a holy war on this subject, we just need to decide what the user experience should be for fatal errors. Do we want to exit with a clean and simple error message, or do we want the backtrace that comes with a panic?
As a developer, I would prefer to see the backtrace. I'm not sure that's what our customers will prefer. Ultimately I don't think it matters much, as long as we are giving messages that are helpful enough to locate and then fix the error.
|
|
||
| // TestConfig returns a barebones configuration suitable for use in tests. | ||
| // Individual tests may want to override some of the values provided. | ||
| func TestConfig() KolideConfig { |
There was a problem hiding this comment.
I'm not super duper psyched about having a public TestXXX symbol although I see why you're doing it and I don't have a better suggestion.
|
I don't see anything egregious here. If you think we should add MySQL back in a separate commit, then feel free to merge this. Good work. |
This PR refactors most of the codebase to use the new config patterns implemented in #149. Now the core service keeps a copy of the
KolideConfigstruct, and service methods can reference the configuration in that struct when they need it. The most significant refactoring is in the sessions code, separating the business logic from the storage layer.