fix/jwtSync #71

bguedel · 2021-08-17T20:45:57Z

fixes #68

per example in related issue; time sync issue can occur when setting issued at to be the current time. This sets the issued at to be 1 minute earlier to avoid this.

Tested locally for 2 days with no issues.

fixes spotify#68 per example in related issue; time sync issue can occur when setting issued at to be the current time. This sets the issued at to be 1 minute earlier to avoid this. Tested locally for 2 days with no issues.

bguedel · 2021-08-18T23:00:01Z

@henriquetruta thoughts?

bguedel · 2021-08-24T15:17:39Z

@neonal Thoughts?

bguedel · 2021-09-07T21:56:45Z

@henriquetruta thoughts?

i see you merged a couple of other PRs that were opened after mine. Wondering when we can get this merged; This is a blocker for my team to use this project ;(

bguedel · 2021-09-20T15:56:23Z

@henriquetruta @neonal

any thoughts?

bguedel · 2021-09-28T21:30:08Z

@neonal?? Looks like you merged a PR a few hours ago. Wondering if you can take a look at this PR??

Or can we get some guidance on if you guys actually accept PR from external?

bguedel · 2021-10-11T22:02:36Z

@barbatron Can I get a quick look?

bguedel · 2021-10-20T14:34:59Z

@barbatron looks like you are looking at some other prs; could i get a look see? maybe a comment or 3?

barbatron

Somehow left my review pending 🤦 sorry about that!

barbatron · 2021-10-07T08:56:59Z

src/main/java/com/spotify/github/v3/clients/JwtTokenIssuer.java

@@ -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))


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?

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

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.

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

src/main/java/com/spotify/github/v3/clients/JwtTokenIssuer.java

…atch-1

added tests questionably; dont fully understand what else could be tested

bguedel · 2021-11-09T18:15:42Z

@barbatron Sorry for the delay;

Added the supplier. I defaulted the main supplier to be the same as it is today so that people that update dont see any change to their code;

Added the "60000" to the tests; but not sure what we expect to be tested here; Effectively i am testing to see if a jwt is created when using the default and using a custom supplier; If you have any other ideas on what can be tested i am open to suggestion.

Right now the output is just a string; sooooo

shaendler · 2021-11-14T19:00:56Z

@bguedel i also see this issue quite frequently in my app. using the 0.1.24 version.
Can you explain how as the library consumer i can control the isseuAt supplier? on my end, i am just creating a client

@barbatron can we urge this fix to merge?

bguedel · 2021-11-16T15:54:11Z

@shaendler good call;

github-java-client/src/main/java/com/spotify/github/v3/clients/GitHubClient.java

Line 622 in dbabfc3

jwtToken = JwtTokenIssuer.fromPrivateKey(privateKey).getToken(appId);

<--- thats where this is actually being used.

which is essentially used by a request such as

github-java-client/src/main/java/com/spotify/github/v3/clients/GitHubClient.java

Line 361 in dbabfc3

final Request request = requestBuilder(path).build();

which is used by the GithubClient --> to which you create in your app.

so this solution wont actually work; though it gives some building blocks to work.

Open to suggestion on how we can make it work. Dont love pushing this all the way up to the GithubClient factory methods.

Otherwise we go back to hardcoding 60000 (60s)???

bguedel · 2021-11-16T15:55:50Z

This is currently how we are using this (through those links)

//Spring @Configuration file
/**
     * Spotify github client.
     *
     * @return
     */
    @Bean
    public GitHubClient githubClient() throws IOException {
        byte[] decodedBytes = Base64.getDecoder().decode(gitHubProperties.getApp().getPrivateKey());
        FileUtils.writeByteArrayToFile(new File(PK_PATH), decodedBytes);

        return GitHubClient.create(
                URI.create("https://pathTo.github/api/v3"),
                FileUtils.getFile(PK_PATH),
                gitHubProperties.getApp().getAppId(),
                gitHubProperties.getApp().getInstallationId());
    }

bguedel · 2021-11-16T18:15:24Z

ill make a pr right quick to change the default behavior to -60s and then still allow someone to override this;

i dont forsee this being an issue for current systems; this will solve the issue; but still give the chance to set this directly; if later someone wants to implement a way to use this feature deeper into the system.

bguedel · 2021-11-16T18:27:51Z

src/main/java/com/spotify/github/v3/clients/JwtTokenIssuer.java

+  private static final long TOKEN_TTL = 600_000;
+  private static final long TOKEN_ISSUED = 60_000;


Fun Fact: Java totally allows _ in numbers for readability;

bguedel · 2021-11-17T21:03:40Z

built this locally again and put it in my app; getting the error again; this supplier doesnt seem to be working the way i had expected.

bguedel · 2021-11-30T15:48:53Z

so far this has worked locally; no more 401s

bguedel · 2021-12-07T14:15:43Z

@barbatron Thoughts?

bguedel · 2021-12-08T17:42:06Z

@barbatron Did you forget to submit your review again?

bguedel · 2021-12-14T22:14:53Z

@barbatron hope you are having a good december;

Before you cut out for some awesome end of year bash, could i get a comment or 4, or maybe event an approval?

dont know why my pr is not really getting built/codecov

wsguede · 2022-01-06T04:12:43Z

Happy New Year Spotify team!

Could someone take a look at this and give some feedback or accept the pr?

@barbatron @henriquetruta @neonal

bguedel · 2022-01-21T15:59:26Z

Hey Spotify team!

Just a friendly reminder; Looking to get this into master. Could I get some comments?
@barbatron @henriquetruta @neonal

barbatron · 2022-01-27T22:47:29Z

Hey there @bguedel , I'm no longer with Spotify so ~~I'm afraid~~ I guess I can't be of help. Filtered out emails from the Spotify org due to the vast amount from other FOSS projects. I'm impressed with your tenacity, though! My team at Sp had lots to do and was getting even more on the plate, and without much buy-in to maintain these peripheral repos apart from dwindling spare time, I'm afraid it could be tricky to get attention. Best of luck though. Sorry I didn't get this ashore before I left back in October.

bguedel · 2022-02-02T17:27:17Z

Hey @henriquetruta @neonal

Just wondering if you had any input on this change?

bguedel · 2022-03-10T19:36:42Z

Wondering if we can get some eyes on this?

ebk45 · 2023-12-22T10:19:51Z

Hi @bguedel.

We apologise for taking so long to get eyes on this PR, we haven't been able to maintain this library to the standard we would have liked to but this will be changing in the new year. If you could please get this rebased and we'll be prioritising all open PRs ready for review.

Thanks!

Ellie

ebk45 · 2024-02-05T15:21:05Z

Hello,

As this PR is now stale and we've not had any new activity on it, we've decided to close it so we can keep track of active/more recent changes. We will leave the branch untouched in case you would like to reopen and rebase.

Thanks!

Ellie

fix/jwtSync

2c53bf2

fixes spotify#68 per example in related issue; time sync issue can occur when setting issued at to be the current time. This sets the issued at to be 1 minute earlier to avoid this. Tested locally for 2 days with no issues.

barbatron suggested changes Oct 25, 2021

View reviewed changes

bguedel added 2 commits November 9, 2021 12:10

Merge branch 'master' of github.com:spotify/github-java-client into p…

2325c1b

…atch-1

add supplier for jwt issued at

a20d753

added tests questionably; dont fully understand what else could be tested

default supplier to remove 60 seconds

ce5f22d

bguedel commented Nov 16, 2021

View reviewed changes

bguedel added 3 commits November 18, 2021 16:17

add duration testing

dddb3bd

Merge branch 'spotify:master' into patch-1

ba2028b

force only 10 mintues for jwt

ee71b38

barbatron approved these changes Jan 27, 2022

View reviewed changes

ebk45 closed this Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix/jwtSync #71

fix/jwtSync #71

bguedel commented Aug 17, 2021

bguedel commented Aug 18, 2021

bguedel commented Aug 24, 2021

bguedel commented Sep 7, 2021

bguedel commented Sep 20, 2021

bguedel commented Sep 28, 2021

bguedel commented Oct 11, 2021

bguedel commented Oct 20, 2021

barbatron left a comment

barbatron Oct 7, 2021

barbatron Oct 25, 2021

bguedel Oct 25, 2021

bguedel Oct 25, 2021 •

edited

Loading

bguedel commented Nov 9, 2021

shaendler commented Nov 14, 2021

bguedel commented Nov 16, 2021

bguedel commented Nov 16, 2021

bguedel commented Nov 16, 2021

bguedel Nov 16, 2021

bguedel Nov 16, 2021

bguedel commented Nov 17, 2021

bguedel commented Nov 30, 2021

bguedel commented Dec 7, 2021

bguedel commented Dec 8, 2021

bguedel commented Dec 14, 2021

wsguede commented Jan 6, 2022

bguedel commented Jan 21, 2022

barbatron commented Jan 27, 2022 •

edited

Loading

bguedel commented Feb 2, 2022

bguedel commented Mar 10, 2022

ebk45 commented Dec 22, 2023

ebk45 commented Feb 5, 2024

		private static final long TOKEN_TTL = 600_000;
		private static final long TOKEN_ISSUED = 60_000;

fix/jwtSync #71

fix/jwtSync #71

Conversation

bguedel commented Aug 17, 2021

bguedel commented Aug 18, 2021

bguedel commented Aug 24, 2021

bguedel commented Sep 7, 2021

bguedel commented Sep 20, 2021

bguedel commented Sep 28, 2021

bguedel commented Oct 11, 2021

bguedel commented Oct 20, 2021

barbatron left a comment

Choose a reason for hiding this comment

barbatron Oct 7, 2021

Choose a reason for hiding this comment

barbatron Oct 25, 2021

Choose a reason for hiding this comment

bguedel Oct 25, 2021

Choose a reason for hiding this comment

bguedel Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

bguedel commented Nov 9, 2021

shaendler commented Nov 14, 2021

bguedel commented Nov 16, 2021

bguedel commented Nov 16, 2021

bguedel commented Nov 16, 2021

bguedel Nov 16, 2021

Choose a reason for hiding this comment

bguedel Nov 16, 2021

Choose a reason for hiding this comment

bguedel commented Nov 17, 2021

bguedel commented Nov 30, 2021

bguedel commented Dec 7, 2021

bguedel commented Dec 8, 2021

bguedel commented Dec 14, 2021

wsguede commented Jan 6, 2022

bguedel commented Jan 21, 2022

barbatron commented Jan 27, 2022 • edited Loading

bguedel commented Feb 2, 2022

bguedel commented Mar 10, 2022

ebk45 commented Dec 22, 2023

ebk45 commented Feb 5, 2024

bguedel Oct 25, 2021 •

edited

Loading

barbatron commented Jan 27, 2022 •

edited

Loading