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

Refactor device auth schema #1344

Merged
merged 10 commits into from
Jul 7, 2022
Merged

Refactor device auth schema #1344

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a9e20c5
Refactor device auth schema
plotnick Jul 1, 2022
a195c9e
Trim superfluous policy resource, comment lack of authz on device_acc…
plotnick Jul 5, 2022
0af0e27
Store request time in device_access_token records
plotnick Jul 6, 2022
027d517
Delete device auth request and insert access token in transaction
plotnick Jul 6, 2022
c4f4d01
fmt
plotnick Jul 6, 2022
77a0d8c
Refine error handling in device_access_token_actor
plotnick Jul 6, 2022
2b504ca
Allow for low-resolution clocks
plotnick Jul 6, 2022
152fd2a
Handle concurrent token grants in transaction
plotnick Jul 6, 2022
bf53d38
Even more refined error handling in device_access_token_create
plotnick Jul 6, 2022
8ce7e42
Refine error handling harder
plotnick Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions nexus/src/db/datastore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4323,14 +4323,34 @@ impl DataStore {
.values(access_token)
.returning(DeviceAccessToken::as_returning());

#[derive(Debug)]
enum TokenGrantError {
ConcurrentRequests,
}
type TxnError = TransactionError<TokenGrantError>;

self.pool_authorized(opctx)
.await?
.transaction(move |conn| {
davepacheco marked this conversation as resolved.
Show resolved Hide resolved
delete_request.execute(conn)?;
Ok(insert_token.get_result(conn)?)
if delete_request.execute(conn)? == 1 {
Ok(insert_token.get_result(conn)?)
} else {
Err(TxnError::CustomError(
TokenGrantError::ConcurrentRequests,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is okay. I would probably have this say:

0 rows back => 404 -- there's no reason to expose this as "concurrent update" -- it just doesn't exist now
more than 1 row back => 500

If you want to do that, vpc_update_firewall_rules() might be an example to follow? You basically need to return something from the transaction that's specific enough to match on it in the map_err() below to decide whether to use Error::internal_error or authz_resource.not_found().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, sorry. Trying again in bf53d38.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, this looks better. The case I'm still not sure about is when we find more than one item. I'd expect this to be a 500 (with a message like "unexpectedly found multiple device auth requests for the same user code") because I don't see how it could ever actually happen. The current code produces a 400 that says something about concurrent requests. (When we were talking about "concurrent requests" earlier, I think I meant concurrent verify requests, which is what could land you in the other case (which is now a 404, which is good). I'm not sure how concurrent requests could land you in this case...unless by "concurrent requests" you mean "we managed to have multiple [concurrent] device auth requests with the same user code", which is true, but that still seems like a server error.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct on all accounts, thank you. Hopefully last try: 8ce7e42.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks great!

))
}
})
.await
.map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
.map_err(|e| match e {
TxnError::CustomError(TokenGrantError::ConcurrentRequests) => {
Error::invalid_request(
"token grant failed due to concurrent requests",
)
}
TxnError::Pool(e) => {
public_error_from_diesel_pool(e, ErrorHandler::Server)
}
})
}

/// Look up a granted device access token.
Expand Down
2 changes: 1 addition & 1 deletion nexus/src/db/model/device_auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl DeviceAccessToken {
silo_user_id: Uuid,
) -> Self {
let now = Utc::now();
assert!(time_requested < now);
assert!(time_requested <= now);
Self {
token: generate_token(),
client_id,
Expand Down