Skip to content

Make the "show password" icon on the login page optional.#4580

Closed
nono-gdv wants to merge 4 commits intoowncloud:masterfrom
nono-gdv:master
Closed

Make the "show password" icon on the login page optional.#4580
nono-gdv wants to merge 4 commits intoowncloud:masterfrom
nono-gdv:master

Conversation

@nono-gdv
Copy link

This patch provides the basic functionality. I'm really not sure about the configuration part; this is just the best I could do.

Also, I'm not sure how to localize the strings I have added.

@ghost
Copy link

ghost commented Aug 26, 2013

Can one of the admins verify this patch?

@Niduroki
Copy link
Member

@jancborchardt @owncloud/designers

@MorrisJobke
Copy link
Contributor

Why should this be configurable? Sorry, but I'm against this change

@nono-gdv
Copy link
Author

The rationale is here: #4577

Basically, in our context, this is a security issue.

@DeepDiver1975
Copy link
Member

@owncloud-bot this is okay to test

@DeepDiver1975
Copy link
Member

@nono-gdv Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Copy link
Member

Choose a reason for hiding this comment

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

while is might be a configuration option (/me still no convinced) it is for sure nothing to be set on the admin page

@nono-gdv
Copy link
Author

@DeepDiver1975 Well, the configuration interface is the part I struggled with. :-) I could very well do with a hidden option, be it configured in config.php or directly in the database (even though I'm not sure editing stuff directly in the database would be sane).

@MorrisJobke
Copy link
Contributor

@nono-gdv A config option inside the config.php is fine. And please leave a comment in config.php.sample

@nono-gdv
Copy link
Author

OK, I'm going to add the option to config.php.

@MorrisJobke
Copy link
Contributor

Now you commit nothing (inside two commits ;)) Keep in mind, that this is your branch and you are therefore allowed to force push.

@nono-gdv
Copy link
Author

Oops, I still have trouble with git and pull requests. :-) I am currently reworking the patch.

@nono-gdv
Copy link
Author

This last version works for me; sorry about the clutter in git log.

Additionally I emailed the contributor agreement a few minutes ago.

<img class="svg" id="password-icon" src="<?php print_unescaped(image_path('', 'actions/password.svg')); ?>" alt=""/>
<input type="checkbox" id="show" name="show" />
<label for="show"></label>
<?php endif; ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Identation

@MorrisJobke
Copy link
Contributor

@karlitschek
Copy link
Contributor

@kabum Confirmed :-)

@jancborchardt
Copy link
Member

Sorry, but either we have this functionality in or we don’t. This is not something which should be configurable. So definitely 👎 for this specific pull request. Let’s find a better solution instead:

Currently the toggle is in 3 places, always for the password field:

  1. Installation page for choosing a password (and for the probably complex database password)
  2. Log in page for putting in your password
  3. Personal settings for changing a password (the »current password« field does not have it)

Now the installation page and the personal settings need this switch so you can double-check the password you just decided. But the log in page actually does not need it. So let’s just cut it there.

Pull request upcoming.

@jancborchardt
Copy link
Member

The pull request is at #4610. @nono-gdv does that solve the issue?

I’ll close this pull request here then – but thank you for bringing up the point again! And I hope the fix works for you. :)

@nono-gdv
Copy link
Author

Simply removing the toggle from the login page is fine with me.

@jancborchardt
Copy link
Member

@nono-gdv sweet! And the pull request was merged. Thanks again for your help! :)

jancborchardt added a commit that referenced this pull request Aug 29, 2013
@lock lock bot locked as resolved and limited conversation to collaborators Aug 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments