Skip to content

Conversation

itssimple
Copy link
Contributor

@itssimple itssimple commented May 28, 2021

GitHub Enterprise has deprecated using access_token only in querystring.

Response from newer versions:

{
    "message": "Must specify access token via Authorization header",
    "documentation_url": "https://docs.github.com/enterprise/3.0/v3/#oauth2-token-sent-in-a-header"
}

…tallations-request

GitHub Enterprise has deprecated using `access_token` only in querystring.

Response from newer versions:
```json
{
    "message": "Must specify access token via Authorization header",
    "documentation_url": "https://docs.github.com/enterprise/3.0/v3/#oauth2-token-sent-in-a-header"
}
```
@itssimple itssimple requested a review from a team May 28, 2021 14:34
@itssimple itssimple changed the title Adding Authorization-header for installations-request fix(GithubEnterpriseIntegration): Adding Authorization-header for installations-request May 29, 2021
@BYK
Copy link
Member

BYK commented May 30, 2021

/gcbrun

@BYK
Copy link
Member

BYK commented May 30, 2021

Thanks a lot @itssimple! I think this was overlooked when fixing the main GitHub auth a while ago.

Pinging the @getsentry/ecosystem team for a review and merge.

Copy link
Member

@MeredithAnya MeredithAnya left a comment

Choose a reason for hiding this comment

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

@itssimple Thank you putting up this PR !

Just a couple of things:

I would recommend adding match_querystring=True to the responses made to the /user endpoints in the GHE tests to just verify that we are making requests without the access token in the query param. (even though we are mocking out the response)

Here is one of the requests, the other is some lines above:

responses.add(
responses.GET,
self.base_url + "/user/installations",
json={"installations": [{"id": installation_id}]},
)

Additionally, can you tell me which version of GHE you tested this out on?


resp = session.get(
"https://{}/api/v3/user/installations".format(installation_data["url"]),
params={"access_token": access_token},
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the params here since you are adding the access_token to the headers now

@itssimple
Copy link
Contributor Author

@MeredithAnya Thanks for checking the PR!

I have committed the changes requested by adding the match_querystring=True to the test, and removing the params from the actual request.

I'm testing this against a 3.0.7 version of GHE.

@BYK
Copy link
Member

BYK commented Jun 1, 2021

/gcbrun

@MeredithAnya
Copy link
Member

@MeredithAnya Thanks for checking the PR!

I have committed the changes requested by adding the match_querystring=True to the test, and removing the params from the actual request.

I'm testing this against a 3.0.7 version of GHE.

Thanks for removing the params ! The match_querystring=True is a new arg, not part of the json so i think needs to be like the following

responses.add(
    responses.GET,
    self.base_url + "/user/installations",
    json={"installations": [{"id": installation_id}]},
    match_querystring=True,
) 

after that I think things should be good

@itssimple
Copy link
Contributor Author

Ah, sorry. It's my first time modifying Python. :)

@BYK
Copy link
Member

BYK commented Jun 1, 2021

/gcbrun

Co-authored-by: MeredithAnya <meredith.a.heller@gmail.com>
@BYK
Copy link
Member

BYK commented Jun 1, 2021

/gcbrun

@BYK
Copy link
Member

BYK commented Jun 1, 2021

/gcbrun

@BYK
Copy link
Member

BYK commented Jun 1, 2021

/gcbrun

@MeredithAnya
Copy link
Member

we got this team !

fix lint once and for all

@BYK BYK enabled auto-merge (squash) June 1, 2021 22:01
@BYK BYK merged commit dbda812 into getsentry:master Jun 1, 2021
@itssimple itssimple deleted the patch-1 branch June 2, 2021 05:13
@github-actions github-actions bot locked and limited conversation to collaborators Jun 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants