Breaking change in master

[clement escoffier]

Hello,

Since yesterday, there is a breaking change in the master branch, breaking the quoickstarts (compilation issue). The RefreshToken class disappeared. 

How do we want to handle this issue?

If we keep it that way, the master branch should be 2.0.0 and not 1.1.0.

Also, when merging something, always check that it does not break the associated quickstarts. 

Clement

[clement escoffier]

The culprit PR is: https://github.com/quarkusio/quarkus/pull/5302

Clement

[Sanne Grinovero]

Could it be re-introduced as a deprecated class?

Breaking stuff was fine so far, but onwards I'd expect it goes without
saying that the core components need to define some mostly-stable API
surface, and try hard to stick to it.

We could have some more flexibility with extensions, in particular the
ones we explicitly flagged as "preview". I don't know what
RefreshToken is - if it's an extension I'd not expect that to warrant
a major version bump but it's a good question.

> --
> You received this message because you are subscribed to the Google Groups "Quarkus Development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to quarkus-dev...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/9025e051-a3d0-4769-813a-bd328344ed79%40Spark.

[clement escoffier]

I agree that preview extension should be able to break a bit. 

But here it broke the quickstart and the guide. So we need to be much more cautious about these.

What you propose (re-introducing the class, with a deprecation notice) would be ok. But I would prefer having Sergey opinion on this (maybe we broke it because it was very buggy before that). 

If we keep the breaking change, we should introduce a label indicating it’s a breaking change so the release notes can mention them.

Clement

[Sergey Beryozkin]

Hi Clement,

I did that PR. I've done some justifications during the PR discussion (spread across few issues). But, to summarize, it is just in time that RefreshToken can be removed as an independent TokenCredential, for the following reasons:

- it is actually not doc-ed at [1]. It did make it to the quickstart, which is why I actually closed the 1st PR I made toward removing it, but...

- we do not offer any support for RefreshToken yet - it is not a real standalone credential, only relevant for the access token refreshment as part of the RT token grant and it is not a user level concern in general; we have an issue to use to auto-refresh access tokens before the user code is invoked, or at some later stage, indirectly use it from the user code via an OidcClient helper.

- Besides there were 2 RefreshToken classes and the one which was used in the tests/quickstarts was from a 'runtime' subpackage.

As such I felt it was the right time to do a quick update now.

I honestly do not expect the users have tried to do anything with RefreshToken - as it would require them to do some of the adapter work directly in the code (bootstrap the OIDC server connection, get a new AT with this RT etc, which is not what would make them say it is easy to work with our adapter :-) ). I left a PR comment that I'd get a soft drink for all if it were the case but then deleted it, but here it is, I'm repeating it again to a wider audience :-).

So it is all safe IMHO, going to follow up with a quickstart fix today.

Cheers, Sergey

[1] https://quarkus.io/guides/security-openid-connect-web-authentication

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/6e6e72b6-a823-4979-8f75-42d26f3ed237%40Spark.

[Sergey Beryozkin]

Sorry, could not fix the quickstart yesterday, I was probably already sleeping by the time the PR was merged :-)

[Max Rydahl Andersen]

Sounds like time to setup CI for PR's against quickstarts - at least their compilation.

Otherwise this will keep happening - intentionally or not.

where are we with extra bandwidth on CI ? :)

/max
https://xam.dk/about

--

[Sergey Beryozkin]

Hi Sanne

So, as I said to Clement, I'm thinking that at this moment of time it is safe to just drop it, though I can reintroduce it, but it was named inconsistently with other TokenCredential types (AccessTokenCredential, IdTokenCredential, RefreshToken), it was in the 'io.quarkus.oidc.runtime' subpackage (the class which made it to the quickstart), with another duplicate RefreshToken being in "io.quarkus.oidc", and most importantly is that we just do not offer any support for it yet.

If the team thinks it has to be reintroduced then I can do it, but in the meantime I'd just go for a quick start fix - and the fix I'm think of is not actually even show Refresh tokens being injected until we can actually offer some support for them :-)

Cheers, Sergey

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAFm4XO1OT4Sabj1xxbJvmUTfjFHa_ENH5Zc9CWD%2BmBbhQbgmgg%40mail.gmail.com.

[clement escoffier]

On 8 Nov 2019 at 11:37 +0100, Sergey Beryozkin <sbia...@redhat.com>, wrote:

Hi Sanne

So, as I said to Clement, I'm thinking that at this moment of time it is safe to just drop it, though I can reintroduce it, but it was named inconsistently with other TokenCredential types (AccessTokenCredential, IdTokenCredential, RefreshToken), it was in the 'io.quarkus.oidc.runtime' subpackage (the class which made it to the quickstart), with another duplicate RefreshToken being in "io.quarkus.oidc", and most importantly is that we just do not offer any support for it yet. 

What do you mean by this?

I can’t find any classes named RefreshToken in Quarkus master (all variants have been deleted?), and it seemed to do something as the Quickstart was using it (at least it was set, whether or not it was used I don’t know).

In the quickstart we had:

        

response.append("<li>refresh_token: ").append(refreshToken.getToken() != null).append("</li>”);

That now need to be something like:

String refreshToken = identity.getCredential(AccessTokenCredential.class).getRefreshToken();

response.append("<li>refresh_token: ").append(refreshToken != null).append("</li>”);

Clement

To view this discussion on the web visit https://groups.google.com/d/msgid/quarkus-dev/CAMsYBfVO2MMVCPuLfzPP6GRWR63RkSGpK56%3DqwtGN4B_Lvy6MA%40mail.gmail.com.

[clement escoffier]

BTW, if we decide to keep the breaking change (and so keep track of it somehow), should I fix the quickstart? 

Clement

[clement escoffier]

[Damn too fast on the send button]

Should I fix the quickstart and the guide?

Clement

[Sergey Beryozkin]

Hi Clement

(CC-ing Pedro and Stuart)

As I said it is not documented this is why I thought it was just in time and safe. As far as the quickstart fix is concerned, since I've broken it, I need to fix it myself. But, let me do few clarifications first, I can get that class back, IMHO we are OK without it, but I'd like to avoid the concerns that perhaps it has been a bit careless, not a big deal, I can put it back... Anyway, few more comments first:

Yes, showing it being accessed as a String by having to access it with

identity.getCredential(AccessTokenCredential.class).getRefreshToken();

is less cool of course, with PR which I will follow up shortly with, it would be

@Inject

AccessTokenCredential at;

// access RT:

at.getRefreshToken();

This pattern does reflect what Refresh Token is about (I thought Pedro was good with it even before I opened PR), it is not a standalone credential that the user would inject and use it for the application purposes. For example, injecting an access token will let the code flow applications to do the remote calls with it to other services, injecting IdToken - interact with the users. But refresh token, no, they can't do anything with it but refresh the access token which goes into the OIDC adapter territory.

So it does not matter IMHO whether how the users can get a hold of the RT value. The point is, they should not doing the drect work with refresh token values and OAuth2 RT grants :-)

So, will I go for a quick start fix (just remove the reference to RefreshToken), or, for restoring RefreshToken ?

Note there are 2 approaches for the users to have the access tokens refreshed:

- Auto-refreshment, the adapter will do it if it thinks the AT will expire soon (we have an issue opened) - will be done before the user code is invoked

- In the user code via an injected helper. If I restore RefreshToken then it can become this injected helper. I was thinking about a more generic helper such as OidcClient (or Pedro's name, Oauth2Client), but I'm fine now with restoring it back. In fact I'll open a draft PR and promote it once approved

Thanks, Sergey

Thanks, Sergey

[Sergey Beryozkin]

Be back in 30 mins...

[Sergey Beryozkin]

OK, I'm restoring it, a non-draft PR will be out shortly. But, I'd really like to keep it be out of 'runtime' subpackage :-) and will maintain the link between AT and RT credentials. We can tweak it during the PR review.

Thanks, Sergey

[Sergey Beryozkin]

In fact, I said earlier RT class was in both io.quarkus.oidc and io.quarkus.oidc.runtime, I reckon the former was likely meant to be used, but we were all in a rush fixing things for 1.0... :-)

[Sergey Beryozkin]

Thanks Pedro, nearly there with a new PR, trying to squeeze the injection of all the token credentials into it :-)

Sergey

On Fri, Nov 8, 2019 at 1:10 PM Pedro Igor Silva <psi...@redhat.com> wrote:

Yeah, we should have APIs outside runtime package. I think your changes should be fine as we don't actually have those types documented but in tests.

[Sergey Beryozkin]

Hi All,

https://github.com/quarkusio/quarkus/pull/5331

Clement, thank you for raising this issue here, I should've started the discussion here instead of limiting it to my PR yesterday.

I'm actually happier now with a new PR, RT is restored (the only difference it is out of 'runtime' subpackage), we still have an AT->RT link and it may become bi-directional, so we can keep the options open where to refresh AT, and having just 'RefreshToken' instead of  'RefreshTokenCredential' (I thought of renaming it) does look OK to me now after all because it kind of shows it is not a standalone credential (though I can still rename if someone suggests it in the new PR).

Will fix the quickstart after this PR gets merged

Sergey

[Sanne Grinovero]

On Fri, 8 Nov 2019, 10:37 Sergey Beryozkin, <sbia...@redhat.com> wrote:

Hi Sanne

So, as I said to Clement, I'm thinking that at this moment of time it is safe to just drop it, though I can reintroduce it, but it was named inconsistently with other TokenCredential types (AccessTokenCredential, IdTokenCredential, RefreshToken), it was in the 'io.quarkus.oidc.runtime' subpackage (the class which made it to the quickstart), with another duplicate RefreshToken being in "io.quarkus.oidc", and most importantly is that we just do not offer any support for it yet.

Sounds reasonable to drop this now assuming the patch makes it to 1.0.0.Final 

Mine was more like a general reminder that after the release we should all be more careful with APIs which have previously been exposed.

(Not meaning to call this one change out specifically, but rather an opportunity to remind this as I saw some confusion on other patches as well)

Thanks 

[Sergey Beryozkin]

Hi Sanne

You are right :-), +1

But RefreshToken is asking to get back now in a new PR :-) though in a new package. Will add you to the reviewers.

Cheers, Sergey