Skip to content

Commit

Permalink
fix(#274): use the validated token to calculate flags to return (#275)
Browse files Browse the repository at this point in the history
* fix(#274): use the validated token to calculate flags to return

This change fixes a bug where the client API would return all flags
that existed in the cache, even if the api token did not have access
to those flags. Crucially, the API token had to have access to
multiple (but not all) projects for this to happen.

The root cause is that we used the incoming edge token to check which
flags to return. Before it gets validated, its `projects` property is
just an empty list. In the filtering, this causes edge to return all
available. Features.

The solution was to instead use the validated edge token that we
create further up.

There is also a test that confirms this behavior is what we expect.

## Discussion point

Could we make it so that the `with_filter` function can only take
validated edge tokens or would that break something else? Might be a
good way to future proof it.

* fix(#274): update more uses

I suspect these will have the same issue

* Update server/src/client_api.rs

Co-authored-by: Simon Hornby <[email protected]>

---------

Co-authored-by: Simon Hornby <[email protected]>
  • Loading branch information
thomasheartman and sighphyre authored Sep 27, 2023
1 parent a1183b0 commit 00661c4
Showing 1 changed file with 56 additions and 4 deletions.
60 changes: 56 additions & 4 deletions server/src/client_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ async fn resolve_features(
} else {
FeatureFilterSet::default()
}
.with_filter(project_filter(&edge_token));
.with_filter(project_filter(&validated_token));

let client_features = match req.app_data::<Data<FeatureRefresher>>() {
Some(refresher) => {
Expand All @@ -92,7 +92,7 @@ async fn resolve_features(
.await
}
None => features_cache
.get(&cache_key(&edge_token))
.get(&cache_key(&validated_token))
.map(|client_features| filter_client_features(&client_features, filter_set))
.ok_or(EdgeError::ClientFeaturesFetchError(FeatureError::Retriable)),
}?;
Expand Down Expand Up @@ -129,7 +129,7 @@ pub async fn get_feature(
.ok_or(EdgeError::AuthorizationDenied)?;

let filter_set = FeatureFilterSet::from(Box::new(name_match_filter(feature_name.clone())))
.with_filter(project_filter(&edge_token));
.with_filter(project_filter(&validated_token));

match req.app_data::<Data<FeatureRefresher>>() {
Some(refresher) => {
Expand All @@ -138,7 +138,7 @@ pub async fn get_feature(
.await
}
None => features_cache
.get(&cache_key(&edge_token))
.get(&cache_key(&validated_token))
.map(|client_features| filter_client_features(&client_features, filter_set))
.ok_or(EdgeError::ClientFeaturesFetchError(FeatureError::Retriable)),
}
Expand Down Expand Up @@ -617,6 +617,58 @@ mod tests {
.all(|f| token.projects.contains(&f.project.clone().unwrap())));
}

#[tokio::test]
async fn client_features_endpoint_filters_correctly_when_token_has_access_to_multiple_projects()
{
let features_cache: Arc<DashMap<String, ClientFeatures>> = Arc::new(DashMap::default());
let token_cache: Arc<DashMap<String, EdgeToken>> = Arc::new(DashMap::default());
let app = test::init_service(
App::new()
.app_data(Data::from(features_cache.clone()))
.app_data(Data::from(token_cache.clone()))
.service(web::scope("/api/client").service(get_features)),
)
.await;

let mut token_a =
EdgeToken::try_from("[]:production.puff_the_magic_dragon".to_string()).unwrap();
token_a.projects = vec!["dx".into(), "eg".into()];
token_a.status = TokenValidationStatus::Validated;
token_a.token_type = Some(TokenType::Client);
token_cache.insert(token_a.token.clone(), token_a.clone());

let mut token_b =
EdgeToken::try_from("[]:production.biff_the_magic_flagon".to_string()).unwrap();
token_b.projects = vec!["unleash-cloud".into()];
token_b.status = TokenValidationStatus::Validated;
token_b.token_type = Some(TokenType::Client);
token_cache.insert(token_b.token.clone(), token_b.clone());

let example_features = features_from_disk("../examples/hostedexample.json");
features_cache.insert("production".into(), example_features.clone());

let req_1 = make_features_request_with_token(token_a.clone()).await;
let res_1: ClientFeatures = test::call_and_read_body_json(&app, req_1).await;
assert!(res_1
.features
.iter()
.all(|f| token_a.projects.contains(&f.project.clone().unwrap())));

let req_2 = make_features_request_with_token(token_b.clone()).await;
let res_2: ClientFeatures = test::call_and_read_body_json(&app, req_2).await;
assert!(res_2
.features
.iter()
.all(|f| token_b.projects.contains(&f.project.clone().unwrap())));

let req_3 = make_features_request_with_token(token_a.clone()).await;
let res_3: ClientFeatures = test::call_and_read_body_json(&app, req_3).await;
assert!(res_3
.features
.iter()
.all(|f| token_a.projects.contains(&f.project.clone().unwrap())));
}

#[tokio::test]
async fn when_running_in_offline_mode_with_proxy_key_should_not_filter_features() {
let features_cache: Arc<DashMap<String, ClientFeatures>> = Arc::new(DashMap::default());
Expand Down

0 comments on commit 00661c4

Please sign in to comment.