2fa #204
2fa #204
Conversation
Gdy taki przycisk będzie, to na pewno nie będzie przeszkadzał i może gdzieś tam się przyda, ale wątpię, aby mógł być sensownie używany. To z tego powodu, że nie mamy jak przeprowadzić takiej weryfikacji użytkownika. Najlepiej jakby była alternatywna metoda, np. nie wiem, weryfikacja SMS albo U2F. SMS to pewnie będą koszty, więc raczej od razu odpada i ogólnie nie wiem czy aż taka rozbudowa ma sens. Wydaje się, że kod/kody jednorazowe awaryjne będą w porządku. |
|
Nie rozumiem czemu nie przejąłeś brancha istniejącego? |
|
U mnie nie zadziałało |
event15
left a comment
event15
left a comment
There was a problem hiding this comment.
Ogólnie nie działa mi to lokalnie, podejrzewam, że może chodzi o datę, która jak sobie zmienię na $time->format to nie zapisuje sie w bazie poprawnie. Z drugiej strony może to być kwestia konfiguracji lokalnej.
Nie działa, to znaczy, że po wylogowaniu w formularzu wpisania kodu za każdym razem jest nieprawidłowy, odrzuca mi ten kod.
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-layer.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-layer.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-page-login.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-page-login.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-page-login.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-page-login.php
Outdated
Show resolved
Hide resolved
- dodanie Intl do obrazu dockera - zastosowanie zalecen lintera kodu - dodanie wymaganych rozszerzen do wymagan composera
|
Uważam, że brakuje tu weryfikacji utworzenia 2fa podobnie jak na GH czy innych stronach:
|
|
Potrzeba pomysłów jak to rozwinąć. Na chwilę obecną działa. |
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-overrides.php
Outdated
Show resolved
Hide resolved
| $note = str_replace( | ||
| ['{{ QR_CODE }}', '{{ SECRET }}', '{{ RECOVERY_CODE }}', '{{ ERROR_START }}', '{{ ERROR_END }}'], | ||
| [ | ||
| '<br><div style="text-align: center;"><img src="' . $this->init->getQRCode() . '"></div><br>', |
There was a problem hiding this comment.
Czy te <br>y są potrzebne? Odstęp między elementami można zrobić przy użyciu margin w CSS. A skoro o stylowaniu mowa, to przy okazji wypadałoby przenieść ten text-align: center; do CSSa.
Brakuje atrybutu [alt] dla obrazka - mógłby mieć wartość np. "QRCode".
There was a problem hiding this comment.
Jeśli chcesz, możesz wprowadzić zmiaany - nie mam zbyt wiele kompetencji do edycji html/css.
| '<br><div style="text-align: center;"><img src="' . $this->init->getQRCode() . '"></div><br>', | ||
| '<code>' . $secret . '</code>', | ||
| '<code>' . $recoveryCode . '</code>', | ||
| '<br><div class="qa-error">', |
There was a problem hiding this comment.
J.w. czy ten <br> jest potrzebny?
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-overrides.php
Outdated
Show resolved
Hide resolved
- przeniesienie strony na osobny link - drobna estetyka
|
Małe podsumowanie: |
|
Zapomniałem dodać: |
- przeniesienie strony na osobny link - drobna estetyka
awaluk
left a comment
awaluk
left a comment
There was a problem hiding this comment.
Działa, to już fajnie ;) Funkcjonalnie jednak trochę kuleje.
Trzeba coś zrobić z zależnościami i composerem, bo instalacja per plugin jest nieco (bardzo) niewygodna. Mamy globalnego composer.json gdzie jest instalowane Sentry, może chociaż tam dodać? Średnie rozwiązanie bo plugin to raczej ma być niezależna całość, no ale gdybyśmy mieli 20 pluginów i do każdego ręcznie instalować zależności w osobnym katalogu... Jak tak to może chociaż jakiś skrypt który sam za jednym uruchomieniem wykryje wszędzie gdzie trzeba instalacji i to zrobi na wywołanie jednego polecenia. Na ten moment nie ma nawet informacji, że trzeba to zrobić, więc przeciętna osoba stawiająca forum od razu zostanie zasypana błędami i musi szukać o co chodzi.
Nie widzę powodu, aby "Ustawienia zabezpieczeń konta" było pogrubione. Co więcej tym generujemy kolejną podstronę do przejścia nie wiadomo skąd, aby zmienić informacje na temat konta. Nie lepiej byłoby to zrobić jako odesłanie ze szczegółów konta czyli /account? Nazywa się to zabezpieczeniami konta, a w rzeczywistości to tylko 2FA, jak już zabezpieczenia to np. zmiana hasła też tam powinna być np. Wiem że tak najprościej, ale próbuję postawić się jako użytkownik i szukam w tym jakiejś logiki i porządku.
Kliknąłem na "włącz" i pokazuje mi się, że jest włączone od "0". Zero? Lepiej dać tam cokolwiek innego, aktualną datę a jak nie to pokazać od "teraz" lub ukryć całkowicie ten wiersz.
Kliknąłem, niby jest włączone i gdy odświeżę stronę to pokazuje mi się znowu kod do skanowania, odświeża mi się secret oraz kod zapasowy - za każdym razem inny. Jak już normalnie przejdę po podstronach i wrócę to jest ok, ale przy odświeżeniu wygląda na to, że nie ma sprawdzenia czy już kod jest wygenerowany czy nie. Po zeskanowaniu użytkownik powinien wpisać do nas pierwszy kod, aby potwierdzić że jest ok, jak już wyżej sam wspomniałeś. Wtedy (i przy każdym kolejnym wejściu) pokazuje się tylko informacja, że 2FA jest włączone, przycisk do wyłączenia oraz ewentualnie opcja ponownego wygenerowania kodu zapasowego - albo nadpisania poprzedniego albo generacji kolejnego.
Włączenie i wyłączenie 2FA koniecznie za podaniem hasła do konta. Wyłączenie być może także za podaniem kodu z apki - tu mam wątpliwość, bo jak ktoś straci apkę to nie będzie mógł tego wyłączyć, ale jednocześnie jak gdzieś zostawi zalogowane konto i ktoś pozna hasło, to będzie mógł wyłączyć 2FA. Wygenerowanie nowego kodu zapasowego jeśli będzie to to samo, przynajmniej za podaniem hasła, być może też podaniem kodu.
Po zeskanowaniu kodu w apce pokazuje mi się jako logo witryny zdjęcie jakiegoś gostka, to tylko u mnie, może jakiś bug apki? Choć raczej wątpię, bo nigdy mi się wcześniej coś takiego nie zdarzyło. Może to się ustawia gdzieś.
Przy użyciu kodu awaryjnego pokazuje się ładny komunikat i aż się prosi, aby była opcja wygenerowania ponownie takiego kodu bez wyłączania całej autoryzacji, jak mówiłem wyżej, no ale powiedzmy że to byłby fajny dodatek. Jak nie będzie to trudno. Gdy wyświetla się ten komunikat o tym, że ktoś się zalogował kodem awaryjnym to jednocześnie nadal nie jest zalogowany - po prawej nie widać jego awatara, tylko dalej ikonę logowania. Dopiero po przejściu na inną stronę nagle pokazuje się jako zalogowany, wydaje się to nieintuicyjne. Jak już tak musi być to chociaż jakiś przycisk "Kontynuuj" czy coś, aby użytkownik nie był zagubiony co się dzieje.
Przy zmianie hasła dostałem błąd:
( ! ) Fatal error: Uncaught ArgumentCountError: Too few arguments to function qa_db_user_set(), 2 passed in /var/www/html/forum/qa-include/qa-base.php(592) : eval()'d code on line 59 and exactly 3 expected in /var/www/html/forum/qa-include/db/users.php on line 136
A po zmianie hasła w ogóle przestało działać mi logowanie kodem. Nie wiem czy to przypadek, czy ma to jakiś wpływ czy co się stało. Po podaniu poprawnego kodu na logowaniu jestem przekierowywany na białą stronę z błędem takim samym jak wyżej.
Zastanawiam się ogólnie nad momentem podawania kodu, gdzie dane logowania jak hasło są wtedy trzymane jako input type hidden, czy to na pewno tak powinno być zrobione. Nawet jeśli tak to w sytuacji gdy podczas wpisywania kodu zmieni się hasło do konta, wtedy użytkownik ciągle dostaje info o błędnym kodzie z apki, a nie błędnych danych. Mało możliwa sytuacja w praktyce, ale w teorii może się wydarzyć i raczej nie powinno się tak dziać.
| "minimum-stability": "dev", | ||
| "autoload": { | ||
| "psr-4": { | ||
| "CodersCommunity\\": "src/" |
There was a problem hiding this comment.
myślę, że namespace powinien być bardziej konkretny, tzn. zawierać jeszcze chociażby nazwę plugina, ale tu jest cała kwestia uzywania composera, opisałem w komentarzu
There was a problem hiding this comment.
miałem na caly ten kod zupełnie inny pomysł z początku, ale zostałem uprzedzony i kod z obiektowego stał się q2a'owy. Przy takim stanie rzeczy przyjąłem to co już było i nie starałem się diametralnie wszystkiego zmieniać. Jeśli się uda dodać do composera głównego zależność a jej tu nie budować, to być może to zniknie, z resztą o ile mam dobrą pamięć, to ten namespace jest nieużyty, a na pewno nie tak, jak powinien.
There was a problem hiding this comment.
nawet jeśli ten byłby nie użyty to CodersCommunity kojarzy się z nazwą całej organizacji forumowej, więc czemu ma należeć do jednego plugina. Tym bardziej już gdyby coś było w globalnym lepiej byłoby dać chociażby CodersCommunity\GoogleAuthenticatorLogin czy cokolwiek innego, co jednoznacznie wskazuje, że chodzi o dany plugin, nie całą grupę.
| } | ||
| }, | ||
| "require": { | ||
| "robthree/twofactorauth": "dev-master" |
There was a problem hiding this comment.
nie lepiej dla bezpieczeństwa wskazać konkretną wersję o jaką nam chodzi?
| ) | ||
| ); | ||
| if (count($result) !== 1) { | ||
| echo 'Invalid num_rows'; |
There was a problem hiding this comment.
nie wiem co to konkretnie ma robić, ale wyświetlenie gdziekolwiek takiego błędu nie brzmi dobrze - nic nie mówi użytkownikowi
There was a problem hiding this comment.
to powinna być zmodyfikowana kopia metody q2a. Widzę, że @Mariusz08 trochę mocno tu sobie wszedł z butami :) do zmiany raczej.
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-overrides.php
Outdated
Show resolved
Hide resolved
Być może uda mi się coś zrobić.
Uważam, że jest to logiczniejsze rozwiązanie iż "Edycja konta" która była wcześniej. Większość serwisów ma osobną strefę na ustawienia zabezpieczeń i nie widzę powodu do odstępowania od tej niepisanej reguły.
Albo nie rozumiem, albo się nie zgadzam.
Jest tak to zrobione, ponieważ:
Zmianę hasła mógłbym zmienić, ale to ingerencja w widoki core, więc musiałbym dość dużo kodu wkleić w overides. Myślę, że jest to wykonalne - nie wiem jak to estymować w czasie pracy.
Prawdopodobnie pobrałeś zanim zrobiłem na to fixa.
Do naprawy.
Z jednej strony tak - z drugiej konkretnym wymogiem to nie jest. Myślę, że to jest do zrobienia, ale wymaga trochę kombinowania. W tej chwili jest już funkcjonalnym kawałkiem kodu bez takich rozwiązań. Ale zgadzam się - raczej warto się nad tym pochylić.
Czym skanowałeś? Nigdy nie widziałem żadnego logo. Musiałbym mieć screen/fotę tego zachowania, pierwszy raz z czymś takim się spotykam.
Nie. Żadna apka na świecie nie oferuje generowania dodatkowych kodów - ewentualnie oferuje listę takich przy zakładaniu 2FA.
Przyznam się szczerze - nigdy nie testowałem kodu awaryjnego więc nie mogę się teraz odnieść.
Ciekawy case. Fajnie, że przetestowałeś to od tej strony bo nie wpadłem na zmianę hasła w czasie działania. Sprawdzę jak u mnie.
NIe robiłem tego formularza i nie wpadłem na to, że to hasło jest trzymane jako input. Zastanowię się jak to ogarnąć. |
Ale edycja konta nadal jest, w tym rzecz. Ja też uważam, że ten podział jest dobry, tyle że to powinno być podzielone na kategorie w ramach ustawień. Zobacz np. nawet GH którego wspominałeś: otwierasz w menu settings i tam masz wszystko podzielone na tematyczne kategorie. To jest super, do tego bym dążył. A u nas tak: klikasz na awatar, tam nawet nie ma wprost ustawień tylko trzeba kliknąć nick i trafiasz na profil. Na profilu guzik "edytuj profil", ok. Na górze znowu zakładki gdzie jedna to jest widok profilu (pogląd), obok zadane pytania (których nijak nie można ruszyć), obok nagle znowu przycisk do edycji danych, dalej "połączone konta" i zaraz jeszcze te zabezpieczenia. Trochę bajzel po prostu, nieintuicyjnie. Idealnie byłoby jakbyśmy mieli jedną stronę osobną: ustawienia. I tam kategorie, np. "profil" i dane widoczne na profilu publicznie. "powiadomienia" - ustawienia powiadomień. "bezpieczeństwo" - zmiana hasła, 2FA, U2F i co jeszcze zapragniesz, "połączone konta" i tam połączenie z FB i Discordem lub możliwość wykonania ich. Obecnie niestety mamy wszystko wszędzie i nie wiadomo gdzie. Domyślam się, że to nie będzie łatwe, o ile w ogóle wykonalne. Ale poklikaj sobie sam i zobacz czy odnalezienie wszystkiego co jest typowo ustawieniami jest u nas dobrze zrobione ;) Dlatego zwróciłem uwagę.
Jeśli do wyłączenia 2FA będzie wymagany kod z apki to koniecznie trzeba. Jak dla mnie to standard, zawsze jest weryfikowane przed włączeniem, aby nie udupić użytkownika, któremu dodanie nie pójdzie.
Zalogowanie, a podanie hasła do włączenia 2FA to całkiem co innego. Zalogować się możesz raz na komputerze, odejść od niego i w tym czasie ktoś Ci ustawi swoje 2FA i jeśli aby je wyłączyć musisz podać kod z jego apki, to masz problem :) Dlatego jak dla mnie do włączenia/wyłączenia 2FA osobno hasło, może być jednorazowo, może być coś jak ma GH, ale to bardziej rozbudowane - dodatkowa autoryzacja hasłem na bardziej drażliwe operacje, która trwa kilka/kilkanaście minut i później jak chcesz coś ważniejszego zrobić to znów hasło i znów jest ważne jakiś czas.
Authy. Chciałem zrobić screena, ale mi nie pozwala... Obsługuje różne ikonki, sam pokazuje od razu dla GH, FB czy innych serwisów i tu sam mi pokazał zdjęcie jakiegoś gostka. Może to sama aplikacja, może to jest gdzieś konfigurowalne w QR, nie mam pojęcia.
Pudło, nawet na GitHubie można wygenerować nowe kody zapasowe nie wyłączając 2FA ;) Chyba że jako apkę zrozumiałeś że chodziło mi na telefonie tę, która generuje kody - to oczywiście nie. Chodziło mi że po naszej stronie normalnie, tak jak pierwotny kod. |
|
Ale edycja konta nadal jest, w tym rzecz. Ja też uważam, że ten podział jest dobry, tyle że to powinno być podzielone na kategorie w ramach ustawień. Zobacz np. nawet GH którego wspominałeś: otwierasz w menu settings i tam masz wszystko podzielone na tematyczne kategorie. To jest super, do tego bym dążył. Ok, ale czy w ramach PR do 2FA mam zmieniać wygląd i umiejscowienie wszystkiego?
Właśnie z tego powodu dodałem ten link w formie pogrubionej.
Czy to MUSI być wykonywane w ramach tego PR? To nie ma związku z funkcjonalnością.
To by wymusiło zmiany w tym pluginie gdyby ktoś chciał w przyszłości coś nowego zrobić (przykładowo block user pw, który też dodaje link w user sub nav). Jeśli ten plugin zmieniałby wygląd i umiejscowienie ustawień użytkownika w layout to albo trzeba zrobić swój własny theme, albo musi wiedzieć, że przy dodawaniu linków będzie musiał uwzględnić że jakiś plugin zewnętrzny zmienia jego umiejscowienie i być może wymusza w nim jakąś zmianę.
Wszystko jest wykonalne, tylko potrzeba odpowiednich środków i czasu. A przede wszystkim chęci na zmianę. Moim celem, gdy wymyślałem 2fa pierwotnie było dodanie funkcjonalności, a nie przeoranie całego theme.
Całe forum nie spełnia ani jednego standardu UX. Wyszedłem z założenia, że to mu nie zaszkodzi. |
|
Nie, to jest wręcz niewskazane w ramach tego PR. Jednak sprawdzając tego PRa to mi się jeszcze bardziej dało we znaki, dlatego o tym tu wspomniałem, bo coś trzeba niewątpliwie z tym zrobić. Nie w tym PR, bo nie tego dotyczy. Być może takie przygotowanie gruntu pod ustawienia powinno być zrobione najpierw w takim razie, aby problemu nie pogłębiać. Tyle że to jest u nas trudne, bo mamy co mamy i każda zmiana jest dobra. Chęci na zmianę u nas są więc bardzo duże, tu się nie zgodzę. Przede wszystkim potrzeba czasu oraz ludzi którzy to zrobią. Wszystko jest wykonalne, tu się zgodzę, tylko pytanie jakim koszem - czasem może się okazać, że zupełnie nieopłacalnym w porównaniu do otrzymanego efektu lub efektów, jakie można by w tym samym czasie uzyskać w innych miejscach.
To akurat zupełnie nic nie zmienia. Tylko przyciąga uwagę, ale nadal nie ma wpływu na to o czym mówiłem w akapicie, który zacytowałeś.
Dość dziwne założenie. To że coś jest kiepskie, to nie znaczy że można bez przemyślenia psuć to dalej ;) Jak mówiłem wyżej - jednym PRem i dyskusją tego nie zmienimy, z tym się zgadzamy. Ale jak będziemy tylko siedzieli ze świadomością że jest kiepsko i dodawali nową funkcjonalność, to też nic się nie stanie. Moja propozycja: w ramach tego PR oczywiście choćbyśmy chcieli to nie zrobimy wszystkiego. W ramach tego przygotować tę funkcjonalność 2FA. Z tyłu głowy mieć, że ustawienia warto przebudować. Można nawet zrobić issue, zapisać obserwacje, pomysły itp. A informację o samym 2FA dodałbym gdzieś na podstronie edycji danych obecnej dodatkowo. Tam jest zmiana maila, hasła itp., odruchowo tam bym szukał 2FA i nie wpadł na to, że to będzie gdzieś indziej. Więc chociaż jakiś link, przycisk, cokolwiek co nakieruje użytkownika, że takie coś jest. |
- wydzielenie custom.css - zmiana wygladu edycji uzytkownika - przeniesienie kodu layers do jednej klasy - podlinkowanie sekcji 2fa w edycji konta
- wizualne poprawki
forum/qa-plugin/q2a-googleauthenticator-login/q2a-user-security-layer.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-user-security-layer.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-layer.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-layer.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-layer.php
Outdated
Show resolved
Hide resolved
forum/qa-plugin/q2a-googleauthenticator-login/q2a-googleauthenticator-layer.php
Outdated
Show resolved
Hide resolved
- wizualne poprawki
|
@awaluk trochę przeorałem to. Jutro opiszę dokładniej co tu się wydarzyło. Jeszcze wymaga wielu poprawek, żeby to można było uznać za świetne. Warto jest jednak pobrać u siebie ten plugin i sprawdzić jakieś ciekawe przypadki logowania/wylogowania normalnie i przez serwisy połączone. Będę testować jeszcze. |
|
No jest lepiej, teraz nie wysypało mi się na zmianie hasła np. Odświeżenie już nie działa, też dobrze. Fajnie że udało się dodać na edycję konta odesłanie, ale teraz zauważyłem, że menu też się sypie. Tu mamy mój profil, ktoś klika tutaj: Nie wiem na jakim dokładnie jesteś etapie implementacji, bo może o tym wszystkim wiesz, ale zaobserwowałem to napiszę dla pewności. Zastanawia mnie trochę ten kod używany do logowania, tzn. ten który jest przesyłany w adresie np. |
- kod logowania ma teraz 32 znaki - dodalem kolumne z czasem wygenerowania kodu - poprawki w menu
- kod logowania usuwa sie po zalogowaniu, tak samo usuwa sie login created do kodu
Autoryzacja dwuetapowa z PRa #147 , dodałem trochę rzeczy, na razie oznaczam jako Draft bo moja aplikacja generuje inne kody niż chce plugin (natomiast kody zapasowe i generownaie działa dobrze), możliwe że to przez różnicę czasu albo coś innego. Później popatrzę.
Zastanawiam się nad dodaniem przycisku wyłączającego 2fa dla danego użytkownika, który jest widoczny tylko dla administratorów (w razie zgubienia telefonu i kodu zapasowego, po ręcznej weryfikacji będzie można bez ingerencji w bazę danych wyłączyć 2fa)
kod potwierdzający założenie qr code
ułatwienia w composerze
konkretna wersja w composerze dla vendora 2fa google
zmiany w overwrites
zabezpieczenie formularza (ewentualnie zamienienie go)
wygląda na to, że nie ma sprawdzenia czy już kod jest wygenerowany czy nie.
hasło przy zakładaniu 2fa
kod z aplikacji przy wyłączaniu 2fa
Gdy wyświetla się ten komunikat o tym, że ktoś się zalogował kodem awaryjnym to jednocześnie nadal nie jest zalogowany - po prawej nie widać jego awatara, tylko dalej ikonę logowania.
Przy zmianie hasła dostałem błąd