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

feature request: provide methods for updating authentication tokens or automatically update the tokens for long-live clients #45

Open
arkbriar opened this issue Aug 12, 2022 · 8 comments
Labels
help wanted Extra attention is needed

Comments

@arkbriar
Copy link

Currently, there's no such interface for updating the expired token held by a Client, which leads to a problem when using a long-live client. We would always fail after the token TTL and have to create another Client in case of authenticate errors. It's not easy for shared services, especially in Rust.

So I wonder if we can provide a method for updating the authentication token of some Client or just updating it automatically inside the client when an authentication error's encountered.

I would prefer the second solution. I think the AuthService::call would be a good location for this. How does this sound to you?

Looking forward to your comments.

@davidli2010
Copy link
Contributor

In principle, this is possible because the token is inserted into the header of the HTTP request every time a request is sent, so replacing the token is easy.

The key issue is how to get the new token? For security reason, we do not keep password in the client. It may be possible to provide a method for re-authentication, where the user provides the password.

In fact, when re-authentication occurs, all service clients need to be recreated inside the client, except for channel, which can be reused. Therefore, I thinkt there is no obvious advantage in re-authentication over creating a new client.

I recommend that you encapsulate the client in your business code, add an method for re-authentication, and use the Deref pattern to eliminate redundant implementations. The code looks like this:

struct MyClient {
    inner: Client,
    // other fields
}

impl Deref for MyClient {
    type Target = Client;

    #[inline]
    fn deref(&self) -> &Self::Target {
        &self.inner
    }
}

impl DerefMut for MyClient {
    #[inline]
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.inner
    }
}

impl MyClient {
    pub fn reauthenticate(&mut self, ...) -> Result<(), MyError> {
        self.inner = Client::connect(...)?;
        Ok(())
    }
}

@arkbriar
Copy link
Author

@davidli2010 It's not possible to achieve the updating without being aware of the response. With your recommendation, we need to check the response for each invocation (and for each kind of request). I prefer a proxy to do that. But I think it's not an ideal way either.

An elegant way should be intercepting the response at the channel level instead of the service level. And it's so implemented in the Go's client, and you can check it here.

@arkbriar
Copy link
Author

IMHO, it doesn't make sense that a client only lives for a while and has no chance to be re-used.

@davidli2010
Copy link
Contributor

davidli2010 commented Aug 16, 2022

I think I understand what you are saying. But I don't know how to refresh the token. Anyway, if you have an idea, you can create a PR and I would like to review it.

@arkbriar
Copy link
Author

After some investigation, I discovered that it's hard to implement a retry logic over the tonic gRPC. It's caused by one of the dependencies of tonic http, which doesn't support the Clone trait on Request. Thus we can not simply clone the request and retry the whole logic. Here's the issue from tonic's repo.

There're workarounds to clone the Request, but they commonly serialize the request and then deserialize from the bytes. It's quite heavy and thus not practical.

I'm getting stuck and can not make any progress either.😅 What a sad story.

@davidli2010
Copy link
Contributor

davidli2010 commented Aug 17, 2022

In my opinion, we should avoid cloning(memory copy) request every time.

@arkbriar
Copy link
Author

In my opinion, we should avoid cloning(memory copy) request every time.

Yeah, I agree. But the problem is we have to clone it before retrying the request due to the constraints required by the GrpcService from tonic, which consumes the request and doesn't give a chance to reuse it. A possible solution for tonic is to allow for passing a reference. Again the serializer introduces other constraints. Currently, it's just a mess.

@davidli2010
Copy link
Contributor

@arkbriar please see #65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants