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

Jump to bottom

Auth: Split signout_redirect_url into per provider settings #75269

Merged
merged 23 commits into from
Nov 29, 2023
Merged

Auth: Split signout_redirect_url into per provider settings #75269

merged 23 commits into from
Nov 29, 2023

Conversation

venkatbvc
Copy link
Contributor

What is this feature?

This is to support separate signout_redirect_url per oauth provider. auth.signout_redirect_url is still supported. Precedence is given to oauth provider signout url if configured both.

Which issue(s) does this PR fix?:

Fixes #73990

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@venkatbvc venkatbvc requested review from torkelo, a team and chri2547 as code owners September 22, 2023 09:21
@venkatbvc venkatbvc requested review from vtorosyan, IevaVasiljeva, zserge, mildwonkey and nikimanoledaki and removed request for a team September 22, 2023 09:21
@grafana-pr-automation grafana-pr-automation bot added type/docs area/backend pr/external This PR is from external contributor labels Sep 22, 2023
chri2547
chri2547 reviewed Sep 25, 2023
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

Left a docs suggestion. Thank you for the contribution.

@venkatbvc
Copy link
Contributor Author

@Jguer Can you please review and let me know your comments

@venkatbvc @chri2547
Update docs/sources/setup-grafana/configure-security/configure-authen…
5dd2dc4 
…tication/grafana/index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@Jguer Jguer self-requested a review September 27, 2023 16:50
@Jguer Jguer added add to changelog no-backport Skip backport of PR labels Sep 27, 2023
@Jguer Jguer changed the title Split signout_redirect_url into per provider settings Auth: Split signout_redirect_url into per provider settings Sep 27, 2023
@Jguer Jguer self-assigned this Sep 27, 2023
@Jguer Jguer added this to the 10.2.x milestone Sep 27, 2023
@Jguer
Copy link
Contributor

Jguer commented Sep 27, 2023

Hey @venkatbvc , a quick code review seems to show everything is in order to get this merged. We're going to test this commit against our local sample idPs to confirm everything is working correctly and hopefully we'll just merge it 🙂 .

Thanks for taking the time to do this

@venkatbvc
Copy link
Contributor Author

@Jguer Let me know if there is any pending action from myside?

@github-actions GitHub Actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale Issue with no recent activity label Nov 26, 2023
@venkatbvc venkatbvc requested a review from a team as a code owner November 27, 2023 11:59
mgyongyosi
mgyongyosi reviewed Nov 27, 2023
View reviewed changes
pkg/login/social/social.go Outdated
@@ -75,6 +75,7 @@ type OAuthInfo struct {
UsePKCE bool `mapstructure:"use_pkce"`
UseRefreshToken bool `mapstructure:"use_refresh_token"`
Extra map[string]string `mapstructure:",remain"`
SignoutRedirectUrl string `toml:"signout_redirect_url"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please change the tag to mapstructure:"signout_redirect_url" and move it above the Extra field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgyongyosi will take a look and align, incase of queries will reach out to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgyongyosi
Modified as per the comment. continuous-integration/drone/pr is pending state. is it due to Milestone is set as 10.2.x?

@github-actions github-actions bot removed the stale Issue with no recent activity label Nov 28, 2023
@mgyongyosi mgyongyosi modified the milestones: 10.2.x, 10.3.x Nov 28, 2023
@venkatbvc
Copy link
Contributor Author

@mgyongyosi Thanks for modifying the milestone. continuous-integration/drone/pr is still in pending state. It is giving below message in the details link: "Build is blocked, please, contact repo admin in order to proceed"
Can you please let me know what shall be done from my side?

@Jguer
Copy link
Contributor

Jguer commented Nov 29, 2023

Hey @venkatbvc , I've finished testing the PR and I'm just waiting on the CI to pass before merging, no further changes should be necessary.

I confirmed it worked with generic oauth, SAML and that the legacy behavior was kept intact.

I made some small changes but I will squash the commits without co-author on merge.

Thanks for taking the time to do this, sorry that it took me some time to get back to you 🙂

@venkatbvc
Copy link
Contributor Author

@Jguer @mgyongyosi Thanks for approving the pull request.
continuous-integration/drone/pr is still in pending state.

@Jguer Jguer merged commit e152323 into grafana:main Nov 29, 2023
12 checks passed
gabor pushed a commit that referenced this pull request Nov 29, 2023
@venkatbvc @chalapat
@chri2547 @Jguer @gabor
Auth: Split signout_redirect_url into per provider settings (#75269)
df2622d 
* Split signout_redirect_url into per provider settings

* Split signout_redirect_url into per provider settings

* Update docs/sources/setup-grafana/configure-security/configure-authentication/grafana/index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

* Split signout_redirect_url into per provider settings

* Split signout_redirect_url into per provider settings

* Split signout_redirect_url into per provider settings

* Split signout_redirect_url into per provider settings

* Split signout_redirect_url into per provider settings

* Split signout_redirect_url into per provider settings

* update docs

* update devenvs

* add missing struct tag

---------

Co-authored-by: Rao, B V Chalapathi <b_v_chalapathi.rao@nokia.com>
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
Co-authored-by: jguer <me@jguer.space>
@Terr4
Copy link

Terr4 commented Dec 21, 2023

Hi all, is this related?
#79778

@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
@kodyabbott kodyabbott mentioned this pull request Dec 27, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chri2547 chri2547 chri2547 left review comments

@mgyongyosi mgyongyosi mgyongyosi approved these changes

@Jguer Jguer Jguer approved these changes

@torkelo torkelo Awaiting requested review from torkelo torkelo is a code owner

@vtorosyan vtorosyan Awaiting requested review from vtorosyan vtorosyan was automatically assigned from grafana/grafana-authnz-team

@IevaVasiljeva IevaVasiljeva Awaiting requested review from IevaVasiljeva IevaVasiljeva was automatically assigned from grafana/grafana-authnz-team

@zserge zserge Awaiting requested review from zserge zserge was automatically assigned from grafana/backend-platform

@mildwonkey mildwonkey Awaiting requested review from mildwonkey mildwonkey was automatically assigned from grafana/backend-platform

@nikimanoledaki nikimanoledaki Awaiting requested review from nikimanoledaki nikimanoledaki was automatically assigned from grafana/backend-platform

@dmihai dmihai Awaiting requested review from dmihai

Assignees

@dmihai dmihai

@Jguer Jguer

Labels
add to changelog area/backend no-backport Skip backport of PR pr/external This PR is from external contributor triage/pending type/docs
Projects
None yet
Milestone
10.2.3
Development

Successfully merging this pull request may close these issues.

Split signout_redirect_url into per provider settings
9 participants
@venkatbvc @Jguer @Terr4 @mgyongyosi @dmihai @chri2547 @aangelisc @tonypowa @chalapat