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

Run detectWebAuthnSupport only on sign-in page #31676

Merged
merged 2 commits into from
Jul 23, 2024
Merged

Run detectWebAuthnSupport only on sign-in page #31676

merged 2 commits into from
Jul 23, 2024

Conversation

wolfogre
Copy link
Member

Fix #31675, regression of #31504.

@wolfogre wolfogre added type/bug topic/ui Change the appearance of the Gitea UI labels Jul 23, 2024
@wolfogre wolfogre added this to the 1.23.0 milestone Jul 23, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 23, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 23, 2024
@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 Jul 23, 2024
wxiaoguang
wxiaoguang reviewed Jul 23, 2024
View reviewed changes
web_src/js/features/user-auth-webauthn.ts
@@ -5,6 +5,10 @@ import {GET, POST} from '../modules/fetch.ts';
const {appSubUrl} = window.config;

export async function initUserAuthWebAuthn() {
if (!document.querySelector('.user.signin')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is not right because link_account also uses signin_inner

But I guess some logic has been broken for a while.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably add a data-page="user-signin" attribute to <body> at some point to clearly indicate the which page we are on instead of relying on these flimsy selectors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you serious? I am talking the logic bug.

Copy link
Member

Choose a reason for hiding this comment

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

I guess next time put a negative review, because otherwise, every PR runs danger of being merged too hastily.

And yes, my remark was just some idea for the future, but apparently @techknowlogick had read it incorrectly.

@lunny lunny added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jul 23, 2024
techknowlogick
techknowlogick approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

+1 to the suggestion that we should add a page attribute rather than relying on the selectors. I think that this PR is a way to reduce the scope of the bug significantly while we do that refactor.

@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 Jul 23, 2024
@techknowlogick techknowlogick enabled auto-merge (squash) July 23, 2024 17:47
@techknowlogick techknowlogick added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 23, 2024
@techknowlogick techknowlogick merged commit bbd1476 into go-gitea:main Jul 23, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 23, 2024
@silverwind
Copy link
Member

silverwind commented Jul 23, 2024 •
edited
Loading

Yes, that was just a suggestion for the future. But if the webauthn code incorrectly runs on the link_account page that would be a outstanding issue, thought not a blocker I guess.

@wxiaoguang
Copy link
Contributor

Are you serious about the logic?

I am talking about the BUG, not about it should refactor or not.

image

@wxiaoguang
Copy link
Contributor

But if the webauthn code incorrectly runs on the link_account page that would be a outstanding issue, thought not a blocker I guess.

It is NOT "incorrectly runs on the link_account page". I have explained above: signin_inner is also used by link_account to "sign in".

zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 24, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
d63259b 
* giteaofficial/main:
  use nolyfill to remove some polyfills (go-gitea#31468)
  Properly filter issue list given no assignees filter (go-gitea#31522)
  Run `detectWebAuthnSupport` only on sign-in page (go-gitea#31676)
  fix OIDC introspection authentication (go-gitea#31632)
  Enable direnv (go-gitea#31672)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
@wolfogre wolfogre mentioned this pull request Jul 24, 2024
Merged
@wolfogre
Copy link
Member Author

wolfogre commented Jul 24, 2024 •
edited
Loading

But if the webauthn code incorrectly runs on the link_account page that would be a outstanding issue, thought not a blocker I guess.

It is NOT "incorrectly runs on the link_account page". I have explained above: signin_inner is also used by link_account to "sign in".

@wxiaoguang I think I got your point, the selector is wrong. It will cause skipping detectWebAuthnSupport on the link_account page, which supports signin-passkey and need the detection, right? What about #31691?

lafriks pushed a commit that referenced this pull request Jul 25, 2024
@wolfogre
Run detectWebAuthnSupport only if necessary (#31691)
cabcca3 
Follow #31676, which is not correct, see
#31676 (comment)

Fix #31675, regression of #31504.
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@KN4CK3R KN4CK3R KN4CK3R approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

@silverwind silverwind Awaiting requested review from silverwind

@delvh delvh Awaiting requested review from delvh

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

All pages show error JavaScript promise rejection: Cannot set properties of null (setting 'textContent').
7 participants
@wolfogre @silverwind @wxiaoguang @techknowlogick @KN4CK3R @lunny @GiteaBot