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

SAML needs to transform user ID sent by IdP into a valid dotCMS user ID #19773

Closed
john-thomas-dotcms opened this issue Jan 8, 2021 · 5 comments
Assignees
Labels
LTS: Excluded Ticket that has been excluded from at least one LTS Merged QA : Approved QA : Passed Internal Release : 5.3.8.4 Included in LTS patch release 5.3.8.4 Release : 21.03 Type : Defect

Comments

@john-thomas-dotcms
Copy link
Contributor

Describe the bug

Customer reported issue: https://dotcms.zendesk.com/agent/tickets/102858

The SAML IdP can send a user ID which does not match dotCMS user ID format, and this ends up breaking some of our code. The customer that reported this issue has one SAML user that can not login to dotCMS using SAML because of this issue - they had to create a separate native login for this user.

To Reproduce

  1. Create a SAML user on the IdP so that the user ID begins with a dash (-).
  2. Attempt to login to dotCMS as that user (using SAML).
    Result: The user is unable to login, and errors are generated in the log file (see below).

Expected behavior

Users should be able to login to dotCMS using SAML, regardless of the format of the user ID provided by the IdP.

Log Messages

[05/11/20 15:44:57:346 GMT] WARN business.ESContentFactoryImpl: Elasticsearch error in index 'cluster_5fe28e94d3.working_20201029012155'
[05/11/20 15:44:57:346 GMT] WARN business.ESContentFactoryImpl: ES Query: {"size":10000,"timeout":"15000ms","query":{"query_string":{"query":"+contenttype:host +working:true +((+owner:-5bsj-fyzefxm7_wwdbsjvn3rweakf0jsi_cszm8vju +ownercanread:true) (permissions:p02088e05-5ff5-43c2-a4fa-11a7272cb199.1p* permissions:p11018493-4378-4cf5-9990-a255ff5d37a7.1p* permissions:p2e53a48f-78bd-4ef5-aad6-794477c02cba.1p* permissions:p62a0d22c-98e1-4e37-9288-d9231a7f2f39.1p* ) )","fields":[],"type":"best_fields","default_operator":"or","max_determinized_states":10000,"enable_position_increments":true,"fuzziness":"AUTO","fuzzy_prefix_length":0,"fuzzy_max_expansions":50,"phrase_slop":0,"escape":false,"auto_generate_synonyms_phrase_query":true,"fuzzy_transpositions":true,"boost":1.0}},"_source":{"includes":["inode","identifier"],"excludes":[]},"sort":[{"moddate":{"order":"desc"}}]}
[05/11/20 15:44:57:346 GMT] WARN business.ESContentFactoryImpl: Class org.elasticsearch.ElasticsearchStatusException: Elasticsearch exception [type=parse_exception, reason=parse_exception: Encountered " "-" "- "" at line 1, column 42.
Was expecting one of:
<BAREOPER> ...
"(" ...
"*" ...
<QUOTED> ...
<TERM> ...
<PREFIXTERM> ...
<WILDTERM> ...
<REGEXPTERM> ...
"[" ...
"{" ...
<NUMBER> ...
]
[05/11/20 15:44:57:346 GMT] WARN business.ESContentFactoryImpl: ----------------------------------------------

Additional context

  • Although this specific issue was caused by a dash in front of the user ID, it's possible that IdPs might create user IDs in other formats that could break our code.
    • So we really need to have a standard way of ensuring that all user IDs provided by the IdP are transformed (if necessary) into a valid dotCMS user ID.
@wezell wezell added this to the Maintenance Sprint milestone Jan 12, 2021
@wezell
Copy link
Contributor

wezell commented Jan 12, 2021

Here is the sequence:

  • Lookup by userId unhashed
  • Lookup by email unhashed
  • Lookup by userId hashed
  • create user with hashed userId

jdotcms added a commit that referenced this issue Jan 13, 2021
fmontes pushed a commit that referenced this issue Jan 21, 2021
…S user ID

* #19773 the id is being hashed now

* #19785 adding fixes for SAML 21.02

* #19785 adding fixes for SAML 21.02

* #19785 fixing an unit test
@erickgonzalez
Copy link
Contributor

PR: #19801

@jcastro-dotcms jcastro-dotcms added the LTS : Next Ticket that will be added to LTS label Jan 25, 2021
@jcastro-dotcms jcastro-dotcms added LTS: Excluded Ticket that has been excluded from at least one LTS LTS: Released labels Feb 3, 2021
@jcastro-dotcms
Copy link
Contributor

The official code changes have been included in the dotCMS 5.3.8.4 LTS release only. The 5.2.8.4 LTS release is several commits behind, which made the code back-port not possible.

@john-thomas-dotcms john-thomas-dotcms added Release : 5.3.8.4 Included in LTS patch release 5.3.8.4 and removed LTS: Released LTS : Next Ticket that will be added to LTS labels Feb 10, 2021
@erickgonzalez
Copy link
Contributor

Fixed... now users on SAML are created using the userID hashed to avoid dash - at the beginning.
Screen Shot 2021-02-18 at 11 13 36 AM

@bryanboza
Copy link
Member

bryanboza commented Mar 15, 2021

Fixed, tested with SAML in a local configuration and now works as expected. Tested on release-21.03 // Postgres // FF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LTS: Excluded Ticket that has been excluded from at least one LTS Merged QA : Approved QA : Passed Internal Release : 5.3.8.4 Included in LTS patch release 5.3.8.4 Release : 21.03 Type : Defect
Projects
None yet
Development

No branches or pull requests

7 participants