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

Add telegram auth to frontend #1107

Merged
merged 10 commits into from
Feb 9, 2022
Merged

Add telegram auth to frontend #1107

merged 10 commits into from
Feb 9, 2022

Conversation

Ksinia
Copy link
Contributor

@Ksinia Ksinia commented Aug 17, 2021

Screenshot 2021-11-07 at 15 32 33

Screenshot 2021-11-07 at 15 31 32

Fixes #707.

@Ksinia Ksinia requested a review from umputun as a code owner August 17, 2021 20:07
@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #1107 (c9b1fb4) into master (988391b) will increase coverage by 0.53%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1107      +/-   ##
==========================================
+ Coverage   47.91%   48.45%   +0.53%     
==========================================
  Files         136      135       -1     
  Lines        2980     3042      +62     
  Branches      667      680      +13     
==========================================
+ Hits         1428     1474      +46     
- Misses       1542     1558      +16     
  Partials       10       10              
Impacted Files Coverage Δ
frontend/app/common/api.ts 70.45% <0.00%> (-3.97%) ⬇️
frontend/app/common/types.ts 100.00% <ø> (ø)
...end/app/components/auth/components/oauth.consts.ts 100.00% <ø> (ø)
frontend/app/components/auth/auth.api.ts 15.90% <12.12%> (-11.37%) ⬇️
frontend/app/components/auth/auth.tsx 83.33% <76.92%> (-2.84%) ⬇️
frontend/app/components/auth/auth.hooks.ts 97.18% <96.87%> (-0.44%) ⬇️
frontend/app/components/auth/auth.messsages.ts 100.00% <100.00%> (ø)
frontend/app/components/auth/components/oauth.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dbc36e...c9b1fb4. Read the comment docs.

@umputun umputun requested a review from akellbl4 August 17, 2021 21:11
@coveralls
Copy link

coveralls commented Aug 17, 2021

Pull Request Test Coverage Report for Build 1350074803

  • 19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 83.842%

Totals Coverage Status
Change from base Build 1349477083: 0.04%
Covered Lines: 5853
Relevant Lines: 6981

💛 - Coveralls

@paskal paskal added this to the v1.9 milestone Aug 17, 2021
@akellbl4
Copy link
Collaborator

akellbl4 commented Aug 20, 2021

@Ksinia thank you for your contribution!
I need a bit more time to review it. I would test it and maybe propose a design for this feature.
I think Telegram should be shown as other third-party providers but we need to add special flow.

btw, the code looks good.

Copy link
Collaborator

@akellbl4 akellbl4 left a comment

Choose a reason for hiding this comment

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

Hey @Ksinia,
I'd propose a visual design like this
Telegram is a third-party auth provider and I'd expect it in the line with others. Another reason we can't put the full name of the provider on the tabs.

CleanShot 2021-08-29 at 17 01 14@2x

@paskal
Copy link
Sponsor Collaborator

paskal commented Nov 7, 2021

@akellbl4 default mock render wrapper for tests don't set up state and fail because of that, would you be so kind as to look into it? The wrapper needs to be rewritten to inject state as well as other places will benefit from it as well.

frontend/app/components/auth/components/oauth.tsx Outdated Show resolved Hide resolved
frontend/app/components/auth/components/oauth.tsx Outdated Show resolved Hide resolved
frontend/package.json Outdated Show resolved Hide resolved
@elmaester
Copy link
Contributor

image
There's a typo and the checks are failing. Or are there other more difficult to fix reasons? I would really like this feature to work as soon as possible ☺️ Let me know if I can help

@elmaester
Copy link
Contributor

@Ksinia can I pray to you so that you may please finish this feature? ☺️ Where is the altar? 😁 I want it to work so badly 😄

@umputun
Copy link
Owner

umputun commented Jan 13, 2022

@Ksinia can I pray to you so that you may please finish this feature? ☺️ Where is the altar? 😁 I want it to work so badly 😄

I wonder, what is your use case and why is one more auth method that important to you? As you probably know, there are many other auth methods currently, and to me TG auth seems like an exotic way to authenticate into remark42.

@elmaester
Copy link
Contributor

Telegram is popular among my target audience, and personally I feel more comfortable giving my or users' data to Telegram as opposed to Google. GitHub is not used by non-developers. Twitter is silly with its account policies. Facebook I don't use, so can't create the integration. And it's one of the worst privacy offenders anyway. So Telegram is the best option really. And I have Google and GitHub active already.

@umputun
Copy link
Owner

umputun commented Jan 13, 2022

I feel more comfortable giving my or users' data to Telegram as opposed to Google.

I'm not really sure what data you giving to google/fb/github in this particular case. Auth works in the opposite way and does not communicate with auth providers outside of the basic oauth(2) flow. In fact, this is google giving users' data (very basic data) to remark42. I mean, remark42 expects users with existing accounts on one of the popular places, and all those places know about the user is known regardless of remark42 usage.

btw, if such "anti-social login" is important, you may consider email auth

@elmaester
Copy link
Contributor

elmaester commented Jan 13, 2022

btw, if such "anti-social login" is important, you may consider email auth

yeah, but configuring an email server is kinda hard. it's on my to-do list, but feels like a big deal :)

giving my or users' data

I guess I misspoke. more accurately I should say I'm reluctant to impose a requirement on my users to have an account with such a service. I appreciate the freedom not to. and also it's a question of which services I depend on (with my personal account they control) to keep my site functioning, comment attribution consistent, etc. I have more trust in the sanity of Telegram than alternatives. and besides there's extra utility: users will be able to subscribe to comment threads with THEIR Telegram accounts, right?

Also since we're having a conversation, I was wondering if there's any admin dashboard in Remark42 which I somehow haven't discovered yet? Can all comments be exported as JSON? how?

@umputun
Copy link
Owner

umputun commented Jan 13, 2022

I was wondering if there's any admin dashboard in Remark42 which I somehow haven't discovered yet?

see https://github.com/umputun/remark42/wiki/Admin-UI

Can all comments be exported as JSON? how?

see API docs: https://remark42.com/docs/contributing/api/

Ksinia and others added 9 commits February 7, 2022 23:33
Bumps [nanoid](https://github.com/ai/nanoid) from 3.1.23 to 3.2.0.
- [Release notes](https://github.com/ai/nanoid/releases)
- [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md)
- [Commits](ai/nanoid@3.1.23...3.2.0)

---
updated-dependencies:
- dependency-name: nanoid
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
- get rid of redux store in favor of local state
- add tests for telegram happy path
- lift auth handling on Auth level
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

Thx a lot!

@umputun umputun merged commit 74b47d3 into umputun:master Feb 9, 2022
@elmaester
Copy link
Contributor

Cool! Thanks a lot! When do those changes make it to the docker image? They haven't yet, as far as I can tell.

@umputun
Copy link
Owner

umputun commented Feb 10, 2022

the image was built automatically. each commit/merge to master triggers the build of a new :master image

@elmaester
Copy link
Contributor

elmaester commented Feb 10, 2022

Ok, then please advise. I did docker-compose pull, and :master image is specified in the .yml, but for me the old image has then loaded with docker-compose up. What should I do differently?

@umputun
Copy link
Owner

umputun commented Feb 10, 2022

well, something is not right with your compose I guess. I just did this:

docker pull  umputun/remark42:master
docker run -it --rm umputun/remark42:master /srv/remark42 server -h

....
remark42 master-6fd7306-20220209T11:07:04

as you can see this is a very fresh version built on 2022/02/09

@elmaester
Copy link
Contributor

Okay, yeah, I got the same output, running latest version then. But here's what the UI looks like:

image

Is it supposed to look like that? And it doesn't work either.
In my yml I've set AUTH_TELEGRAM=true
Is a different configuration necessary?

@umputun
Copy link
Owner

umputun commented Feb 10, 2022

Is a different configuration necessary?

check the docks please, here, see telegram section

@elmaester
Copy link
Contributor

I have that. I'm receiving notifications to my Telegram channel, that part works. Auth does not

@umputun
Copy link
Owner

umputun commented Feb 10, 2022

yeah, the same here. Just added a bot and allowed tg auth on https://remark42.com/demo/ and nothing good happened

@Ksinia @akellbl4 @paskal - something is not right I think

@paskal
Copy link
Sponsor Collaborator

paskal commented Feb 10, 2022

The current master image has build info master-6fd7306-20220209T11:07:23, which is one commit behind the master. We'll fix the build and Telegram auth will become available. ETA 12h.

@umputun
Copy link
Owner

umputun commented Feb 10, 2022

i have forces rebuild and deployed new :master, seems to work

@umputun
Copy link
Owner

umputun commented Feb 10, 2022

I think I spoke too soon. I was able to authenticate once, but after logout in all my attempts to auth I'm getting mysterious "error:0
frw5x-202202-10113850-qiuzk
"

@umputun
Copy link
Owner

umputun commented Feb 10, 2022

actually, it does work, but telegram ui is confusing as hell. @paskal - I think we need to add to docs "press Start button in telegram"

Because I pressed it before I have missed the button the second time

@elmaester
Copy link
Contributor

Another issue:
doesn't fit
The popup overflows below the bottom of the webpage, and anything below is not reachable for viewing or clicking.
Maybe depends on page width. Or it appeared after I did

document.getElementsByClassName("auth-submit")[0].click()

image
Either way, clicking on Check got me the error.0 an nothing else.

@umputun
Copy link
Owner

umputun commented Feb 11, 2022

have you followed the instruction and clicked the link above first? And after this "Start" in tg client?

@elmaester
Copy link
Contributor

Ok, yeah, I was missing the step in the tg client. Authenticated now. There was no notification about any necessary interaction from the tg client if it is minimized. A user would never know. I wonder if anything can be done to make the process more intuitive.
Also, the overflow is a serious issue. One must not assume that the page will have sufficient extra space below for the entire popup to fit. In my case it doesn't, until I manage to click the button with a console command. There was no other way to click the button as it was outside the view and not reachable. Seems like a styling issue.
And another styling thing:
image
When the window is narrow, this place could use a margin.

@elmaester
Copy link
Contributor

elmaester commented Feb 11, 2022

As a last resort, some extra explanatory text here could help:
image

@umputun
Copy link
Owner

umputun commented Feb 11, 2022

And another styling thing:

let's keep this PR discussion about tg auth issues

@elmaester
Copy link
Contributor

elmaester commented Feb 11, 2022

Come to think of it, the error.0 text could be made more descriptive too. Perhaps instructions to interact with the tg client could go there. (Users will not read the documentation)

@paskal
Copy link
Sponsor Collaborator

paskal commented Feb 13, 2022

image

Now both issues (with hidden window and error.0) are fixed, here is what the Check button says if you haven't clicked Start in Telegram after clicking the link or scanning the QR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Telegram Login
6 participants