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

Avoid auto complete for OpenID scopes #16160

Merged
merged 10 commits into from
Jun 18, 2024
Merged

Avoid auto complete for OpenID scopes #16160

merged 10 commits into from
Jun 18, 2024

Conversation

hishamco
Copy link
Member

Fixes #13184

@hishamco
Copy link
Member Author

@hyzx86 in which browser you saw the bug?

@hyzx86
Copy link
Contributor

hyzx86 commented May 26, 2024 via email

@@ -54,7 +54,7 @@

<div class="mb-3" asp-validation-class-for="Scopes">
<label asp-for="Scopes" class="form-label">@T["Scopes"]</label>
<input asp-for="Scopes" class="form-control" />
<input asp-for="Scopes" class="form-control"autocomplete="off" />
Copy link
Contributor

@hyzx86 hyzx86 May 26, 2024

Choose a reason for hiding this comment

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

Referring to the screenshot below, there are two places where it is recommended to add directly to the form element

Also, the added attribute lost a space :)
image

Copy link
Member Author

Choose a reason for hiding this comment

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

there are two places where it is recommended to add directly to the form element

Where are they?

Also, the added attribute lost a space :)

Really? which browser? coz we using this attribute in several places

Copy link
Contributor

@hyzx86 hyzx86 May 27, 2024

Choose a reason for hiding this comment

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

Ignore the screenshot above, it appears to be out of date

I'm using Edge, the other place I'm referring to is Client Secret, the latest version of the code I looked at has added the autocomplete property.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK , the above screenshot is indeed from the latest version of the source code, the Scopes element has indeed not been adjusted, but the Client Secret has been adjusted, and it doesn't seem to be working as expected, it's still autofilling (note that the textboxes in the red area on the left contain a light blue background).

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean the current auto-filling? I don't think so. Please the tested browser

Copy link
Member

Choose a reason for hiding this comment

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

If you have credentials saved for the host, then it'll auto-fill them here still:

image

I have admin/Password1! saved for OC login. Check it after saving something like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a strange problem, I applied the changes in the code, but the problem still exists, if you open and save in an anonymous window, the autocompletion really doesn't work, but it still autofills in the default browser!

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't see the Client Secret in my screen, is there any configuration

Copy link
Member

Choose a reason for hiding this comment

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

Tick the checkbox on the screenshot.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see the Client Secret in my screen, is there any configuration

Yes, you have to check Use code response type , Refer to the screenshot above

@MikeAlhayek
Copy link
Member

@hishamco what is the status of this PR? This is a good PR that is fixing an issue.

@hishamco hishamco requested review from Piedone and hyzx86 June 17, 2024 05:07
@hishamco
Copy link
Member Author

Screenshot 2024-06-17 080831

Screenshot 2024-06-17 080858

@Piedone
Copy link
Member

Piedone commented Jun 17, 2024

Anybody with anything else or can we merge?

@@ -144,7 +144,7 @@

<div class="mb-3 collapse" asp-validation-class-for="ClientSecret">
<label asp-for="ClientSecret" class="form-label">@T["Client Secret"]</label>
<input asp-for="ClientSecret" class="form-control" type="password" autocomplete="off" />
<input asp-for="ClientSecret" class="form-control" type="password" autocomplete="new-password" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a good solution 👍

@hishamco hishamco merged commit 7a14fd9 into main Jun 18, 2024
6 checks passed
@hishamco hishamco deleted the hishamco/openid-secrets branch June 18, 2024 08:07
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.

Don't let browsers auto-fill Scopes and Client Secret OpenId fields
4 participants