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

fix/jwtSync #71

Closed
wants to merge 7 commits into from
Closed

fix/jwtSync #71

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c53bf2
fix/jwtSync
bguedel Aug 17, 2021
2325c1b
Merge branch 'master' of github.com:spotify/github-java-client into p…
bguedel Nov 9, 2021
a20d753
add supplier for jwt issued at
bguedel Nov 9, 2021
ce5f22d
default supplier to remove 60 seconds
bguedel Nov 16, 2021
dddb3bd
add duration testing
bguedel Nov 18, 2021
ba2028b
Merge branch 'spotify:master' into patch-1
bguedel Nov 18, 2021
ee71b38
force only 10 mintues for jwt
bguedel Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/com/spotify/github/v3/clients/JwtTokenIssuer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public class JwtTokenIssuer {

private static final SignatureAlgorithm SIGNATURE_ALGORITHM = SignatureAlgorithm.RS256;
private static final long TOKEN_TTL = 600000;
private static final long TOKEN_ISSUED = 6000;

private final PrivateKey signingKey;

Expand Down Expand Up @@ -74,7 +75,7 @@ public String getToken(final Integer appId) {
.setIssuer(String.valueOf(appId))
.signWith(signingKey, SIGNATURE_ALGORITHM)
.setExpiration(new Date(System.currentTimeMillis() + TOKEN_TTL))
.setIssuedAt(new Date())
.setIssuedAt(new Date(System.currentTimeMillis() - TOKEN_ISSUED))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a test for this?

If the current time is provided by a Supplier<long> then this could be more easily tested. Perhaps two constructors, one where this supplier is just () -> System.currentTimeMillis() and another ctor w @VisibleForTesting where tests can provide the supplier in order to get known values.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Don't feel strongly about it, apart from keeping them checks happy)

Copy link
Author

Choose a reason for hiding this comment

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

not really sure what i am testing here?
that the setter accepted my new time? or that the time was in fact 60 seconds before now?

I can for sure look into it though if you think a test is for sure needed.

Copy link
Author

@bguedel bguedel Oct 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I can for sure understand getting another constructor in here so that people that dont have drift or care about it can ignore it.

.compact();
}
}