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

Server crashes when using OCID connection + Sign in with Apple #4701

Closed
piscis opened this issue Nov 22, 2024 · 6 comments
Closed

Server crashes when using OCID connection + Sign in with Apple #4701

piscis opened this issue Nov 22, 2024 · 6 comments
Labels

Comments

@piscis
Copy link

piscis commented Nov 22, 2024

We're currently trying to setup CoralTalk together with a OCID ID provider (https://logto.io) using AppleID as a connector. It turns out when OCID token exchange is done the server crashes with a ValidationError: stating that name and picture must be a string and not null.

Because Apple SignIn does not return a "name" or "picture" logto returns a null instead of an empty string. The error seems to be caused in this two lines:

picture: Joi.string(),
name: Joi.string(),

When forcing null values into empty strings before validation everything seams to work:

...
picture: Joi.string().allow(null).default('').custom((value) => value === null ? '' : value),
name: Joi.string().allow(null).default('').custom((value) => value === null ? '' : value),
...

Example of the user user info provided to CoralTalk from LogTo.io after successful authentication via Apple SignIn:

{
    "sub": "lxa7wl57jyjf",
    "name": null,
    "picture": null,
    "updated_at": 1732302616720,
    "username": null,
    "created_at": 1732291345221,
    "organizations": [],
    "organization_data": [],
    "organization_roles": []
}

When forcing null values into empty strings everything seems to work. In case a new user registers and the name is an empty string CoralTalk asks for the username during the registration.

But I do not know if this could have some other implications for CoralTalk. Would be nice if someone could clarify.

Expected behavior:

It would be great if CoralTalk would allow null values for "picture" or "name" to make it possible to connect via login providers like "SignIn with Apple".

At least validation errors in the OCID connector should not crash the server.

Actual behavior:

It crashes the server and makes it impossible to use providers that do not provide name or picture information.

Related Issues:
no

Versions:

  • NodeJS: 14.31.2
  • NPM: 8.0.0
  • MongoDB: 4.2
  • Redis: 3.2
  • Browser: Chrome / Firefox / Safari
  • OS: Ubuntu LTS (AMD64) deployed via docker: coralproject/talk:7
@piscis piscis added the bug label Nov 22, 2024
@piscis piscis changed the title Server crashes when using OCID connection + AppleID Server crashes when using OCID connection + Sign in with Apple Nov 22, 2024
@losowsky
Copy link
Member

Hi there

Picture isn't required but every user needs a username, so that they can be identified in the community. It's listed as "required" in our SSO docs for that reason. You can see a list of required fields in the JWT in our SSO docs, as well as information on how to successfully connect SSO providers: https://docs.coralproject.net/sso

@piscis
Copy link
Author

piscis commented Nov 23, 2024

Hi @losowsky thanks for taking the time to look into this. And yes you are correct if we would use jwt via sso integration we could handle this from our side but we are trying to use the ocid connector and in case of apple signin there is no username or name but if I patch coraltalk to convert the null values into an empty string a user gets ask to pick a username during registration. So I thing it’s fine in case a username is not available because coraltalk is asking for one to be created before finishing the registration.

Also there is the point that if the ocid connector throws the Joi validation error the server crashes afaik

would you accept a pr with the change's above to make it possible to use something like logto.io or keycloak together with apple signin via ocid ?

@tessalt
Copy link
Contributor

tessalt commented Nov 25, 2024

@piscis we would take a look at a small change to the OIDC integration!

@piscis
Copy link
Author

piscis commented Nov 26, 2024

@tessalt Thank you for the reply! I’ve prepared a pull request (#4702) for this issue.

Quick side question: we tried to contact you about pricing a couple of days ago through the website. Do you offer plans for commercial hosting for CoralTalk?

@tessalt
Copy link
Contributor

tessalt commented Nov 26, 2024

@piscis can you confirm the name or company you used in the form? Not sure which submission is yours.

@piscis
Copy link
Author

piscis commented Nov 26, 2024

@tessalt my colleagues from all3dp.com tried to get a quote

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

No branches or pull requests

3 participants