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

load U2F js only on pages which need it #11585

Merged
merged 6 commits into from
Jan 20, 2021
Merged

load U2F js only on pages which need it #11585

merged 6 commits into from
Jan 20, 2021

Conversation

kdomanski
Copy link
Contributor

Currently the U2F js will load on every page, as long as the user is enrolled in TOTP. WIth this change, the js will be only loaded on the U2F login screen and in the two-factor auth settings.

@kdomanski
Copy link
Contributor Author

cc @silverwind

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 23, 2020
@lafriks
Copy link
Member

lafriks commented May 23, 2020

Why not just set RequireU2F in needed handlers?

@CirnoT
Copy link
Contributor

CirnoT commented May 24, 2020

Yup, we should continue using RequireU2F but it should be removed from loadSecurityData.

It seems that it was added to loadSecurityData in order to populate U2F in window.config however I see no reason why it should be true on every page.

@silverwind
Copy link
Member

My comment in #11573 was more aimed at the removal of the RequireU2F variable here which would result in unconditional loading which should be avoided if possible.

I think the current loading mechanism via RequireU2F is generally fine (until we port the script to webpack/npm). The window.config.U2F variable is unused an can be dropped, it was added when I copied other similar variables to window.config in advance preparation.

@kdomanski
Copy link
Contributor Author

Updated. Please restart the arm64 failing test.

@techknowlogick techknowlogick added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label May 25, 2020
@techknowlogick techknowlogick added this to the 1.13.0 milestone May 25, 2020
@kdomanski
Copy link
Contributor Author

ping @lafriks @silverwind

@silverwind
Copy link
Member

Not sure I understand the impact of the move of this variable.

Is theU2FRegistrations variable not required in the new context?

@kdomanski
Copy link
Contributor Author

The impact is that the U2F is set to load only on the security settings page and not wherever the security data is loaded, e.g. oauth endpoints.
It's inspired by your comment #11573 (comment)

I'm pretty sure that the U2FRegistrations variable is orthogonal to this change.

@silverwind
Copy link
Member

Maybe @6543 can test this? I don't have a security token, so I can't verify.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 29, 2020
@CirnoT
Copy link
Contributor

CirnoT commented May 29, 2020

Works for me.

@6543
Copy link
Member

6543 commented May 29, 2020

will test soon

@6543
Copy link
Member

6543 commented May 29, 2020

ok cant test at the moment, it looks like my local test instance has no working U2F at all :/
tested with this branch and master - self cert https

I dont have time to dig into it at the moment - so neither a lgtm nor a block

@kdomanski
Copy link
Contributor Author

@6543 What do you mean by "has no working U2F at all", what's going wrong? I'd love to help you test this.

@kdomanski
Copy link
Contributor Author

@CirnoT If you could please suggest another reviewer, merging this PR would make it easier to deal with #11573

@CirnoT
Copy link
Contributor

CirnoT commented Jul 24, 2020

Not aware of anyone else aside from @6543, sorry.

@silverwind
Copy link
Member

Maybe @jonasfranz can test.

@6543
Copy link
Member

6543 commented Jul 24, 2020

I'll have some time today - Will hava a try again :)

@6543
Copy link
Member

6543 commented Jul 24, 2020

hmm still get Could not read your security key. with and without your patch :/

@kdomanski
Copy link
Contributor Author

I've been getting a similar error when the base URL in the config was HTTP and not HTTPS.

@lunny lunny removed this from the 1.13.0 milestone Sep 10, 2020
@lunny lunny added this to the 1.14.0 milestone Sep 10, 2020
@stale
Copy link

stale bot commented Nov 9, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Nov 9, 2020
@6543 6543 added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Nov 9, 2020
@stale stale bot removed the issue/stale label Nov 9, 2020
templates/base/head.tmpl Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 19, 2021
@GiteaBot GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 19, 2021
@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 Jan 20, 2021
@zeripath
Copy link
Contributor

make lg-tm work

@zeripath zeripath merged commit 26da20a into go-gitea:master Jan 20, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants