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

Implement require_auth with validate_request #290

Merged
merged 5 commits into from
Dec 2, 2022
Merged

Implement require_auth with validate_request #290

merged 5 commits into from
Dec 2, 2022

Conversation

82marbag
Copy link
Contributor

Motivation

This is a follow-up to: #289
ValidateRequest is an abstraction of a validator and RequireAuthorization is an instance, but the latter has yet to be implemented in terms of the former. They now are two independent types.

Solution

RequireAuthorization is now implemented by ValidateRequest.

This is a breaking change.

@82marbag
Copy link
Contributor Author

Next could be the async version

@82marbag
Copy link
Contributor Author

test-msrv fails on: Error: could not compile hashbrown

@82marbag
Copy link
Contributor Author

Any update on this?

@davidpdrsn davidpdrsn added the breaking change A PR that makes a breaking change. label Dec 2, 2022
davidpdrsn
davidpdrsn reviewed Dec 2, 2022
View reviewed changes
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Sorry the delay!

I think this looks great. Just a couple of nit picks.

If you merge latest master then CI should be fixed.

tower-http/src/auth/require_authorization.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization.rs Outdated Show resolved Hide resolved
tower-http/src/auth/require_authorization.rs Outdated Show resolved Hide resolved
@82marbag
Address comments
8b06841 
Signed-off-by: Daniele Ahmed <ahmeddan@amazon.de>
@82marbag
Merge branch 'master' into require-auth-new
c9e4b4a
davidpdrsn
davidpdrsn approved these changes Dec 2, 2022
View reviewed changes
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Thanks!

I'm currently combing through PRs and issues to figure what goes into the next breaking release but this'll be part of that for sure.

@davidpdrsn davidpdrsn added this to the 0.4 milestone Dec 2, 2022
@davidpdrsn davidpdrsn merged commit 55d3a39 into tower-rs:master Dec 2, 2022
okjodom added a commit to fedimint/fedimint that referenced this pull request Jul 31, 2023
@okjodom
fix: migrate gateway auth after tower version bump
370ac49 
- tower-http v0.4 fully replaces `RequireAuthorization` layer with
  `ValidateRequest`. We need to migrate our dependency accordingly

  - See v0.4 release notes: https://github.com/tower-rs/tower-http/releases/tag/tower-http-0.4.0
  - See the relevant PR: tower-rs/tower-http#290
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidpdrsn davidpdrsn davidpdrsn approved these changes

Assignees
No one assigned
Labels
breaking change A PR that makes a breaking change.
Projects
None yet
Milestone
0.4
Development

Successfully merging this pull request may close these issues.
2 participants
@82marbag @davidpdrsn