Skip to content

CosmosDB: support for changing AuthorizationToken in pipeline #339

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

Merged
merged 1 commit into from
Sep 21, 2021

Conversation

MindFlavor
Copy link
Contributor

CosmosDB crate allows to change the AuthorizationToken without having to recreate the client(s). This is useful for quickly switching security context.
The relevant code didn't account for the new pipeline architecture. This PR fixes that (so now even the permission_token_usage test case works as expected).

Fixes #337.

@MindFlavor MindFlavor added Cosmos The azure_cosmos crate bugfix labels Aug 5, 2021
@MindFlavor MindFlavor requested a review from rylev August 5, 2021 13:58
self.auth_token = auth_token.clone();

// we replace the AuthorizationPolicy. This is
// the last-1 policy by construction.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems pretty error prone... I wonder if there's a better way to do it... Perhaps something like this could be better:

  /// Set the auth token used
    pub fn auth_token(&mut self, auth_token: AuthorizationToken) {
        self.auth_token = auth_token.clone();

        let new_auth_policy: Arc<dyn azure_core::Policy<CosmosContext>> =
            Arc::new(crate::AuthorizationPolicy::new(auth_token));
        use std::any::Any;
        let auth_policy = self
            .pipeline_mut()
            .policies_mut()
            .iter_mut()
            .find(|p| (**p).type_id() == TypeId::of::<AuthorizationPolicy>())
            .unwrap(); // TODO: add auth policy if one doesn't exist?
        std::mem::replace(auth_policy, new_auth_policy);
    }

I've not tested whether this works, but I think it should.

Copy link
Contributor Author

@MindFlavor MindFlavor Aug 5, 2021

Choose a reason for hiding this comment

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

Yes it is error prone, unfortunately we forego the type safe approach so these problems are bound to happen.
In this instance, I think we should get away with the simpler code since:

  1. The only place we construct the pipeline is new_pipeline_from_options.
  2. In new_pipeline_from_options, we explicitly add the AuthorizationPolicy as last retry policy (penultimate position).

Since there is no way to construct a pipeline without an AuthorizationPolicy at the penultimate position my proposed code will (should? 🤣) always work.

On the other hand, your code is safer but it would correctly work on a buggy pipeline (AuthorizationPolicy must be the last retry policy for it to work properly) so I think it's a moot point.

What about an assert? Maybe a debug_assert that gets removed from production code?
The assert should enforce that the swapped policy is indeed an AuthorizationPolicy.

@rylev rylev merged commit b2e0df3 into Azure:main Sep 21, 2021
@MindFlavor MindFlavor deleted the issue/337/cosmos_change_auth branch October 14, 2021 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cosmos The azure_cosmos crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CosmosClient should propagate the new AuthorizationToken to the AuthorizationPolicy
2 participants