Replace authn.Request for *http.Request #9
Conversation
|
Any chance this can get merged? 😊 |
emcfarlane
force-pushed
the
ed/makeRequest
branch
from
eb79a33
to
87caeaf
Compare
Replaces authn.Request with *http.Request for better interop. Signed-off-by: Edward McFarlane <[email protected]>
emcfarlane
force-pushed
the
ed/makeRequest
branch
from
87caeaf
to
b3a3e19
Compare
emcfarlane
changed the title
|
@terinjokes @seandlg thanks the interest. I've had to rewrite the history to sign commits. The PR has been updated to reflect the new proposal. |
|
@terinjokes the use case would be for authorization on specific methods. The example in #12 showcases using an allowlist base on which method is invoked. Protocol has been brought up before as some users wish to disable one or the other entirely. |
Signed-off-by: Edward McFarlane <[email protected]>
emcfarlane
force-pushed
the
ed/makeRequest
branch
from
d86599a
to
1144c96
Compare
Signed-off-by: Edward McFarlane <[email protected]>
emcfarlane
force-pushed
the
ed/makeRequest
branch
from
1144c96
to
478629f
Compare
|
Also works well in my project and allowed me to delete a bunch of code from the tests. Is it possible to do a release from 7013291 to allow the protocol functions to be moved to the runtime without blocking the improvements here? |
|
I think we'll want to finish connectrpc/connect-go#756 and get that merged before merging this so that when this lands, the accessors we are removing already have suitable replacements. Otherwise, I think this PR is probably ready to merge. |
Signed-off-by: Edward McFarlane <[email protected]>
emcfarlane
force-pushed
the
ed/makeRequest
branch
from
acd94c8
to
a8cdbbd
Compare
| if len(procedure) < 4 { // two slashes + service + method | ||
| return request.URL.Path | ||
| } |
There was a problem hiding this comment.
Same remark as in connect-go PR. This would incorrectly allow "//foo".
| // "/service/method". If the request path does not contain a procedure name, the | ||
| // entire path is returned. | ||
| func InferProcedure(request *http.Request) string { | ||
| path := strings.TrimSuffix(request.URL.Path, "/") |
There was a problem hiding this comment.
Why do we do this? Do connect RPC servers actually accept an invalid trailing slash like this? Pretty sure gRPC servers are usually strict and do not allow this.
| default: | ||
| return connect.ProtocolConnect |
There was a problem hiding this comment.
Is there no value in returning "unknown" (or empty string, etc) when the request doesn't look like any of these? Since this is middleware, it seems highly likely it could be used with a mux that has both connect and non-connect routes, so I think we do need better classification here.
To improve testability of
authnmiddleware and remove API surface inauthnthis PR proposes dropping theauthn.Requestin favour of*http.Request. This is a breaking change to the API. We remove all references toauthn.Requestobjects which is replaced by accessing the*http.Requestdirectly.Fixes #8