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.

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

Jw/changing how jwt is passed around #1130

Merged

Conversation

shopifyski
Copy link
Contributor

@shopifyski shopifyski commented Mar 4, 2024

Standardized on a structure to pass around auth data instead of expecting downstream code to parse headers
pub struct UserAuthContext { pub scheme: Option<String>, pub token: Option<String>, }

an immediate follow up cleanup should address the following challenges:

  • duplication between code in http/user/mod.rs and http/user/extract.rs
  • optionality of UserAuthContext fields - when context is required, its fields should not be optional
  • tie userauthcontext to the auth strategy. make the strategy define whether certain edge cases (e.g. lack of token) should be interpreted as errors or dismissed

@shopifyski shopifyski marked this pull request as draft March 4, 2024 18:22
Copy link
Contributor

@jeremywrowe jeremywrowe left a comment

Choose a reason for hiding this comment

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

Love the change! Let me know if you want help resolving some of the todo items. Other than that, I'd recommend moving this one to a normal PR state.

libsql-server/src/auth/parsers.rs Outdated Show resolved Hide resolved
libsql-server/src/http/user/extract.rs Outdated Show resolved Hide resolved
@shopifyski shopifyski marked this pull request as ready for review March 5, 2024 00:55
@LucioFranco
Copy link
Contributor

@shopifyski looks like the test CI failures are normal we can just re-run that job, but it looks like you need to run cargo fmt on your changes to get the checks job to pass.

Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Overall seems like a good change 👍 I left some feedback, feel free to ping me on slack if you have any questions. An additional heads up we are working on cutting a release this week so I will time the merging of this with that, likely after that just to be safe. Let me know if there are any timeline concerns and questions! Thanks!

libsql-server/src/auth/parsers.rs Outdated Show resolved Hide resolved
libsql-server/src/auth/parsers.rs Outdated Show resolved Hide resolved
libsql-server/src/auth/parsers.rs Outdated Show resolved Hide resolved
libsql-server/src/auth/user_auth_strategies/mod.rs Outdated Show resolved Hide resolved
libsql-server/src/auth/user_auth_strategies/mod.rs Outdated Show resolved Hide resolved
libsql-server/src/http/user/extract.rs Outdated Show resolved Hide resolved
libsql-server/src/http/user/extract.rs Outdated Show resolved Hide resolved
libsql-server/src/rpc/replica_proxy.rs Outdated Show resolved Hide resolved
libsql-server/src/rpc/replication_log.rs Outdated Show resolved Hide resolved
libsql-server/tests/cluster/replication.rs Outdated Show resolved Hide resolved
@LucioFranco
Copy link
Contributor

@shopifyski looks like some checks CI failures that need to be addressed! I just ran ci.

@shopifyski shopifyski requested a review from LucioFranco March 11, 2024 14:45
@shopifyski shopifyski force-pushed the jw/changing-how-jwt-is-passed-around branch from 3205cdc to 843c9f7 Compare March 13, 2024 17:24
@shopifyski shopifyski force-pushed the jw/changing-how-jwt-is-passed-around branch from 47c874b to 37dc162 Compare March 13, 2024 19:25
@shopifyski shopifyski requested a review from LucioFranco March 14, 2024 21:18
Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

.map(|v| HeaderValue::from_maybe_shared(v).expect("Should already be valid header"))
.ok_or(AuthError::AuthHeaderNotFound)
.and_then(|h| h.to_str().map_err(|_| AuthError::AuthHeaderNonAscii))
.and_then(|t| UserAuthContext::from_auth_str(t))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.and_then(|t| UserAuthContext::from_auth_str(t))
.and_then(UserAuthContext::from_auth_str)

btw if the input type is the same as the closure this works as well!

@LucioFranco LucioFranco added this pull request to the merge queue Mar 15, 2024
Merged via the queue into tursodatabase:main with commit 3b978fe Mar 15, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants