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

Add regenerate secret feature for oauth2 #6291

Merged
merged 2 commits into from
Mar 9, 2019

Conversation

jonasfranz
Copy link
Member

No description provided.

@jonasfranz jonasfranz added this to the 1.8.0 milestone Mar 9, 2019
@jonasfranz jonasfranz added type/enhancement An improvement of existing functionality type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed type/enhancement An improvement of existing functionality labels Mar 9, 2019
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 9, 2019
return
}
ctx.Flash.Success(ctx.Tr("settings.update_oauth2_application_success"))
ctx.HTML(200, tplSettingsOAuthApplications)
Copy link
Member

Choose a reason for hiding this comment

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

I would like we redirect to the oauth2 application edit page but not render it again so that it's safe to refresh the current page.

Copy link
Member Author

Choose a reason for hiding this comment

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

This won't work since the secret is not saved and will only be shown once.

Copy link
Member

Choose a reason for hiding this comment

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

Don't familiar with macaron and I don't know if it supports what I said. I will give this a LGTM and so that my opinion will not block this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only way would be via session but that would complicate the procedure a lot and it will be less secure since the token is saved in ram / session provider.

Copy link
Member

Choose a reason for hiding this comment

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

There is temporary session variables that allow this work for redirects

@codecov-io
Copy link

Codecov Report

Merging #6291 into master will decrease coverage by 0.01%.
The diff coverage is 4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6291      +/-   ##
==========================================
- Coverage   38.89%   38.88%   -0.02%     
==========================================
  Files         359      359              
  Lines       50993    51018      +25     
==========================================
+ Hits        19833    19836       +3     
- Misses      28290    28312      +22     
  Partials     2870     2870
Impacted Files Coverage Δ
routers/user/setting/oauth2.go 0% <0%> (ø) ⬆️
routers/routes/routes.go 83.08% <100%> (+0.02%) ⬆️
models/repo_list.go 67.89% <0%> (+1.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8211e01...c388948. Read the comment docs.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 9, 2019
@jonasfranz jonasfranz merged commit 8fffb06 into go-gitea:master Mar 9, 2019
@jonasfranz jonasfranz deleted the feature/regenerate-secret branch March 9, 2019 16:30
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants