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

Automatické odhlašování a drobné úpravy závislostí #147

Merged
merged 5 commits into from
Jul 6, 2021

Conversation

frenzymadness
Copy link
Owner

SSIA

@frenzymadness frenzymadness linked an issue Jun 26, 2021 that may be closed by this pull request
@adelpopelkova
Copy link
Collaborator

Super, vypadá to dobře, odhlášení funguje. Jedna věc, která mě tak napadla při testování, zda by nebylo dobré informovat o tom, že uživatel byl odhlášen, protože poté, co někam klikne a je odhlášen, vyskočí stránka s chybou (401 Unauthorized), což je očekávané, ale nevím, zda to náhodou někoho nezmate. (Případně můžeme mergnout tento PR a přidat to jako issue s nice2have label.)

@nappex
Copy link
Collaborator

nappex commented Jul 4, 2021

Super, vypadá to dobře, odhlášení funguje. Jedna věc, která mě tak napadla při testování, zda by nebylo dobré informovat o tom, že uživatel byl odhlášen, protože poté, co někam klikne a je odhlášen, vyskočí stránka s chybou (401 Unauthorized), což je očekávané, ale nevím, zda to náhodou někoho nezmate. (Případně můžeme mergnout tento PR a přidat to jako issue s nice2have label.)

Dobrý postřeh, mě napadá, že by se odhlášenému uživateli neměly zobrazovat tlačítka, které nemůže použít.

@frenzymadness
Copy link
Owner Author

Super, vypadá to dobře, odhlášení funguje. Jedna věc, která mě tak napadla při testování, zda by nebylo dobré informovat o tom, že uživatel byl odhlášen, protože poté, co někam klikne a je odhlášen, vyskočí stránka s chybou (401 Unauthorized), což je očekávané, ale nevím, zda to náhodou někoho nezmate. (Případně můžeme mergnout tento PR a přidat to jako issue s nice2have label.)

Dobrý postřeh, mě napadá, že by se odhlášenému uživateli neměly zobrazovat tlačítka, které nemůže použít.

Tohle bez složitějšího JS nepůjde. Jde o to, že máš stránku, která se v prohlížeči dynamicky nemění a tobě vyprší přihlášení a pak by musel JS nějak schovat tlačítka, která nemáš vidět.
Lepší s jednodušší bude připravti stránku pro HTTP chybu 401, která ti prostě řekne, ať se znovu přihlásíš. Tahle chyba se ukáže jen těm, kteří budou delší dobu neaktivní nebo polezou někam, kam nemají.

@frenzymadness
Copy link
Owner Author

Přidal jsem samostatnou stránku pro HTTP chybu 401.

Copy link
Collaborator

@adelpopelkova adelpopelkova left a comment

Choose a reason for hiding this comment

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

Přidal jsem samostatnou stránku pro HTTP chybu 401.

Super, tak jsem si to i původně představovala.

registry/templates/401.html Outdated Show resolved Hide resolved
@nappex
Copy link
Collaborator

nappex commented Jul 5, 2021

Super, vypadá to dobře, odhlášení funguje. Jedna věc, která mě tak napadla při testování, zda by nebylo dobré informovat o tom, že uživatel byl odhlášen, protože poté, co někam klikne a je odhlášen, vyskočí stránka s chybou (401 Unauthorized), což je očekávané, ale nevím, zda to náhodou někoho nezmate. (Případně můžeme mergnout tento PR a přidat to jako issue s nice2have label.)

Dobrý postřeh, mě napadá, že by se odhlášenému uživateli neměly zobrazovat tlačítka, které nemůže použít.

Tohle bez složitějšího JS nepůjde. Jde o to, že máš stránku, která se v prohlížeči dynamicky nemění a tobě vyprší přihlášení a pak by musel JS nějak schovat tlačítka, která nemáš vidět.
Lepší s jednodušší bude připravti stránku pro HTTP chybu 401, která ti prostě řekne, ať se znovu přihlásíš. Tahle chyba se ukáže jen těm, kteří budou delší dobu neaktivní nebo polezou někam, kam nemají.

No a nešlo by ho prostě jen redirectovat po odhlášení do stavu, v kterém jsme když aplikaci spustíme poprvé ? V tomto bodě vidíme prostě jen přihlašovací okno a nic víc + přidat nějaký flash message, že byl uživatel odhlášen

@frenzymadness
Copy link
Owner Author

V posledním commitu jsem předělal chybové stránky na flash messages.

@frenzymadness
Copy link
Owner Author

No a nešlo by ho prostě jen redirectovat po odhlášení do stavu, v kterém jsme když aplikaci spustíme poprvé ? V tomto bodě vidíme prostě jen přihlašovací okno a nic víc + přidat nějaký flash message, že byl uživatel odhlášen

Nějak by to určitě šlo, ale nebude to "prostě jen" 😄 Napadají mě dvě možnosti:

  1. Stránka bude mít v JS jednoduchý odpočet a po daném čase, který bude stejný jako možná délka neaktivity, se provede redirect a ukáže se hláška o odhlášení. Odpočet by se nuloval při každém refreshi (automaticky, protože by JS se spustil znovu) a také při kliku do stránky, který nezpůsobí refresh (např. při práci s přehledem dárců, kde se stránka jako celek nenačítá znovu).
  2. Stránka bude nějak jednoduše pomocí JS pingat server a když zjistí, že session expirovala, provede se redirect. Tady si ale nejsem jist, zda každý požadavek na server odeslaný pomocí JS nepovede k prodloužení session. I když snad by to mohlo fungovat a JS na stránce by automaticky neměl obnovovat její cookies.

@adelpopelkova
Copy link
Collaborator

V posledním commitu jsem předělal chybové stránky na flash messages.

Přesměrování funguje, flash messages se také objeví.

@nappex
Copy link
Collaborator

nappex commented Jul 6, 2021

V posledním commitu jsem předělal chybové stránky na flash messages.

Přesměrování funguje, flash messages se také objeví.

Mě to také funguje. Já si myslím, že to takhle může zůstat, tvé řešení se mi líbí. Můj nápad na automatický redirect by to celé zbytečně zesložitěl a výsledek by to dramaticky nezlepšilo. Za mě tedy, nechme to, jak jsi to udělal.
Automatický redirect můžeme mít něco jak nice2have do budoucna.

@frenzymadness
Copy link
Owner Author

Chtěl jsem zkusit implementaci automatického redirectu, ale narazil jsem ještě na jeden problém a to jak ověřovat platné přihlášení jen pokud je uživatel přihlášen a jinak to nedělat.

Nechám si to na jindy. Teď jsem vyřešil konflikt a mergnu to, jakmile bude CI zelené.

@frenzymadness frenzymadness merged commit a7b3ea3 into master Jul 6, 2021
@frenzymadness frenzymadness deleted the session_expiration branch July 6, 2021 20:53
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.

Session cookie expirace
3 participants