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

Things which should need upstream pull requests #38

Closed
gabrc52 opened this issue Jan 19, 2023 · 3 comments
Closed

Things which should need upstream pull requests #38

gabrc52 opened this issue Jan 19, 2023 · 3 comments
Labels
coding requires coding matrix-ecosystem Improvements to Matrix as a whole, not MIT or Uplink-specific P3 priority 3 (nice to have but doesn't block launch)

Comments

@gabrc52
Copy link
Contributor

gabrc52 commented Jan 19, 2023

Changing "SAML" to "Touchstone"

Currently, the name of the sign in provider is hardcoded to SAML.

Patch/hotfix:

https://github.com/matrix-org/synapse/blob/develop/synapse/handlers/saml.py#L77
Change the hardcoded name from SAML to Touchstone

Actual solution:

Make it read the name from the config, and default to "SAML". Add a new configuration parameter. Make this for the user-facing icon/brand too and add the same patch to CAS too.

Allow changing your display name on first login

Patch:

Modify the template to include a name field. Thinking code could look like this:

  1. Read other POST parameters and pass them https://github.com/matrix-org/synapse/blob/fa8616e65c82367712a7b75c62682a89541b6330/synapse/rest/synapse/client/new_user_consent.py#L106
  2. Pass down the new parameters https://github.com/matrix-org/synapse/blob/fa8616e65c82367712a7b75c62682a89541b6330/synapse/handlers/sso.py#L876
  3. Right here, add session.display_name = given_display_name https://github.com/matrix-org/synapse/blob/fa8616e65c82367712a7b75c62682a89541b6330/synapse/handlers/sso.py#L895

Actual solution:

Talk in synapse-dev room and figure out what the best config options would be, what the best visuals and CSS would be in general. Make another pull request (once the first one is merged) to:

  1. optionally confirm their localpart or change it to a new one
  2. optionally confirm their display name or change it to a new one or leave it blank (which currently can only be left blank with confirm_localpart
  3. optionally confirm their email or other 3pids or leave them blank (only those 2 options because allowing setting an arbitrary email would defeat the point of 3pid verification)

1, 2 and 3 should be independent from each other and not mutually exclusive. The homeserver admin should be able to configure it to only allow changing display names, or only localpart or only the 3 of them and so on.

Maybe confirm_localpart could be #1, confirm_display_name could be #2, and confirm_email could be #3, but this would require changing the behavior of confirm_localpart, and I'm unsure if backwards compatibility is needed.

Alternative (or addition) that would allow for the other checkboxes to exist too without needing source code modification: allow for Synapse plugins to customize the signup experience, so everyone can ask for whatever things they want and add a callback for those things

--

Upstream issue to the above: matrix-org/synapse#14790

@gabrc52 gabrc52 added P3 priority 3 (nice to have but doesn't block launch) matrix-ecosystem Improvements to Matrix as a whole, not MIT or Uplink-specific coding requires coding labels Jan 19, 2023
@gabrc52
Copy link
Contributor Author

gabrc52 commented Jan 20, 2023

Also for matrix-nio: matrix-nio/matrix-nio#375 - on second thought the best thing to do might be to add another flag for the homeserver and in the client you abstract it, but the api should keep being low level, just as in the API. This would make it into a reasonable PR.

@gabrc52
Copy link
Contributor Author

gabrc52 commented Feb 3, 2023

Possibly relevant: https://github.com/matrix-org/matrix-synapse-saml-mozilla/

Asking to confirm display name or additional information may be possible with a mapping provider. We can make a new one and release it. I would still like it to be in Synapse, but it may work as an interim solution without being hacky. Documenting that this is possible should also be done for Synapse. (I didn't know you could redirect people with such a thing)

@gabrc52
Copy link
Contributor Author

gabrc52 commented Aug 18, 2023

matrix-org/synapse#16094 Touchstone fixed!

The enhanced behavior is an uncommon use case so having our own fork of that is fine...

I don't think I'm using matrix-nio anymore, nor does it seem actively maintained. mautrix is very actively maintained (because of beeper) but pretty undocumented. I have some TODOs on what to improve in mautrix (mostly missing functoins), but having such a broad issue in this repo is impractical.

@gabrc52 gabrc52 closed this as completed Aug 18, 2023
@gabrc52 gabrc52 reopened this Aug 18, 2023
@gabrc52 gabrc52 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coding requires coding matrix-ecosystem Improvements to Matrix as a whole, not MIT or Uplink-specific P3 priority 3 (nice to have but doesn't block launch)
Projects
None yet
Development

No branches or pull requests

1 participant