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

imap::client::Client::authenticate: Base64 encode the result of the A… #97

Merged
merged 6 commits into from
Nov 10, 2018

Conversation

kmkaplan
Copy link

@kmkaplan kmkaplan commented Nov 7, 2018

Fixes issue #95.

Please bear with me, I'm only beginning Rust.

I see two issues with this one-liner:

  1. it is not backward compatible. What's the rules for this?
  2. as Authenticator::process returns a String, it is not possible to submit arbitrary binary data. For fixing this process would have to return a Vec<u8>. What do you think?

@jonhoo
Copy link
Collaborator

jonhoo commented Nov 7, 2018

There are already a number of breaking changes pending that are waiting on #69, so I'm not too concerned about 1. As for 2, that's a good question... In some sense, what we want is something that returns impl AsRef<[u8]> (which could be both Vec<u8> and &str). Can you try that type and change the .as_str() to .as_ref()?

@jonhoo
Copy link
Collaborator

jonhoo commented Nov 7, 2018

Hmm, actually, no, that won't work. The way to actually do this is to change the Authenticator trait to be this:

pub trait Authenticator {
    type Response: AsRef<[u8]>;

    fn process(&self, challenge: &str) -> Self::Response;
}

Should we make challenge a &[u8] too? I don't know what the spec says here?

@kmkaplan
Copy link
Author

kmkaplan commented Nov 7, 2018

IMAP 4rev1 says:

A server challenge consists of a command continuation request response with the "+" token followed by a BASE64 encoded string.

So I guess we should also change the challenge to &[u8].

Tasks for next pull request:

  1. change Authenticator to accept and return AsRef([u8]),
  2. update examples/gmail_oauth2.rs to use the new Authenticator.

@jonhoo
Copy link
Collaborator

jonhoo commented Nov 7, 2018

Yeah, I think that means we should base64 decode the server challenge before passing it to the Authenticator, which suggests that we should have challenge: &[u8]. You should just make those changes to this PR instead of opening another one. You can make more modifications by pushing more commits to the branch you already have open :)

@jonhoo
Copy link
Collaborator

jonhoo commented Nov 10, 2018

Awesome, I think we're nearly there! I'd like to request two more relatively straightforward changes:

  • Change challenge from Vec<u8> to &[u8]
  • Add a short documentation string to the associated type and method of Authenticate to mention that base64 encode/decode happens automatically :) Maybe also a line to the trait itself giving a reference to the relevant part of the IMAP spec for auth?

@jonhoo
Copy link
Collaborator

jonhoo commented Nov 10, 2018

Beautiful, thank you!

@jonhoo jonhoo merged commit 28e4201 into mattnenterprise:master Nov 10, 2018
@kmkaplan kmkaplan deleted the issue-95-authenticate-base64 branch November 10, 2018 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants