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

SSO Support (also needed for ElementOne) #24

Closed
ceearrbee opened this issue Aug 13, 2021 · 60 comments
Closed

SSO Support (also needed for ElementOne) #24

ceearrbee opened this issue Aug 13, 2021 · 60 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@ceearrbee
Copy link

SSO as the login method for servers is becoming popular - such as with Mozilla's, is there any likelihood of this being implemented anytime soon?

@alphapapa
Copy link
Owner

Hi,

I don't use SSO, so I don't plan to implement it myself (unless someone wanted to sponsor it in some way). But, patches welcome, and I'll be glad to help integrate it. :)

@alphapapa alphapapa added enhancement New feature or request help wanted Extra attention is needed labels Aug 13, 2021
@Necronian
Copy link

After talking to alphapapa in matrix I wrote the following which could be a helpful jumping off point for integrating sso. According to https://matrix.org/docs/guides/sso-for-client-developers the server should support m.login.sso and m.login.token.

(defun necronian-ement-sso-api (data)
  (let* ((id (cdr (assq 'user_id data)))
         (username (save-match-data
                     (string-match "@\\(.*\\):.*" id)
                     (match-string 1 id)))
         (server (cdr (assq 'home_server data)))
         (uri-prefix (cdr (assq 'base_url
                                (cdr (assq 'm\.homeserver
                                           (cdr (assq 'well_known data)))))))
         (token (cdr (assq 'access_token data))))
    (ement-connect
     :session (make-ement-session
               :user (make-ement-user :id id :username username)
               :server (make-ement-server :name server
                                          :uri-prefix uri-prefix)
               :token token
               :events (make-hash-table :test #'equal)
               :transaction-id 0))))

(defun sso-login (uri-prefix proc msg)
  (let* ((token (save-match-data
                  (string-match "GET /\\?loginToken=\\(.*\\)\s.*" msg)
                  (match-string 1 msg)))
         (login-data (ement-alist "type" "m.login.token"
                                  "token" token)))
    (ement-api (make-ement-session
                :server (make-ement-server :uri-prefix uri-prefix))
      "login"
      :then #'necronian-ement-sso-api
      :method 'post
      :data (json-encode login-data))
    (delete-process "ement-sso")))

(defun necronian-ement-sso (uri-prefix &optional local-port)
  (make-network-process
   :name "ement-sso"
   :family 'ipv4
   :host "localhost"
   :service (or local-port 4567)
   :filter (apply-partially #'sso-login uri-prefix)
   :server t)
  (browse-url (concat uri-prefix
                      "/_matrix/client/r0/login/sso/redirect?redirectUrl=http://localhost:"
                      (number-to-string (or local-port 4567)))))

Calling (necronian-ement-sso "<homeserver-url>") should open a browser and navigate to your sso page. Log in as you would usually and ement should log in to your server.

@alphapapa
Copy link
Owner

Thanks, that is very helpful.

@alphapapa alphapapa self-assigned this Aug 15, 2021
@alphapapa alphapapa removed the help wanted Extra attention is needed label Aug 15, 2021
@wehlutyk
Copy link

This looks great! Does it have a chance of working through pantalaimon? My understanding is pantalaimon handles the login when I use it, as I don't inform ement of the home server pantalaimon is proxying to.

@alphapapa
Copy link
Owner

This looks great! Does it have a chance of working through pantalaimon? My understanding is pantalaimon handles the login when I use it, as I don't inform ement of the home server pantalaimon is proxying to.

I have no idea, since I don't use Pantalaimon myself. If you could find out and report back, that would be helpful. :)

@alphapapa alphapapa changed the title SSO Support SSO Support (also needed for ElementOne) Mar 16, 2022
@oneingan
Copy link

oneingan commented Apr 17, 2022

Trying this with (necronian-ement-sso "https://one.element.io")

and after sign-in into browser, emacs fails with:

#<process firefox https://one.element.io/_matrix/client/r0/login/sso/redirect?redirectUrl=http://localhost:4567>
Ement: Sync request sent, waiting for response...
Ement: Response arrived after 10.88 seconds.  Reading 2.5M JSON response...
Ement: Reading JSON took 0.85 seconds
Ement: Reading events...66% 
Ement: Sync request sent, waiting for response...
error in process sentinel: let: HTTP error
error in process sentinel: HTTP error
error in process sentinel: let: HTTP error
error in process sentinel: HTTP error [59 times]
Ement: Response arrived after 30.22 seconds.  Reading 751 JSON response...
Ement: Reading JSON took 0.00 seconds
Ement: Reading events... 
Ement: Sync request sent, waiting for response...
error in process sentinel: run-hook-with-args: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: HTTP error [58 times]
Ement: Response arrived after 30.18 seconds.  Reading 751 JSON response...
Ement: Reading JSON took 0.00 seconds
Ement: Reading events... 
Ement: Sync request sent, waiting for response...
error in process sentinel: run-hook-with-args: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: HTTP error [30 times]
error in process sentinel: Symbol’s function definition is void: ement-room-list-auto-update [2 times]
(...)

can you point me how to debug this error?
ping: @Necronian

@Necronian
Copy link

Honestly, my elisp is not very good so I'm not sure if I can help.

To start this code will always be brittle since ement doesn't really have an interface for users to create/initialize a new session. We create the session struct ourselves and need to make sure we initialize it correctly with the information ement is expecting. That has changed slightly since my last post. This is what I'm using now, which I know fixes some issues.

(defun necronian-ement-sso-api (data)
  (let* ((id (cdr (assq 'user_id data)))
         (username (save-match-data
                     (string-match "@\\(.*\\):.*" id)
                     (match-string 1 id)))
         (server (cdr (assq 'home_server data)))
         (uri-prefix (cdr (assq 'base_url
                                (cdr (assq 'm\.homeserver
                                           (cdr (assq 'well_known data)))))))
         (token (cdr (assq 'access_token data))))
    (ement-connect
     :session (make-ement-session
               :user (make-ement-user :id id
                                      :username username
                                      :room-display-names (make-hash-table))
               :server (make-ement-server :name server
                                          :uri-prefix uri-prefix)
               :token token
               :events (make-hash-table :test #'equal)
               :transaction-id (ement--initial-transaction-id)))))

(defun sso-login (uri-prefix proc msg)
  (let* ((token (save-match-data
                  (string-match "GET /\\?loginToken=\\(.*\\)\s.*" msg)
                  (match-string 1 msg)))
         (login-data (ement-alist "type" "m.login.token"
                                  "token" token)))
    (ement-api (make-ement-session
                :server (make-ement-server :uri-prefix uri-prefix))
      "login"
      :then #'necronian-ement-sso-api
      :method 'post
      :data (json-encode login-data))
    (delete-process "ement-sso")))

(defun necronian-ement-sso (uri-prefix &optional local-port)
  (make-network-process
   :name "ement-sso"
   :family 'ipv4
   :host "localhost"
   :service (or local-port 4567)
   :filter (apply-partially #'sso-login uri-prefix)
   :server t)
  (browse-url (concat uri-prefix
                      "/_matrix/client/r0/login/sso/redirect?redirectUrl=http://localhost:"
                      (number-to-string (or local-port 4567)))))

@oneingan
Copy link

Thank you so much for your fast reply. Sadly your updated SSO code is not working for me neither. At least I expect to revive this thread and some elisp expert could help in the future. I'll keep trying.

@alphapapa
Copy link
Owner

alphapapa commented Apr 19, 2022 via email

@oneingan
Copy link

oneingan commented Apr 19, 2022

Sure, I run (necronian-ement-sso "https://one.element.io") again with new excerpt of code shared by @Necronian and after login in browser I get same message error than before:

error in process sentinel: run-hook-with-args: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: HTTP error [30 times]

By the way I'll be happy to help debugging this in-depth.

@alphapapa
Copy link
Owner

alphapapa commented Apr 19, 2022 via email

@oneingan
Copy link

oneingan commented Apr 19, 2022

OK, I will try to be more clear. Reproducing the problem in a pristine emacs environment emacs -Q. I evaluate this code:

(add-to-list 'load-path "/home/user/.emacs.d/contrib-lisp/ement")
(add-to-list 'load-path "/home/user/.emacs.d/contrib-lisp/plz")
(require 'ement)

(defun necronian-ement-sso-api (data)
  (let* ((id (cdr (assq 'user_id data)))
         (username (save-match-data
                     (string-match "@\\(.*\\):.*" id)
                     (match-string 1 id)))
         (server (cdr (assq 'home_server data)))
         (uri-prefix (cdr (assq 'base_url
                                (cdr (assq 'm\.homeserver
                                           (cdr (assq 'well_known data)))))))
         (token (cdr (assq 'access_token data))))
    (ement-connect
     :session (make-ement-session
               :user (make-ement-user :id id
                                      :username username
                                      :room-display-names (make-hash-table))
               :server (make-ement-server :name server
                                          :uri-prefix uri-prefix)
               :token token
               :events (make-hash-table :test #'equal)
               :transaction-id (ement--initial-transaction-id)))))

(defun sso-login (uri-prefix proc msg)
  (let* ((token (save-match-data
                  (string-match "GET /\\?loginToken=\\(.*\\)\s.*" msg)
                  (match-string 1 msg)))
         (login-data (ement-alist "type" "m.login.token"
                                  "token" token)))
    (ement-api (make-ement-session
                :server (make-ement-server :uri-prefix uri-prefix))
      "login"
      :then #'necronian-ement-sso-api
      :method 'post
      :data (json-encode login-data))
    (delete-process "ement-sso")))

(defun necronian-ement-sso (uri-prefix &optional local-port)
  (make-network-process
   :name "ement-sso"
   :family 'ipv4
   :host "localhost"
   :service (or local-port 4567)
   :filter (apply-partially #'sso-login uri-prefix)
   :server t)
  (browse-url (concat uri-prefix
                      "/_matrix/client/r0/login/sso/redirect?redirectUrl=http://localhost:"
                      (number-to-string (or local-port 4567)))))

being ~/.emacs.d/contrib-lisp/ement and ~/.emacs.d/contrib-lisp/plz the place where i download git third party packages.

Then i run (necronian-ement-sso "https://one.element.io"), firefox new tab opens and I login in ElementOne site authorizing localhost. Browser hangs and Emacs Messages buffers prints:

#<process firefox https://one.element.io/_matrix/client/r0/login/sso/redirect?redirectUrl=http://localhost:4567>
Ement: Sync request sent, waiting for response...
Ement: Response arrived after 11.87 seconds.  Reading 2.6M JSON response...
Ement: Reading JSON took 0.84 seconds
Ement: Reading events...62% 
Ement: Sync request sent, waiting for response...
error in process sentinel: let: HTTP error
error in process sentinel: HTTP error
error in process sentinel: let: HTTP error
error in process sentinel: HTTP error
error in process sentinel: let: HTTP error
error in process sentinel: HTTP error
error in process sentinel: let: HTTP error
error in process sentinel: HTTP error
error in process sentinel: let: HTTP error
error in process sentinel: HTTP error
error in process sentinel: let: HTTP error
error in process sentinel: HTTP error
error in process sentinel: let: HTTP error
error in process sentinel: HTTP error [57 times]
Ement: Response arrived after 30.27 seconds.  Reading 751 JSON response...
Ement: Reading JSON took 0.00 seconds
Ement: Reading events... 
Ement: Sync request sent, waiting for response...
error in process sentinel: run-hook-with-args: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: HTTP error [60 times]
Ement: Response arrived after 30.41 seconds.  Reading 751 JSON response...
Ement: Reading JSON took 0.00 seconds
Ement: Reading events... 
Ement: Sync request sent, waiting for response...
error in process sentinel: run-hook-with-args: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: HTTP error [4 times]
error in process sentinel: Quit [8 times]
error in process sentinel: HTTP error [4 times]
error in process sentinel: Quit [2 times]
error in process sentinel: HTTP error [6 times]
error in process sentinel: Symbol’s function definition is void: ement-room-list-auto-update [2 times]
Ement: Response arrived after 30.21 seconds.  Reading 751 JSON response...
Ement: Reading JSON took 0.00 seconds
Ement: Reading events... 
Ement: Sync request sent, waiting for response...
error in process sentinel: run-hook-with-args: Symbol’s function definition is void: ement-room-list-auto-update
error in process sentinel: Symbol’s function definition is void: ement-room-list-auto-update

I'm running "GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.30, cairo version 1.16.0)"

@alphapapa
Copy link
Owner

alphapapa commented Apr 20, 2022 via email

@oneingan
Copy link

oneingan commented Apr 20, 2022

Ok, I evaluate this now and its working well:

(unless (package-installed-p 'quelpa)
  (with-temp-buffer
    (url-insert-file-contents "https://raw.githubusercontent.com/quelpa/quelpa/master/quelpa.el")
    (eval-buffer)
    (quelpa-self-upgrade)))

(quelpa
 '(quelpa-use-package
   :fetcher git
\   :url "https://github.com/quelpa/quelpa-use-package.git"))
(require 'quelpa-use-package)

;; Install `plz' HTTP library (not on MELPA yet).
(use-package plz
  :quelpa (plz :fetcher github :repo "alphapapa/plz.el"))

;; Install Ement.
(use-package ement
	     :quelpa (ement :fetcher github :repo "alphapapa/ement.el"))
(require 'ement)

(defun necronian-ement-sso-api (data)
  (let* ((id (cdr (assq 'user_id data)))
         (username (save-match-data
                     (string-match "@\\(.*\\):.*" id)
                     (match-string 1 id)))
         (server (cdr (assq 'home_server data)))
         (uri-prefix (cdr (assq 'base_url
                                (cdr (assq 'm\.homeserver
                                           (cdr (assq 'well_known data)))))))
         (token (cdr (assq 'access_token data))))
    (ement-connect
     :session (make-ement-session
               :user (make-ement-user :id id
                                      :username username
                                      :room-display-names (make-hash-table))
               :server (make-ement-server :name server
                                          :uri-prefix uri-prefix)
               :token token
               :events (make-hash-table :test #'equal)
               :transaction-id (ement--initial-transaction-id)))))

(defun sso-login (uri-prefix proc msg)
  (let* ((token (save-match-data
                  (string-match "GET /\\?loginToken=\\(.*\\)\s.*" msg)
                  (match-string 1 msg)))
         (login-data (ement-alist "type" "m.login.token"
                                  "token" token)))
    (ement-api (make-ement-session
                :server (make-ement-server :uri-prefix uri-prefix))
      "login"
      :then #'necronian-ement-sso-api
      :method 'post
      :data (json-encode login-data))
    (delete-process "ement-sso")))

(defun necronian-ement-sso (uri-prefix &optional local-port)
  (make-network-process
   :name "ement-sso"
   :family 'ipv4
   :host "localhost"
   :service (or local-port 4567)
   :filter (apply-partially #'sso-login uri-prefix)
   :server t)
  (browse-url (concat uri-prefix
                      "/_matrix/client/r0/login/sso/redirect?redirectUrl=http://localhost:"
                      (number-to-string (or local-port 4567)))))

Thanks everyone for your effort.

@alphapapa
Copy link
Owner

@uningan Thanks, that's great.

@Necronian It looks like your code works. I'd guess it's sufficiently trivial (i.e. there's no other obvious way to implement it) that copyright could hardly be an issue, but on the other hand, the FSF's policies are what they are (i.e. no more than 15 lines of changes without copyright assignment. Since I'm planning to submit Ement to GNU ELPA, which requires FSF CA, are you willing to pursue that, so this code could be accepted as-is in a patch?

@Necronian
Copy link

I'll try to get a FSF CA. Honestly it feels a bit silly for something so trivial, but sure.

I've always wanted to eventually integrate it into ement-connect. Possibly when a blank password is entered and m.login.sso is supported start the sso process.

What I have now has been working so I just haven't felt a huge need.

@alphapapa
Copy link
Owner

I'll try to get a FSF CA. Honestly it feels a bit silly for something so trivial, but sure.

Thanks. I'm trying to go by the book here so that there won't be any troubles in the future (which are always much harder to resolve after the fact).

I've always wanted to eventually integrate it into ement-connect. Possibly when a blank password is entered and m.login.sso is supported start the sso process.

Probably we'll need to check the server's supported methods and prompt the user for the one to use.

@oneingan
Copy link

oneingan commented Jun 2, 2022

Not working for me again, any hints?

#<process firefox https://one.element.io/_matrix/client/r0/login/sso/redirect?redirectUrl=http://localhost:4567>
error in process sentinel: cond: Keyword argument :room-display-names not one of (:id :displayname :account-data :color :message-color :username :avatar-url :avatar)
error in process sentinel: Keyword argument :room-display-names not one of (:id :displayname :account-data :color :message-color :username :avatar-url :avatar)

stil ok on you @Necronian ?

@alphapapa
Copy link
Owner

alphapapa commented Jun 2, 2022

Not working for me again, any hints?

#<process firefox https://one.element.io/_matrix/client/r0/login/sso/redirect?redirectUrl=http://localhost:4567>
error in process sentinel: cond: Keyword argument :room-display-names not one of (:id :displayname :account-data :color :message-color :username :avatar-url :avatar)
error in process sentinel: Keyword argument :room-display-names not one of (:id :displayname :account-data :color :message-color :username :avatar-url :avatar)

stil ok on you @Necronian ?

That code does not exist in Ement.el anymore. You need to remove the setting of the :room-display-names argument in his code.

@h0cheung

This comment was marked as off-topic.

@alphapapa

This comment was marked as off-topic.

@h0cheung

This comment was marked as off-topic.

@alphapapa

This comment was marked as off-topic.

@h0cheung

This comment was marked as off-topic.

@ceearrbee
Copy link
Author

I was also able to get an account on a server that uses SSO, so I should be able to develop this feature myself when I have time.

FWIW, I have since lost the information for that account, so I'm still unable to test SSO myself. If anyone can offer a temporary account on a homeserver that provides SSO logins, please let me know.

Mozilla offers a free Matrix server that requires SSO - you can find more information at https://chat.mozilla.org .

alphapapa added a commit that referenced this issue May 30, 2023
See <#24>.

Co-developed-by: Jeffrey Stoffers <jstoffers@uberpurple.com>
@alphapapa
Copy link
Owner

@ceearrbee Thanks, that helped a lot.

@phil-s and anyone else who's interested: the wip/sso branch currently seems to work on the mozilla.org server. Please note that I had to make an update to plz.el to improve support for HTTP redirects, which solves the error request for .well-known URI did not return a ‘plz’ response. That version of plz is not yet tagged, so if you are getting that error, you'll need to install plz from master or ELPA-devel. That new version of plz will likely be tagged this week.

alphapapa added a commit that referenced this issue May 30, 2023
Closes <#24>.

Co-developed-by: Jeffrey Stoffers <jstoffers@uberpurple.com>
@alphapapa
Copy link
Owner

alphapapa commented May 30, 2023

I went ahead and merged to master. Testing would be appreciated. Maybe a v0.10 release with this feature can be tagged in time for TWIM on Friday; but if not, maybe next week.

@phil-s
Copy link

phil-s commented May 30, 2023

Great, that's now mostly working!

After entering my username I do still get the Warning (emacs): Ement: ‘plz’ request for .well-known URI did not return a ‘plz’ response, but things go pretty well from that point...

It's no longer requesting a password, so I go straight to URL entry and select sso and get to my browser log-in form. I have one minor glitch here in that the submission to http://localhost:4567/?loginToken=<token> appears to fail with "Unable to connect. Firefox can’t establish a connection to the server at localhost:4567." however Emacs happily loads up all my chat rooms at that point, so it did everything it needed to do.

Regarding this [plz] error
I don't know why that's happening. The server is sending a HTTP 301 redirect, but plz calls curl with --location, which is supposed to automatically follow the redirect.

What version of plz do you have installed? What version of curl do you have? And what platform are you on?

I'm running Ubuntu 22.04 with curl 7.81.0 and the new plz commit a0a6d623352aa1caee722c16649190611a253cbc "Fix: (plz--sentinel) Skip HTTP redirect headers".

If you're willing to tell me the server name (even privately), it would make it easier for me to debug this problem, so I could make that HTTP request myself.

I'll have to ask other people about that, but I'm happy to test anything you can think of.

Tracing make-process shows me (make-process :name "plz-request-curl" :buffer #<buffer *plz-request-curl*> :coding binary :command ("curl" "--silent" "--compressed" "--location" "--config" "-") :connection-type pipe :sentinel plz--sentinel :stderr #<buffer *plz-request-curl*> :noquery nil) and I'm not sure what happens with the --config - part (presumably it's waiting for some config entry on stdin, and I didn't know what to give it), but omitting that and using the domain from my @<user>:<domain> User ID, curl gives me the following headers and the JSON response from the message quoted previously.

$ curl -I --silent --compressed --location https://<domain>/.well-known/matrix/client
HTTP/2 301 
date: Tue, 30 May 2023 15:01:44 GMT
content-type: text/html
content-length: 162
location: https://www.<domain>/.well-known/matrix/client

HTTP/2 301 
date: Tue, 30 May 2023 15:01:45 GMT
location: https://chat.<domain>/.well-known/matrix/client
cache-control: max-age=3600
expires: Tue, 30 May 2023 16:01:45 GMT
vary: Accept-Encoding
server: cloudflare
cf-ray: 7cf7daad0b33b4ab-CHC

HTTP/2 200 
server: nginx
date: Tue, 30 May 2023 15:01:45 GMT
content-type: application/json
content-length: 168
last-modified: Wed, 26 May 2021 05:14:53 GMT
etag: "60add94d-a8"
access-control-allow-origin: https://chat.<domain>
access-control-allow-methods: GET
accept-ranges: bytes
strict-transport-security: max-age=63072000

If I stop plz from from killing the *plz-request-curl* buffer, that contains:

HTTP/2 200 
server: nginx
date: Tue, 30 May 2023 15:20:11 GMT
content-type: application/json
content-length: 168
last-modified: Wed, 26 May 2021 05:14:53 GMT
etag: "60add94d-a8"
access-control-allow-origin: https://chat.<domain>
access-control-allow-methods: GET
accept-ranges: bytes
strict-transport-security: max-age=63072000

{
  "io.element.e2ee": {
    "default": false,
    "secure_backup_required": false
  },
  "im.vector.riot.jitsi": {
    "preferredDomain": "meet.<domain>"
  }
}

Process plz-request-curl stderr finished

@phil-s
Copy link

phil-s commented May 30, 2023

Ah, plz--skip-redirect-headers returns nil even when it skipped a header, so that while loop exits too early.

The plz buffer starts out like this when that's being processed:

HTTP/2 301 
date: Tue, 30 May 2023 15:36:20 GMT
content-type: text/html
content-length: 162
location: https://www.<domain>/.well-known/matrix/client

HTTP/2 301 
date: Tue, 30 May 2023 15:36:20 GMT
location: https://chat.<domain>/.well-known/matrix/client
cache-control: max-age=3600
expires: Tue, 30 May 2023 16:36:20 GMT
vary: Accept-Encoding
server: cloudflare
cf-ray: 7cf80d56c97bb4ab-CHC

HTTP/2 200 
server: nginx
date: Tue, 30 May 2023 15:36:21 GMT
content-type: application/json
content-length: 168
last-modified: Wed, 26 May 2021 05:14:53 GMT
etag: "60add94d-a8"
access-control-allow-origin: https://chat.<domain>
access-control-allow-methods: GET
accept-ranges: bytes
strict-transport-security: max-age=63072000

{
  "io.element.e2ee": {
    "default": false,
    "secure_backup_required": false
  },
  "im.vector.riot.jitsi": {
    "preferredDomain": "meet.<domain>"
  }
}

(but with carriage returns after each header line; github is stripping those.)

alphapapa added a commit to alphapapa/plz.el that referenced this issue May 30, 2023
@alphapapa
Copy link
Owner

@phil-s Another silly mistake by me, sorry. I was thinking that unless would return the value of re-search-forward when it finds a match; the perils of writing code too late at night. :) I just pushed a new commit to plz's master branch to fix that. Thanks for your thorough debugging!

I'll have to ask other people about that, but I'm happy to test anything you can think of.

I don't think that will be necessary anymore. This should fix it...

@phil-s
Copy link

phil-s commented May 31, 2023

That issue looks sorted now, thanks. I'm afraid I'm now getting a new problem -- a subsequent plz call produces this:

error in process sentinel: ement-api-error: Ement API error: "3: URL malformed. The syntax was not correct."

debug-on-entry for plz gave me this, which indeed doesn't look like a valid URL:

Debugger entered--entering a function:
  plz(get "///_matrix/client/r0/login?" :headers (("Content-Type" . "application/json")) :body nil :body-type text :as json-read :then #f(compiled-function (data) #<bytecode -0x4e795e4091fe8d2>) :else ement-api-error :connect-timeout 10 :timeout 60 :noquery t)
  apply(plz (get "///_matrix/client/r0/login?" :headers (("Content-Type" . "application/json")) :body nil :body-type text :as json-read :then #f(compiled-function (data) #<bytecode -0x4e795e4091fe8d2>) :else ement-api-error :connect-timeout 10 :timeout 60 :noquery t))
  ement-api(#s(ement-session :user #s(ement-user :id "@<user>:<domain>" :displayname nil :account-data nil :color nil :message-color nil :username "<user>" :avatar-url nil :avatar nil) :server #s(ement-server :name "<domain>" :uri-prefix nil) :token nil :transaction-id 1212214747 :rooms nil :next-batch nil :device-id "65d4b54e411e80eb98aac73cc1393c588135442196ca786cfbf6b3a6827ebe94" :initial-device-display-name "Ement.el: <user>@<localhost>" :has-synced-p nil :account-data nil :events #<hash-table equal 0/65 0x157a7fc33ffd>) "login" :then #f(compiled-function (data) #<bytecode -0x4e795e4091fe8d2>))
  ement-connect(:user-id "@<user>:<domain>")
  funcall-interactively(ement-connect :user-id "@<user>:<domain>")
  call-interactively(ement-connect record nil)
  command-execute(ement-connect record)
  execute-extended-command(nil "ement-connect" nil)
  funcall-interactively(execute-extended-command nil "ement-connect" nil)
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

I see ement-api building that URL, and the error looks like it's due to :uri-prefix nil in the session argument.

I can confirm that swapping between the current and previous revision of plz.el toggles this failure.

I haven't checked, but perhaps it's assuming that if it gets any well-known data at all, that it will include something for matrix? (In my case there is a JSON response, but nothing for matrix.)

@phil-s
Copy link

phil-s commented May 31, 2023

Correction...

I'd been guessing (wrongly) that .../.well-known/matrix/client was returning all kinds of "well known" things (in particular, I'd recognised meet.<domain> as a video conferencing platform), and so I figured that perhaps the /matrix/client part was irrelevant in this case; but I've just confirmed with curl that I don't get JSON if I drop the latter components of the URL, and I've also realised that the JSON mentions "riot" and "element" which are both Matrix terminology, so it does now seem to me that it's all targeted Matrix data.

The JSON data does not include the URL which I was previously being prompted to enter manually, though.

I don't know whether the JSON is supposed to include that, or if it should be inferred from the URL that plz was requesting, but in any case it does seem to me that ement is not finding what it expects to find where it expects to find it.

This issue aside, I'm entirely unfamiliar with any well-known URLs and behaviour, so I have no insight into whether ours is genuinely broken, or if it's just another edge case for ement to check for. (Although even if this is broken at my end, it'll be nice if ement can fall back to prompting.)

@phil-s
Copy link

phil-s commented May 31, 2023

It looks to me like it's a valid scenario.

https://spec.matrix.org/latest/client-server-api/#well-known-uri says:

Extract the base_url value from the m.homeserver property. This value is to be used as the base URL of the homeserver.
If this value is not provided, then FAIL_PROMPT.

@phil-s
Copy link

phil-s commented May 31, 2023

By comparison, I see that $ curl https://www.mozilla.org/.well-known/matrix/client gives:

{
    "m.homeserver": {
        "base_url": "https://mozilla.modular.im"
    },
    "m.identity_server": {
        "base_url": "https://vector.im"
    }
}

I'll have to raise an internal request to do similarly for the server I'm using; but that can wait until after things are working here without it :)

@alphapapa
Copy link
Owner

@phil-s Thanks again for the detailed investigation.

I'm sorry but I'm not sure I fully understand: are you saying that the JSON returned for the request to https://YOUR-MXID-HOSTNAME/.well-known/matrix/client does not include the m.homeserver key?

If it doesn't, then the homeserver is not configured correctly--a not-uncommon problem, judging from the reports I've heard from other users about using other homeservers.

Then, are you saying that Ement should be able to cope with that? Well, yes, it should "fail prompt," but apparently, yes, it's assuming that if json-read returns a value, it will include the m.homeserver key, and it's using that value to build the URL. Instead it should verify that the value appears to be an HTTP URL (or URI prefix, or whatever it's supposed to be called here), and "fail prompt" otherwise.

So, that problem isn't directly related to SSO support, but I'm glad you found it. I'll push a fix probably tomorrow. Thanks.

@phil-s
Copy link

phil-s commented May 31, 2023

Yes, all correct. The entirety of the JSON I get is shown in #24 (comment) and does not include "m.homeserver".

So, that problem isn't directly related to SSO support, but I'm glad you found it.

I'm happy to be providing lots of edge-cases prior to the feature being merged :)

I'll push a fix probably tomorrow.

Excellent, thank you. I'll re-test after that (and then see if I can get someone at my end to fix that JSON!)

alphapapa added a commit that referenced this issue May 31, 2023
@alphapapa
Copy link
Owner

alphapapa commented May 31, 2023

@phil-s Ok, it should be working now (with the current master versions of ement and plz).

Note to self: I should probably make v0.10 of Ement depend on v0.6 of plz.

@phil-s
Copy link

phil-s commented Jun 1, 2023

Thank you, that is now working nicely for me!

Should ement-connect acquire a new keyword for the authentication method? At present I can do this:

(ement-connect :user-id "@..." :uri-prefix "https://...")

But (on account of that less-than-ideal local JSON response from my Matrix server) I still have to interactively enter sso at a prompt. I think it would be nice if the potential prompts all had relevant keywords. Something like:

(ement-connect :user-id "@..." :uri-prefix "https://..." :auth-method 'sso)

I note that if I enable session saving, then (call-interactively 'ement-connect) is working without further prompting, but a new keyword still seems like a useful enhancement.

@phil-s
Copy link

phil-s commented Jun 1, 2023

The only other question I have left is wrt one of my earlier notes:

I ... select sso and get to my browser log-in form. I have one minor glitch here in that the submission to http://localhost:4567/?loginToken=<token> appears to fail with "Unable to connect. Firefox can’t establish a connection to the server at localhost:4567." however Emacs happily loads up all my chat rooms at that point, so it did everything it needed to do.

The issue being that (very much by design) that error screen looks like the opposite of success.

(I was going to include a small screenshot but github doesn't like the png for some reason; but it's a browser-generated error screen, so you'll see your version just by visiting http://localhost:4567, assuming that's not valid at the time.)

I figure that ideally ement would be sending the web browser something here to make it happier, but I don't know how much additional effort that would entail.

It can be confusing the first time though -- in my case my window manager switches me to another workspace to get to Firefox, so I no longer have visibility on Emacs when I'm dealing with the SSO form; and consequently the first time I saw that submission error message in Firefox I assumed that the authentication process had failed.

Conversely when signing in via SSO with the standalone Element client I get automatically switched back to the original workspace after submitting the browser form, and there is no error message in the browser; so the experience is nicer there. That said, the browser doesn't actually display any confirmation message (it's still showing the form with the submit button I'd just clicked); and with Element I see that the browser form refers to The application at element://vector/webapp/ vs The application at http://localhost:4567 for Ement, so maybe the "jumping back to the application after authenticating" part is tied to having a custom element protocol.

I don't know what the options are for improving this, and it's not really a big deal (users will undoubtedly notice before long that the authentication process worked, and once you know, you know); but it would be a "nice-to-have" if Ement could cause the web browser to appear less like something had gone wrong.

@alphapapa
Copy link
Owner

Thank you, that is now working nicely for me!

Great, thanks.

Should ement-connect acquire a new keyword for the authentication method? At present I can do this:

(ement-connect :user-id "@..." :uri-prefix "https://...")

But (on account of that less-than-ideal local JSON response from my Matrix server) I still have to interactively enter sso at a prompt. I think it would be nice if the potential prompts all had relevant keywords. Something like:

(ement-connect :user-id "@..." :uri-prefix "https://..." :auth-method 'sso)

I note that if I enable session saving, then (call-interactively 'ement-connect) is working without further prompting, but a new keyword still seems like a useful enhancement.

Hm, I'm not sure. I don't think it's intended that the user call ement-connect manually in most cases, even for initial connection. And with all the "flow" that has to be handled based on the server's responses, I don't know how practical it would be, or how useful, to try to add extra logic and arguments and documentation to allow the user to potentially short-circuit a prompt one time, when first authenticating a new session. ISTM it would take more effort on the user's part to read the docstring and figure out what argument to give than to just call the command interactively and use completion for the prompt.

Something that might be helpful would be to interactively prompt the user to choose to save the session if the option isn't yet enabled, because many, if not most, users won't know that such an option exists or needs to be set in order to save the token and not have to enter credentials each time.

What do you think?

@alphapapa
Copy link
Owner

I don't know what the options are for improving this, and it's not really a big deal (users will undoubtedly notice before long that the authentication process worked, and once you know, you know); but it would be a "nice-to-have" if Ement could cause the web browser to appear less like something had gone wrong.

Yes, that should be improved. Probably we do need to send the browser an HTTP response with at least a plain HTML page that says that authentication succeeded and the tab can be closed and the user should switch back to Emacs. Like you said, I don't think we can automate that, especially with all the sandboxing browsers do, but we can at least make it less confusing.

Thanks very much for your help with this feature. I think it will benefit many users to come.

@phil-s
Copy link

phil-s commented Jun 1, 2023

with all the "flow" that has to be handled based on the server's responses, I don't know how practical it would be, or how useful

Ah, fair enough.

Something that might be helpful would be to interactively prompt the user to choose to save the session if the option isn't yet enabled, because many, if not most, users won't know that such an option exists or needs to be set in order to save the token and not have to enter credentials each time.

Yes, that sounds useful. Perhaps ement-save-sessions could default to a new value 'ask to trigger the prompt. Depending on how granular you wanted to make it, I guess that useful responses could be:

  • save this session (for the current server only; ask for others)
  • save all sessions for any server (never ask)
  • never save a session for this server (ask for others)
  • never save a session for any server (never ask)
  • do not save this time; ask again next time
  • help

Differentiating "this server" from "all servers" may be overkill, but I think "never save a session for this server (ask for others)" is the only response that isn't facilitated by the combination of the user option value and the known list of saved sessions (as it would require an additional list of blacklisted servers to be maintained; maybe this could be integrated into the session file, though?). Or you could simply omit this from the set of valid responses to avoid the complication. I suspect the other responses might all be fairly straightforward.

@alphapapa alphapapa modified the milestones: 0.10, 0.11 Jun 14, 2023
alphapapa added a commit that referenced this issue Jul 5, 2023
Use the secondary browser to browse the URL, because typically
JavaScript is required, which EWW doesn't support; and if the page is
opened in EWW first, it may break the process, because the server may
not allow that URL to be loaded multiple times.

See #24.
@alphapapa
Copy link
Owner

Yes, that sounds useful. Perhaps ement-save-sessions could default to a new value 'ask to trigger the prompt. Depending on how granular you wanted to make it, I guess that useful responses could be:

* save this session (for the current server only; ask for others)

* save all sessions for any server (never ask)

* never save a session for this server (ask for others)

* never save a session for any server (never ask)

* do not save this time; ask again next time

* help

Differentiating "this server" from "all servers" may be overkill, but I think "never save a session for this server (ask for others)" is the only response that isn't facilitated by the combination of the user option value and the known list of saved sessions (as it would require an additional list of blacklisted servers to be maintained; maybe this could be integrated into the session file, though?). Or you could simply omit this from the set of valid responses to avoid the complication. I suspect the other responses might all be fairly straightforward.

I'm not sure how I feel about those ideas. It seems a bit complicated to me, but some of them could be good. I'll defer those ideas to the future. For now, I'll consider this feature to have been implemented in v0.11. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

9 participants