Pull request

[fellou89]

Hello,

I've started to get the project I'm working on ready for production. I have generalized my auth backend so that it can be configured for different use cases. It leverages another caddy module I'm developing, it's mentioned in the README documentation I added.

[fellou89]

I don't expect this to be merged at the state it's in, I still need to write the full suite of unit tests, and rebase from your master branch; but I thought I should submit a pull request to get your notes and comments on it. Thanks!

[freman]

Hi.

Looks pretty good, just a couple of comments for you so far (I haven't had time to read it all)

I'm pretty sure you can put your own name in the MIT license of any file you've created yourself, at least that's what a quick google tells me. I can hardly own that code, you wrote it.

Check out https://github.com/golang/go/wiki/CodeReviewComments, specifically the Indent Error Flow section. Keeping track of the elses and indents makes it a little difficult to follow, I suggest using the third pattern.

Any reason https://github.com/freman/caddy-reauth/pull/8/files#diff-e5c054ab3dd5a3359c959db20f048436R186 this couldn't be one if with three &&'d parameters? Similar complaint for https://github.com/freman/caddy-reauth/pull/8/files#diff-e5c054ab3dd5a3359c959db20f048436R194

lifewindow / cleanwindow - Might be better as lifetime and cleaninterval but I'm not going to fight you on that, just I've not seen that terminology before.

Would you consider using strings.EqualFold for all your checks of f.Validation

It's probably better to skip the ioutil.ReadAll followed by json.Unmarshal - generally a better idea to use a limited body reader and json decoder. For example:

const limit = 1000000 // Some sensible limit that covers the maximum possible size of a real response plus some leeway 

dec := json.NewDecoder(io.LimitReader(refreshResp.Body, limit))
var body map[string]interface{}
if err := dec.Decode(&body); err != nil {
    return err
}

This saves memory, and prevents the service you're calling from spewing out garbage to the point you can't handle it.

IMPORTANT Please close your response body, it's a bit difficult the way it's written but once you refactor with a few less elses you should find it easy to defer refreshResp.Body.Close(). Without this you will be leaking.

When you can you should compile your regular expressions just one time, you can do that by declaring a package level variable. For example:

var reKeyCheck = regexp.MustCompile(`(#[[:alnum:]+\-*_*]+#)`)

As it stands replaceInputs only replaces the first #thing# it finds, the following is a simpler example

var reKeyCheck = regexp.MustCompile(`#([-\w]+)#`)
func replaceInputs(value string, inputMap map[string]string) string {
    keyMatch := reKeyCheck.FindStringSubmatch(value)
    if len(keyMatch) == 2 {
        return strings.Replace(value, keyMatch[0], inputMap[keyMatch[1]], -1)
    }
    return value
}

I suspect you're after something a little more like

var reKeyCheck = regexp.MustCompile(`#([-\w]+)#`)

func replaceInputs(value string, inputMap map[string]string) string {
    keyMatch := reKeyCheck.FindAllStringSubmatch(value, -1)
    for _, match := range keyMatch {
        value = strings.Replace(value, match[0], inputMap[match[1]], -1)
    }
    return value
}

[fellou89]

Most of those have been oversights on my part; I did catch that replace inputs bug in one of my unit tests. I will be updating the pull request with fixes for the above and some more unit tests probably tomorrow. Thanks for taking a look!

[freman]

While you're there.

Is there any particular reason you went with #thing# vs say {thing}, pre existing code or something?

The reason I ask is simple, I intend to support {}, and I'm concerned that ## might accidentally conflict with the # component of a url

[fellou89]

I picked # because I wanted something that would stand out in a yaml file and not get ignored as part of a string value. I have no preference though, I think your url concern is valid justification to switch. I won't be able to submit the PR today as I had planned, internet went out my place and threw off my schedule.

On Wed, Apr 4, 2018, 7:03 PM Shannon Wynter notifications@github.com wrote:

While you're there.

Is there any particular reason you went with #thing# vs say {thing}, pre existing code or something?

The reason I ask is simple, I intend to support {}, and I'm concerned that ## might accidentally conflict with the # component of a url

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/freman/caddy-reauth/pull/8#issuecomment-378771885, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2aE1ffuqlTuRuT2ODJIpr0FZI_08LIks5tlVEigaJpZM4TDGaq .

[fellou89]

Are there any other things I need to correct before this can be merged in?

[fellou89]

I've made the requested changes.

[freman]

Looking good, tests still work? Run go lint over it?

[fellou89]

Passing gometalinter except for a couple warnings. Tests are updated and working, I've also tested the code on my development and pre-production environments for the project I'm using it in and it looks good to go.

[fellou89]

Is it good to merge now? :)