Deal with various auth token types #247

tgeoghegan · 2023-06-29T22:45:44Z

As of 1, the Janus aggregator API is explicit about which kind of authentication token it uses (Bearer or DapAuth) and also generates bearer tokens by default. This PR adopts changes to the aggregator API message definitions.

jbr

lgtm, one nit (typ)

jbr · 2023-06-29T22:49:03Z

src/aggregator_api_mock.rs

Not related to this PR: This file is dead code and should have been removed in #246. No need to modify it in this PR, but it also doesn't hurt anything

Yeah, I was wondering about that when I had to fix up both this file and src/api_mocks/aggregator_api.rs! Let's nuke the file in a separate PR.

jbr · 2023-06-29T22:50:51Z

src/clients/aggregator_client/api_types.rs

+pub struct AuthenticationToken {
+    /// Type of the authentication token. Always "Bearer" in divviup-api.
+    #[serde(rename = "type")]
+    typ: String,


Nit:

Suggested change

typ: String,

r#type: String,

avoids having to rename (and the awkward typ)

If you don't like that, maybe call it token_type in rust and rename to "type" in serde?

Oh, cool, I didn't know about this bit of syntax. I'm happy to use r#type in this project if you like it better.

jbr · 2023-06-29T22:53:30Z

src/routes/tasks.rs

@@ -102,6 +103,8 @@ pub mod collector_auth_tokens {
        let leader = task.leader_aggregator(&db).await?;
        let client = leader.client(client);
        let task_response = client.get_task(&task.id).await?;
-        Ok(Json([task_response.collector_auth_token]))
+        Ok(Json([task_response


I'm surprised this is what rustfmt likes but who am I to second-guess the linter

jbr · 2023-06-29T22:58:15Z

Not necessarily in this PR, but should we have code that checks the token type and errors if it's not Bearer? Does our api specification allow for anything other than Bearer? If not, why not just call the fields aggregator_bearer_token and collector_bearer_token and avoid the complexity?

tgeoghegan · 2023-06-29T23:03:52Z

Not necessarily in this PR, but should we have code that checks the token type and errors if it's not Bearer? Does our api specification allow for anything other than Bearer? If not, why not just call the fields aggregator_bearer_token and collector_bearer_token and avoid the complexity?

In the future, we may need to support partner aggregators that implement all sorts of different authentication schemes that aren't necessarily bearer tokens. For example, Daphne currently only supports the Dap-Auth-Token form. So formulating the API this way is my meager attempt at future-proofing, though divviup-api would need more work to support that sort of thing. For example, the representation of a partner aggregator would have to indicate what authentication mode it uses.

jbr · 2023-06-29T23:08:21Z

In that case it seems like we probably should error out if we encounter a token type that's not Bearer? #248

tgeoghegan · 2023-06-29T23:29:20Z

Let's hold off on merging this until the corresponding Janus change lands. I'll take care of merging when appropriate (and rebase as needed).

divergentdave · 2023-06-30T17:00:37Z

src/clients/aggregator_client/api_types.rs

@@ -101,12 +101,33 @@ impl From<Option<i64>> for QueryType {
    }
 }

+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
+pub struct AuthenticationToken {


Note that we use a Serde struct for this object here, and an internally-tagged enum in Janus. In follow-up work, I think it might make sense to treat the authentication token as an opaque JSON blob.

Oh! In thinking this through I realized we never need to use these tokens in this repo, only pass them between servers? In that case I can close #248 and I agree with this comment. Unless divviup-api needs to be the owner of acceptable tokens, we could just pass around a serde_json::Value

That would work for the aggregator auth tokens, which are opaque to divviup-api, but not necessarily for collector auth tokens. The tension here is that the Janus aggregator API can express multiple token types, hence the { "type": <>, "token": <> } structure, but divviup-api represents collector auth tokens as a string which implicitly should be used as Bearer tokens. So when providing collector auth tokens to subscribers, divviup-api needs to extract the token field of the AuthenticationToken structure.

We could make this less awkward but using the AuthenticationToken representation in the API divviup-api offers to its clients.

jbr · 2023-07-28T00:37:57Z

Do we need to validate anything about the bearer token here? We are going to check if it works, which matters more than the format

jbr · 2023-07-28T01:53:25Z

client/src/aggregator.rs

+
+#[derive(Serialize, Deserialize, Debug, Clone, Eq, PartialEq)]
+pub struct CollectorAuthenticationToken {
+    pub r#type: String,


Is this currently always expected to be Bearer? Could this be an internally-tagged enum?

tgeoghegan · 2023-07-28T17:09:10Z

Do we need to validate anything about the bearer token here? We are going to check if it works, which matters more than the format

I think that the aggregator capability/config query you're wiring up in divviup/janus#1646 / #340 is sufficient validation that the token is well formed and valid.

src/entity/aggregator/new_aggregator.rs

src/entity/aggregator/update_aggregator.rs

jbr · 2023-08-01T00:15:57Z

migration/src/m20230731_225021_base64url_unpadded_agg_api_tokens.rs

Are we at a point where we need to do this? The approach taken by #302 is just to delete all aggregators. If we need to do this, we probably also should do so in #302

If we're planning to blow away some database tables anyway, then yeah, let's skip the migrations. Let's talk tomorrow (Tuesday) about a deployment plan since we have a bunch of disruptive changes going out this week.

Based on discussion in Slack today, our plan is to delete all existing aggregators from the staging deploy of divviup-api before we deploy this change and #302, and then re-create any aggregators as needed, eliminating the need to migrate existing data. Accordingly, I've rolled back the commit that added the DB migration (and rebased for good measure).

As of [1], the Janus aggregator API is explicit about which kind of authentication token it uses (`Bearer` or `DapAuth`) and also generates bearer tokens by default. This PR adopts changes to the aggregator API message definitions. [1]: divviup/janus#1548

tgeoghegan requested review from a team and jbr as code owners June 29, 2023 22:45

tgeoghegan mentioned this pull request Jun 29, 2023

aggregator API: default to bearer tokens divviup/janus#1548

Merged

jbr approved these changes Jun 29, 2023

View reviewed changes

jbr reviewed Jun 29, 2023

View reviewed changes

divergentdave reviewed Jun 30, 2023

View reviewed changes

tgeoghegan force-pushed the timg/agg-api-bearer-token branch from 427140c to a9c14f8 Compare July 27, 2023 23:17

jbr reviewed Jul 28, 2023

View reviewed changes

tgeoghegan force-pushed the timg/agg-api-bearer-token branch from d7bdc5d to 050bee6 Compare July 31, 2023 19:12

jbr reviewed Jul 31, 2023

View reviewed changes

src/entity/aggregator/new_aggregator.rs Outdated Show resolved Hide resolved

jbr reviewed Jul 31, 2023

View reviewed changes

src/entity/aggregator/update_aggregator.rs Outdated Show resolved Hide resolved

tgeoghegan force-pushed the timg/agg-api-bearer-token branch from 050bee6 to 69df990 Compare August 1, 2023 00:05

jbr reviewed Aug 1, 2023

View reviewed changes

tgeoghegan force-pushed the timg/agg-api-bearer-token branch 2 times, most recently from 9b7d350 to d86536f Compare August 1, 2023 17:37

tgeoghegan force-pushed the timg/agg-api-bearer-token branch from d86536f to a494af8 Compare August 1, 2023 17:40

tgeoghegan merged commit fa6950e into main Aug 1, 2023
6 checks passed

tgeoghegan deleted the timg/agg-api-bearer-token branch August 1, 2023 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deal with various auth token types #247

Deal with various auth token types #247

tgeoghegan commented Jun 29, 2023

jbr left a comment

jbr Jun 29, 2023

tgeoghegan Jun 29, 2023

jbr Jun 29, 2023

tgeoghegan Jun 29, 2023

jbr Jun 29, 2023

jbr commented Jun 29, 2023

tgeoghegan commented Jun 29, 2023

jbr commented Jun 29, 2023 •

edited

Loading

tgeoghegan commented Jun 29, 2023

divergentdave Jun 30, 2023

jbr Jun 30, 2023

tgeoghegan Jun 30, 2023

jbr commented Jul 28, 2023

jbr Jul 28, 2023

tgeoghegan commented Jul 28, 2023

jbr Aug 1, 2023

tgeoghegan Aug 1, 2023

tgeoghegan Aug 1, 2023

Deal with various auth token types #247

Deal with various auth token types #247

Conversation

tgeoghegan commented Jun 29, 2023

jbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbr commented Jun 29, 2023

tgeoghegan commented Jun 29, 2023

jbr commented Jun 29, 2023 • edited Loading

tgeoghegan commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbr commented Jul 28, 2023

Choose a reason for hiding this comment

tgeoghegan commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbr commented Jun 29, 2023 •

edited

Loading