Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace authn.Request for *http.Request #9

Merged
merged 11 commits into from
Sep 30, 2024
Merged

Replace authn.Request for *http.Request #9

merged 11 commits into from
Sep 30, 2024

Conversation

emcfarlane
Copy link
Collaborator

@emcfarlane emcfarlane commented Feb 2, 2024 •
edited
Loading

To improve testability of authn middleware and remove API surface in authn this PR proposes dropping the authn.Request in favour of *http.Request. This is a breaking change to the API. We remove all references to authn.Request objects which is replaced by accessing the *http.Request directly.

Fixes #8

akshayjshah
akshayjshah reviewed Feb 5, 2024
View reviewed changes
authn.go Outdated Show resolved Hide resolved
akshayjshah
akshayjshah reviewed Feb 5, 2024
View reviewed changes
authn.go Outdated Show resolved Hide resolved
@seandlg
Copy link

seandlg commented Jun 13, 2024

Any chance this can get merged? 😊

@emcfarlane
Remove authn.Request
b3a3e19 
Replaces authn.Request with *http.Request for better interop.

Signed-off-by: Edward McFarlane <[email protected]>
@emcfarlane emcfarlane changed the title Export constructor for Request Replace authn.Request for *http.Request Jun 14, 2024
@emcfarlane
Copy link
Collaborator Author

@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
terinjokes approved these changes Jun 14, 2024
View reviewed changes
Copy link

@terinjokes terinjokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a use case for Procedure and Protocol in this package? I've not needed to reach for them and they aren't used in the examples either.

@emcfarlane
Copy link
Collaborator Author

emcfarlane commented Jun 14, 2024 •
edited
Loading

@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.

@emcfarlane
Update README example
7013291 
Signed-off-by: Edward McFarlane <[email protected]>
seandlg
seandlg approved these changes Jun 15, 2024
View reviewed changes
Copy link

@seandlg seandlg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very lean. I have implemented authentication with this PR in my repo and the testing is a breeze, given that I can simply craft http.Request and test my AuthFunc.

akshayjshah
akshayjshah reviewed Jun 24, 2024
View reviewed changes
authn.go Outdated Show resolved Hide resolved
@terinjokes
Copy link

terinjokes commented Jul 16, 2024 •
edited
Loading

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?

@jhump
Copy link
Member

jhump commented Jul 22, 2024

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.

@emcfarlane
Add inferred methods
a8cdbbd 
Signed-off-by: Edward McFarlane <[email protected]>
jhump
jhump reviewed Sep 4, 2024
View reviewed changes
Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keep the implementations and tests you had over in the connect-go PR? They seemed strictly more correct.

authn.go Outdated
Comment on lines 101 to 103
if len(procedure) < 4 { // two slashes + service + method
return request.URL.Path
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark as in connect-go PR. This would incorrectly allow "//foo".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a testcase.

authn.go Outdated
// "/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, "/")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed allowing trailing suffix.

authn.go Outdated
Comment on lines 82 to 83
default:
return connect.ProtocolConnect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now return "", false.

@emcfarlane emcfarlane mentioned this pull request Sep 6, 2024
Closed
dependabot bot and others added 3 commits September 30, 2024 12:13
@dependabot @emcfarlane
Upgrade go, buf and google.golang.org/protobuf (connectrpc#13)
95670f4 
Bumps google.golang.org/protobuf from 1.32.0 to 1.33.0.
Bumps buf from 1.27.0 to 1.33.0.
Migrate buf configuration files to v2. Done by `buf config migrate`.
Regenerates the proto files used for testing.
Upgrades min supported go version to v1.20.x.

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Edward McFarlane <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Edward McFarlane <[email protected]>
Signed-off-by: Edward McFarlane <[email protected]>
@drice-buf @emcfarlane
Update CONTRIBUTING.md: CLA -> DCO (connectrpc#15)
c30f2e6 
See connectrpc/connect-query-es#386

Signed-off-by: Dan Rice <[email protected]>
Signed-off-by: Edward McFarlane <[email protected]>
@drice-buf @emcfarlane
Update DCO language (connectrpc#16)
ef16a43 
Signed-off-by: Dan Rice <[email protected]>
Signed-off-by: Edward McFarlane <[email protected]>
jhump and others added 3 commits September 30, 2024 12:13
@jhump @emcfarlane
Add jhump to maintainers (connectrpc#17)
08bcf75 
Signed-off-by: Josh Humphries <[email protected]>
Signed-off-by: Edward McFarlane <[email protected]>
@emcfarlane
Bump min Go to 1.21 (connectrpc#18)
be24644 
Upgrade go to 1.23 with min 1.21. Update deps. Fix lint issues.

Signed-off-by: Edward McFarlane <[email protected]>
@emcfarlane
Fix
a20d53d 
Signed-off-by: Edward McFarlane <[email protected]>
@emcfarlane
Copy link
Collaborator Author

@jhump updated to move the connect-go implementation here.

@emcfarlane emcfarlane merged commit 135799f into connectrpc:main Sep 30, 2024
5 of 7 checks passed
@emcfarlane emcfarlane deleted the ed/makeRequest branch September 30, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seandlg seandlg seandlg approved these changes

@jhump jhump jhump approved these changes

@terinjokes terinjokes terinjokes approved these changes

@akshayjshah akshayjshah Awaiting requested review from akshayjshah

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unable to create Requests
6 participants
@emcfarlane @seandlg @terinjokes @jhump @akshayjshah @drice-buf