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

Authentication breaks for "child" clients #612

Open
marcusirgens opened this issue Mar 19, 2024 · 1 comment
Open

Authentication breaks for "child" clients #612

marcusirgens opened this issue Mar 19, 2024 · 1 comment

Comments

@marcusirgens
Copy link
Contributor

marcusirgens commented Mar 19, 2024

Looks like authentication still breaks for "child" clients, such as when making an installation client, even after #602.

I believe the code in question, also from #562 (3ce474a), is here:

octocrab/src/lib.rs

Lines 1540 to 1551 in 344cfa7

if let Some(mut auth_header) = auth_header {
// Only set the auth_header if the authority (host) is empty (destined for
// GitHub). Otherwise, leave it off as we could have been redirected
// away from GitHub (via follow_location_to_data()), and we don't
// want to give our credentials to third-party services.
if parts.uri.authority().is_none() {
auth_header.set_sensitive(true);
parts
.headers
.insert(http::header::AUTHORIZATION, auth_header);
}
}

From what I can tell, it doesn't look like the authentication layer (from the middleware module) is modified when the client is cloned:

octocrab/src/lib.rs

Lines 977 to 991 in 344cfa7

pub fn installation(&self, id: InstallationId) -> Octocrab {
let app_auth = if let AuthState::App(ref app_auth) = self.auth_state {
app_auth.clone()
} else {
panic!("Github App authorization is required to target an installation");
};
Octocrab {
client: self.client.clone(),
auth_state: AuthState::Installation {
app: app_auth,
installation: id,
token: CachedToken::default(),
},
}
}

How would you like to see this solved, @XAMPPRocky? Is there a specific reason for why the auth logic is duplicated, or is it because it's hard to change the client, once constructed?

@XAMPPRocky
Copy link
Owner

Thank you for your issue! I don't think there's a specific reason.

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

No branches or pull requests

2 participants