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.

Sign up for GitHub

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

Jump to bottom

Add expiration argument back to GithubIntegration.create_jwt #2439

Merged
merged 2 commits into from Mar 17, 2023
Merged

Add expiration argument back to GithubIntegration.create_jwt #2439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f704fe
Add expiration argument back to create_jwt
EnricoMi Feb 27, 2023
57e8977
Fix lint
EnricoMi Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions github/GithubIntegration.py
View file
Open in desktop
Expand Up @@ -90,17 +90,23 @@ def _get_installed_app(self, url):
completed=True,
)

def create_jwt(self):
def create_jwt(self, expiration=None):
"""
Create a signed JWT
https://docs.github.com/en/developers/apps/building-github-apps/authenticating-with-github-apps#authenticating-as-a-github-app

:return string:
"""
if expiration is not None:
assert isinstance(expiration, int), expiration
assert (
Consts.MIN_JWT_EXPIRY <= expiration <= Consts.MAX_JWT_EXPIRY
), expiration

now = int(time.time())
payload = {
"iat": now + self.jwt_issued_at,
"exp": now + self.jwt_expiry,
"exp": now + (expiration if expiration is not None else self.jwt_expiry),
"iss": self.integration_id,
}
encrypted = jwt.encode(payload, key=self.private_key, algorithm="RS256")
Expand Down
2 changes: 1 addition & 1 deletion github/GithubIntegration.pyi
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ class GithubIntegration:
) -> None: ...
def _get_installed_app(self, url: str) -> Installation: ...
def _get_headers(self) -> Dict[str, str]: ...
def create_jwt(self, expiration: int = ...) -> str: ...
def create_jwt(self, expiration: Optional[int] = ...) -> str: ...
def get_access_token(
self, installation_id: int, permissions: Optional[Dict[str, str]] = ...
) -> InstallationAuthorization: ...
Expand Down
21 changes: 21 additions & 0 deletions tests/GithubIntegration.py
View file
Open in desktop
Expand Up @@ -61,6 +61,27 @@ def testCreateJWT(self):
)
sys.modules["time"].time = self.origin_time

def testCreateJWTWithExpiration(self):
self.origin_time = sys.modules["time"].time
sys.modules["time"].time = lambda: 1550055331.7435968
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels awkward. Why not use a mock here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like it either, just copied from existing testCreateJWT above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we should fix both places, and I'd rather circle back and fix one, rather than both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is meant to be a quick fix, to get a fix for 1.58.0 out, asap. People are rightly annoyed: #2430 (comment)

Cleaning up test code can be done for 1.59.0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have time to cut a release right now, but I'll get this merged.

github_integration = github.GithubIntegration(
integration_id=APP_ID,
private_key=PRIVATE_KEY,
jwt_expiry=120,
jwt_issued_at=-30,
)
token = github_integration.create_jwt(60)
payload = jwt.decode(
token,
key=PUBLIC_KEY,
algorithms=["RS256"],
options={"verify_exp": False},
)
self.assertDictEqual(
payload, {"iat": 1550055301, "exp": 1550055391, "iss": APP_ID}
)
sys.modules["time"].time = self.origin_time

def testGetInstallations(self):
github_integration = github.GithubIntegration(
integration_id=APP_ID, private_key=PRIVATE_KEY
Expand Down