search

simplesamlphp / simplesamlphp-module-oidc

A SimpleSAMLphp module for OIDC OP support.
Other
45 stars 22 forks source link

Release refresh token only if offline_access scope is used #191

Closed cicnavi closed 1 year ago

cicnavi commented 1 year ago

Note: client has to have 'offline_access' scope registered.

Fixes #154 Fixes #187

pradtke commented 1 year ago

I think this looks good. I was wondering how to check if any apps are currently making use of refresh tokens with out requesting offline scope (e.g. will I break any apps when I deploy this update). I couldn't see anything that would appear in the logs for a refresh token. Do you have any ideas?

cicnavi commented 1 year ago

No, I don't see how would we know if RP was using refresh token that we have released.... Found it... You can try and check this: during the access token generation when using code grant, the authorization code id is related to the newly generated access token. You can check this in DB table oidc_access_token. In the case of refresh token grant, there is no authorization code, so the entry for the auth_code_id column in that case is null. If you have nulls for auth_code_id, then the refresh token was used to get new access token. However, this is only for the currently available access token (not sure how often you delete expired tokens from DB...). We have entries like that, meaning that clients did not ask for refresh token using offline_access scope, but relied on the refresh token to get new token... So, we actually have that problem :/.

Our plan is this: we have a testing environment so we'll release new version there, announce the changes to RPs, and invite them to test things out. After some time, we will push that to production.

This also brings out the question about version of our next release. I was thinking to tag the next version as v2.1.0, as we have a new feature to support new scope, but despite the fact that we have a breaking change :/. I would write a small note that this is a change to bring us more in line with the spec... What do you think? Or should we tag a v3?

pradtke commented 1 year ago

I'll try to add a patch to our deployment to log current refresh token usage. I think once we have some data it will be easier to decide the impact. I suppose a 3rd option is to make it a configuration option 'alwaysIssueRefreshToken' and then when people upgrade they can decide what to do.

cicnavi commented 1 year ago

I think the default option is a good idea! This way we can still release v2 without problems. I'll try to implement it on Monday.

codecov[bot] commented 1 year ago

Codecov Report

Base: 38.78% // Head: 39.60% // Increases project coverage by +0.82% :tada:

Coverage data is based on head (2f0d5af) compared to base (36f12eb). Patch coverage: 65.78% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #191 +/- ## ============================================ + Coverage 38.78% 39.60% +0.82% - Complexity 857 868 +11 ============================================ Files 105 107 +2 Lines 2955 2987 +32 ============================================ + Hits 1146 1183 +37 + Misses 1809 1804 -5 ``` | [Impacted Files](https://codecov.io/gh/simplesamlphp/simplesamlphp-module-oidc/pull/191?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp) | Coverage Δ | | |---|---|---| | [lib/Factories/Grant/AuthCodeGrantFactory.php](https://codecov.io/gh/simplesamlphp/simplesamlphp-module-oidc/pull/191/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp#diff-bGliL0ZhY3Rvcmllcy9HcmFudC9BdXRoQ29kZUdyYW50RmFjdG9yeS5waHA=) | `0.00% <0.00%> (ø)` | | | [lib/Server/ResponseTypes/IdTokenResponse.php](https://codecov.io/gh/simplesamlphp/simplesamlphp-module-oidc/pull/191/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp#diff-bGliL1NlcnZlci9SZXNwb25zZVR5cGVzL0lkVG9rZW5SZXNwb25zZS5waHA=) | `0.00% <ø> (ø)` | | | [lib/Services/ConfigurationService.php](https://codecov.io/gh/simplesamlphp/simplesamlphp-module-oidc/pull/191/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp#diff-bGliL1NlcnZpY2VzL0NvbmZpZ3VyYXRpb25TZXJ2aWNlLnBocA==) | `46.47% <0.00%> (ø)` | | | [lib/Services/Container.php](https://codecov.io/gh/simplesamlphp/simplesamlphp-module-oidc/pull/191/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp#diff-bGliL1NlcnZpY2VzL0NvbnRhaW5lci5waHA=) | `0.00% <0.00%> (ø)` | | | [lib/Server/Grants/AuthCodeGrant.php](https://codecov.io/gh/simplesamlphp/simplesamlphp-module-oidc/pull/191/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp#diff-bGliL1NlcnZlci9HcmFudHMvQXV0aENvZGVHcmFudC5waHA=) | `5.85% <11.11%> (+5.85%)` | :arrow_up: | | [lib/Utils/Checker/Rules/ScopeOfflineAccessRule.php](https://codecov.io/gh/simplesamlphp/simplesamlphp-module-oidc/pull/191/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp#diff-bGliL1V0aWxzL0NoZWNrZXIvUnVsZXMvU2NvcGVPZmZsaW5lQWNjZXNzUnVsZS5waHA=) | `100.00% <100.00%> (ø)` | | | [lib/Utils/ScopeHelper.php](https://codecov.io/gh/simplesamlphp/simplesamlphp-module-oidc/pull/191/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp#diff-bGliL1V0aWxzL1Njb3BlSGVscGVyLnBocA==) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simplesamlphp)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

cicnavi commented 1 year ago

Please check the new config option for refresh token release, which is true by default. Basically this means no change in behavior regarding refresh token release comparing with v1. However, we have a possibility to do it in a spec compliant way by setting the option to false...

pradtke commented 1 year ago

Thanks @cicnavi . I'll try to upgrade out internal environment soon.