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.

Sign up for GitHub

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

Jump to bottom

[IMPROVE] Use SessionId for credential token in SAML request #13791

Merged
merged 4 commits into from
Apr 6, 2019
Merged

[IMPROVE] Use SessionId for credential token in SAML request #13791

merged 4 commits into from
Apr 6, 2019

Conversation

MohammedEssehemy
Copy link
Contributor

@MohammedEssehemy MohammedEssehemy commented Mar 19, 2019 •
edited
Loading

to be more stricter for current session

@MohammedEssehemy
Use SessionId for credential token
96267ee 
to be more stricter for current session
@CLAassistant
Copy link

CLAassistant commented Mar 19, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@MohammedEssehemy
Fix linting issues
3564db9 
Remove unused vars
@engelgabriel
Copy link
Member

Thanks @MohammedEssehemy

Can you please explain what is going to be the difference in the end user experience?

@MohammedEssehemy
Copy link
Contributor Author

@engelgabriel because the credential Token was generated randomly client side, a malicious attacker could send curated link to victim and when the user log this will lead to account take over by attacker.

Now, we use session id and so attacker has no way to change his session id.

@engelgabriel engelgabriel added this to the 1.0.0 milestone Mar 22, 2019
@engelgabriel
Copy link
Member

I understand. Seems like a good idea indeed!

@MohammedEssehemy
Copy link
Contributor Author

any updates here?

@engelgabriel engelgabriel changed the title Use SessionId for credential token in saml request [IMPROVE] Use SessionId for credential token in SAML request Apr 6, 2019
@engelgabriel engelgabriel merged commit 3967a74 into RocketChat:develop Apr 6, 2019
@rodrigok rodrigok mentioned this pull request Apr 28, 2019
Merged
rafaelks
rafaelks reviewed May 1, 2019
View reviewed changes
app/meteor-accounts-saml/server/saml_server.js
@@ -92,11 +92,11 @@ Meteor.methods({
});

Accounts.registerLoginHandler(function(loginRequest) {
if (!loginRequest.saml || !loginRequest.credentialToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change broke the mobile apps I think (cc @geekgonecrazy)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup I think so as well... :(

Copy link
Contributor Author

@MohammedEssehemy MohammedEssehemy May 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

can't the mobile app send the connection id in the SAML request instead of the random secret?

geekgonecrazy added a commit that referenced this pull request May 1, 2019
@geekgonecrazy
Revert "[IMPROVE] Use SessionId for credential token in SAML request (#…
bffd510 
…13791)"

This reverts commit 3967a74.
@geekgonecrazy geekgonecrazy mentioned this pull request May 1, 2019
Merged
rodrigok pushed a commit that referenced this pull request May 7, 2019
@geekgonecrazy @rodrigok
Revert "[IMPROVE] Use SessionId for credential token in SAML request (#…
436eae8 
…13791)" (#14345)

This reverts commit 3967a74.
rodrigok pushed a commit that referenced this pull request May 9, 2019
@geekgonecrazy @rodrigok
Revert "[IMPROVE] Use SessionId for credential token in SAML request (#…
eb35011 
…13791)" (#14345)

This reverts commit 3967a74.
rodrigok added a commit that referenced this pull request May 10, 2019
@rodrigok
Release 1.0.3 (#14446)
f19473c 
* [FIX] New day separator overlapping above system message (#14362)

* Improve German translations (#14351)

* Use the plural for discussions-section in side panel

* Formal and informal translations for 1.0

* fix german typos

* [FIX] Main thread title on replies (#14372)

* fix

* fix test

* fix setting

* Update tests/pageobjects/main-content.page.js

Co-Authored-By: ggazzo <guilhermegazzo@gmail.com>

* Update app/ui-utils/client/lib/RoomHistoryManager.js

Co-Authored-By: ggazzo <guilhermegazzo@gmail.com>

* [FIX] Bell was too small on threads (#14394)

* [FIX] Messages on threads disappearing (#14393)

* fix subscription-changed updating all messages(#14391)

* Fix: Message body was not being updated when user disabled nrr message (#14390)

* [NEW] Allow change Discussion's properties (#14389)

* [FIX] Unnecessary meteor.defer on openRoom (#14396)

* [FIX] more message actions to threads context(follow, unfollow, copy, delete) (#14387)

* added more message actions to threads context

* more actions

* change token name (#14379)

* [FIX] Pressing Enter in User Search field at channel causes reload (#14388)

* Prevent default on enter in User search

* Prevent form submission in membersList

* If using subpath make sure streams use that also for multi-instance.  Fixes #13200 (#14376)

* Revert "[IMPROVE] Use SessionId for credential token in SAML request (#13791)" (#14345)

This reverts commit 3967a74.

* Add fallback to mongo version that doesn't require clusterMonitor role (#14403)

* [FIX] Users actions in administration were returning error (#14400)

* Fix actions collapse into popup in userInfo

* Refactor userActions

* [FIX] Error 400 on send a reply to an old thread (#14402)

* fix error 400 on send a reply to an old thread

* ignoring properly hidden messages

* [FIX] Messages on thread panel were receiving wrong context/subscription  (#14404)

* [FIX] preview pdf its not working (#14419)

* [FIX] renderMessageBody was caching messages in wrong scenarios #14420

* LingoHub Update 🚀 (#14426)

Manual push by LingoHub User: Diego Sampaio.
Project: Rocket.Chat

Made with ❤️ by https://lingohub.com

* [FIX] Mentions message missing 'jump to message' action (#14430)

* fixed context

* threads context

* [FIX] Escape unrecognized slash command message (#14432)

* Add missing german translations (#14386)

* [FIX] IE11 support (#14422)

* Add symlinks to ES6 node_modules imports

* Add URL polyfill for IE11

* Fix thread replies for IE11

* [IMPROVE] allow users to skip activeUsers to be ready (#14431)

* allow users to skip activeUsers to be ready

* Update main.js

* Update app/ui-master/client/main.js

Co-Authored-By: ggazzo <guilhermegazzo@gmail.com>

* [IMPROVE] Don't use regex to find users (#14397)

* Don't use regex to find users

* Invert logic on model methods

* Escape username regex

* Find users in batch

* Use only normalizeMessagesForUser

* Don't ignore username case to get owners on graphql

* Fixes on DAU and MAU aggregations (#14418)

* Fixes on SAU and MAU aggregations

* Report new data from DAU/MAU

* Run tests agains a mongodb container in CI

* Try to run CI correctly

* Fix drop database

* Parse desktop app User Agent correctly

* Fix aggregation of past sessions

* Return past month today

* Fix bug

* Add migration

* Fixed migration

* Migration improvements

* Fix crowd sync by using correct logging method (#14405)

* Fix room names in user info dialogs (#14415)

* Fix discussion name being invalid (#14442)

Closes #14378

* Fix i18n files keys sort (#14433)

* Add script to normalize i18n files

* Fix i18n files

* Set as official script

* Update package-lock.json

* fix (#14443)

* Update threads.css

* Bump version to 1.0.3

* regen changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geekgonecrazy geekgonecrazy geekgonecrazy left review comments

@rafaelks rafaelks rafaelks left review comments

@engelgabriel engelgabriel engelgabriel approved these changes

@sampaiodiego sampaiodiego Awaiting requested review from sampaiodiego

@rodrigok rodrigok Awaiting requested review from rodrigok

@Hudell Hudell Awaiting requested review from Hudell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MohammedEssehemy @CLAassistant @engelgabriel @geekgonecrazy @rafaelks