Provide ability to set Display name

jaark / twofactor_yubikey

Yubikey Two-factor authentication provider for NextCloud

GNU Affero General Public License v3.0

21 stars 7 forks source link

Provide ability to set Display name #19

Open My1 opened 5 years ago

My1 commented 5 years ago

users with multiple yubis may want a way to name their yubikeys to have it easier to discern them.

Regards

jaark commented 5 years ago

An option al key name field has been added.

This required some relatively major changes to the code as it wasn't really using the model layer properly. Adding new fields and properties should be easier in future.

My1 commented 5 years ago

awesome. I'll check this as soon as I get the next release over here.

jaark commented 5 years ago

If you have a test environment, could you give the version in git a try please?

I want to run through a number of test scenarios before releasing to the app store as the old version wasn't written with things like changes to the database schema in mind so I want to test upgrades and fresh installs just in case it causes any problems. I'm aiming to be happy enough to push it out by the weekend (or the one after depending on if I get time).

My1 commented 5 years ago

I just need a quick info on how to properly drop this in (especially since nc iirc wants to see signatures for quite a while already.

jaark commented 5 years ago

You can just download the repository and copy the twofactor_yubikey directory into your apps directory.

NC only needs signatures when downloading from the app store.

This version has only been tested within my dev environment at the moment so please do not apply this to any environment where people would get upset if it breaks things (worst case scenario should be that Yubikey users cannot log in) or if you don't have much time to troubleshoot or roll-back. Disabling the app and reinstalling the app-store version should fix any issues.

My1 commented 5 years ago

sure I have a test NC which i use for trying out new addons in general never tried ones from git though

My1 commented 5 years ago

seems to work here. I can set a name and I can log in. whether "null" is a good name for unnamed Yubis is a question for discussion though)

jaark commented 5 years ago

That's odd, it doesn't show as 'null' here. Just a blank space.

What version of NC and PHP are you running?

My1 commented 5 years ago

PHP Version 7.2.17 NC 15.0.7

jaark commented 5 years ago

Did those 'null' keys exist with the previous version of the plugin?

My1 commented 5 years ago

well I need to check the DB but originally the table iirc was just username, of the affected user, the Yubikey public ID and probably (I dont remember) an index. so I doubt they could have been anywhere before.

jaark commented 5 years ago

I can duplicate that result when having keys that exist before upgrading the plugin. I'll get that fixed.

My1 commented 5 years ago

that's my current DB, obviously I just painted away the Yubikey IDs for privacy.

and yeah it's about existing keys.

but still hilarious that type:null gets outputted as String:"null" one of the more annoying SQL quirks in PHP I have played workarounds for because it's annoying.

jaark commented 5 years ago

Yeah, it is annoying. I've just pushed an update with a work-around to not display null in that case.