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 ***@***.***> 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 <#8 (comment)>, 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?

[freman]

I like to avoid else's whenever I can, there's little cost in assigning a variable once, then immediately overwriting it but I find it easier to read.

url := h.refreshURL
if endpoint.Url != "" {
    url = h.endpoint.Url
}

[freman]

This would probably be easier to read as a switch and your error could use a little more information

switch endpoint.Method {
case http.MethodPost:
    refreshTokenReq, err = http.NewRequest(endpoint.Method, url+endpoint.Path, strings.NewReader(data.Encode()))
case http.MethodGet:
    refreshTokenReq, err = http.NewRequest(endpoint.Method, url+endpoint.Path+"?"+data.Encode(), nil)
default:
    err = fmt.Errorf("Endpoint '%s' had an unhandled method '%s'", endpoint.Name, endpoint.Method)
}
if err != nil {
    return nil, err
}

[freman]

Move this one up to the package level and name it better so it's easier to find and understand, I was just giving you an example :D

And my apologies, rather than io.LimitReader I should have given you http.MaxBytesReader

[fellou89]

to use http.MaxBytesReader a http.ResponseWriter is needed and Authenticate doesn't have access to the ServeHttp writer. At the moment I just have a httptest.ResponseRecorder, if you have a better idea let me know.

[freman]

no, lol that's fine, ignore this part, I clearly was as close to correct as there is the first time around.

[freman]

this *_* business isn't quite right, pop your regexs into https://regex101.com/ for an explanation of what it's doing and you can test it against various strings

the outer [ and ] create a single character matching class

What you're telling regexp to match is 1 or more (that trailing +)

• [:alnum:] (Any alpha numeric value [A-Z0-9])
• +
• -
• *
• _
• * (redundant because you're already matching * above)

What you want is to match is 1 or more (that trailing +)

• -
• \w (any word character, which is [A-Z0-9_])
• * (if you really do mean to match * and it's not just an accident)

And by shrinking your capture group to exclude the { and } you don't need to do the slice work in the replaceInputs function

So, {([-\w]+)}

then the keyMatch below will have

[
    ["{matchedstring}", "matchestring"],
    ["{anotherstring}","anotherstring"],
]

and can be used like this

value = strings.Replace(value, m[0], inputMap[m[1]], -1)

instead of

value = strings.Replace(value, m[0], inputMap[m[1][1:len(m[1])-1]], -1)

Less work for you, less work for the regexp engine

[freman]

This type assertion will result in a panic if the value returned is not a bool - https://play.golang.org/p/YPnhZF276um

if it's ok to assume that any value that isn't a bool is false then you could do something like

if client_auth, isa := secrets.GetValue(reauth, "client_authorization").(bool); isa && client_auth {

Check https://golang.org/ref/spec#Type_assertions for more info

[freman]

It's a nitpick but a simple string == "" benchmarks twice as fast (in ns) vs len(string) == 0 at least on my machine.

string == "" also has the benefit of not being confused with len(slice) == 0

There's a whole thread over here about it https://groups.google.com/forum/#!topic/golang-nuts/7Ks1iq2s7FA

[freman]

Looks like this is a nicely typed error, makes it easier to detect reliably

if _, isa := err.(*bigcache.EntryNotFoundError) {

[freman]

this else isn't required, if the above was true then it returns, if it hasn't returned then the return below will happen.

(if you haven't worked it out yet, else is one of my trigger words lol)

[freman]

This else isn't required, as it would have returned from the error block.

[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? :)