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.

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: implement offline scope in the way google expects #3088

Merged
merged 10 commits into from Feb 14, 2023
Merged

fix: implement offline scope in the way google expects #3088

merged 10 commits into from Feb 14, 2023

Conversation

tsearle
Copy link
Contributor

@tsearle tsearle commented Feb 9, 2023

google doesn't implements offline access as a scope but instead as a special uri parameter

this PR adapts the code such that if the offline_access scope is requested, it gets transformed into the parameter
access_type=offline

@CLAassistant
Copy link

CLAassistant commented Feb 9, 2023

CLA assistant check
All committers have signed the CLA.

@tsearle tsearle changed the title implement offline scope in the way google expects fix: implement offline scope in the way google expects Feb 9, 2023
@tsearle
Copy link
Contributor Author

tsearle commented Feb 9, 2023

linked issue
ory/network#242

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Merging #3088 (c997c66) into master (c70704c) will increase coverage by 0.05%.
The diff coverage is 91.30%.

❗ Current head c997c66 differs from pull request most recent head 4c2bb42. Consider uploading reports for the commit 4c2bb42 to get more accurate results

@@            Coverage Diff             @@
##           master    #3088      +/-   ##
==========================================
+ Coverage   77.41%   77.46%   +0.05%     
==========================================
  Files         315      315              
  Lines       19812    19835      +23     
==========================================
+ Hits        15337    15365      +28     
+ Misses       3289     3283       -6     
- Partials     1186     1187       +1     
Impacted Files Coverage Δ
selfservice/strategy/oidc/provider_google.go 93.33% <91.30%> (+93.33%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tsearle
Copy link
Contributor Author

tsearle commented Feb 10, 2023 via email

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

selfservice/strategy/oidc/provider_google.go Outdated Show resolved Hide resolved
selfservice/strategy/oidc/provider_google.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Feb 10, 2023

Maybe you can add a small test that shows that the offline access thing is correctly configured when the scope is set? :)

@aeneasr aeneasr merged commit 39043d4 into ory:master Feb 14, 2023
@aeneasr
Copy link
Member

aeneasr commented Feb 14, 2023

Thank you for making Ory better :)

@tsearle tsearle deleted the make_oidc_auth_url_overridable branch February 14, 2023 16:15
@vinckr
Copy link
Member

vinckr commented Feb 21, 2023

Hello @tsearle
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants