Minor restructure for greater confidence that only authenticated requests are proxied

[aeijdenberg]

Description

We've been reviewing parts of the code base to evaluate usage of this app (and we like what we see so far :) ). This PR is a minor tidy up that I think makes crucial parts easier to reason about.

It splits the Authenticate() method which previously returned an HTTP status code into 2 methods, one to handle session related work and which returns a more standard Go error code, and a second to add headers to requests.

It then changes both invocations such that only a nil error code regards the session as authenticated, whereas the old code didn't explicitly check for success, rather it checked for known errors and then assumed in the absence of an expected error code, it would imply success.

Motivation and Context

I found the old code a little difficult to reason about in terms of what side-effects Authenticate() had, and it was more work for someone reading the code to need to follow everything the method did in order for the caller to be satisfied that they were handlng it correctly.

For example, with the code as is, if in the future someone modified the Authenticate() method to return a 400 bad request if the JSON cookie wouldn't unmarshal, which on it's own would be totally reasonable, the request would then be proxied through to the upstream server as the current code would fall through to fail open if an unexpected error was returned.

How Has This Been Tested?

It hasn't. The code changes are pretty small, and it is assumed the unit tests and code review will pick up any issues.

Checklist:

• My change requires a change to the documentation or CHANGELOG.
• I have updated the documentation/CHANGELOG accordingly.
• I have created a feature (non-master) branch for my PR.

[JoelSpeed]

Thanks for working on this, I think this makes sense and improved the code. Added on minor nit below about using a switch that seems a little redundant, other than that, great!

Could you please update the Changelog to add a note about this PR

[JoelSpeed]

Is there any reason for using a switch here? Would a typical if err != nil block not suffice? (I say != as I would personally reorder the statements so that the happy path is the least indented)

[aeijdenberg]

Agreed - I think my first pass handled one type of error differently. Now though with only a non-nil check, the switch makes no sense. Have fixed as suggested. Thanks!

[JoelSpeed]

Looks good! Thanks for helping to clean up the code, we need a lot more of this 😅