Skip to content

Fix personal token auth for pagination#602

Merged
XAMPPRocky merged 1 commit intoXAMPPRocky:mainfrom
pnevyk:fix-token-auth-next-page
Mar 19, 2024
Merged

Fix personal token auth for pagination#602
XAMPPRocky merged 1 commit intoXAMPPRocky:mainfrom
pnevyk:fix-token-auth-next-page

Conversation

@pnevyk
Copy link
Contributor

@pnevyk pnevyk commented Mar 10, 2024

Previously the AuthHeaderLayer added the auth header only when the authority was empty (i.e., base URI for GitHub was supposed to be used). However, the page URIs (e.g., next page) returned by GitHub API contains the absolute URL including the hostname. Because of this, the following code didn't work for resources that needed authentication (e.g., private repos)

use octocrab::Octocrab;

#[tokio::main]
async fn main() -> octocrab::Result<()> {
    let token = std::env::var("GITHUB_TOKEN").expect("GITHUB_TOKEN env variable is required");

    let octocrab = Octocrab::builder().personal_token(token).build()?;

    let page = octocrab
        .repos("owner", "private-repo")
        .list_commits()
        .send()
        .await?;

    octocrab
        .get_page::<octocrab::models::repos::RepoCommit>(&page.next)
        .await?;

    Ok(())
}

This fix adds the base URI to the AuthHeaderLayer so that it can check whether a request is destined for GitHub (either the hostname is empty or it is equal to the base URI).

Previously the AuthHeaderLayer added the auth header only when the
authority was empty (i.e., base URI for GitHub was supposed to be used).
However, the page URIs (e.g., next page) returned by GitHub API contains
the absolute URL including the hostname. Because of this, the following
code didn't work for resources that needed authentication (e.g., private
repos)

```
let page = /* get a page of something using octocrab */

// This fails with NOT_FOUND
octocrab.get_page(&page.next).await;
```

This fix adds the base URI to the AuthHeaderLayer so that it can check
whether a request is destined for GitHub (either the hostname is empty or
it is equal to the base URI).
@marcusirgens
Copy link
Contributor

This also breaks paginators for installations and apps, would be great to get this merged.

@XAMPPRocky XAMPPRocky merged commit 8d9d065 into XAMPPRocky:main Mar 19, 2024
@XAMPPRocky
Copy link
Owner

Thank you for your PR, and congrats on your first contribution! 🎉

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.

3 participants