Skip to content

[IMPROVE] OAuth Role Sync#13761

Merged
geekgonecrazy merged 27 commits intoRocketChat:developfrom
hypery2k:feature/oauth_groups_12243
Apr 15, 2019
Merged

[IMPROVE] OAuth Role Sync#13761
geekgonecrazy merged 27 commits intoRocketChat:developfrom
hypery2k:feature/oauth_groups_12243

Conversation

@hypery2k
Copy link
Copy Markdown
Contributor

see #12992

@geekgonecrazy geekgonecrazy changed the title IMPROVE] Improved support for OAuth Provider [IMPROVE] Improved support for OAuth Provider Mar 27, 2019
@hypery2k
Copy link
Copy Markdown
Contributor Author

hypery2k commented Apr 3, 2019

The error in circle seems to not being related to my changes:

Element <div class="sidebar-item__ellipsis">...</div> is not clickable at point (184, 197). Other element would receive the click: <div class="sidebar-item__ellipsis">...</div>

Any chance to get this merged into master?

@hypery2k
Copy link
Copy Markdown
Contributor Author

@geekgonecrazy ok for you?

@geekgonecrazy geekgonecrazy changed the title [IMPROVE] Improved support for OAuth Provider [IMPROVE] OAuth Role Sync Apr 12, 2019
Copy link
Copy Markdown
Contributor

@geekgonecrazy geekgonecrazy left a comment

Choose a reason for hiding this comment

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

Also please at least add the english i18n key for Accounts_OAuth_Custom_Merge_Roles don't need to bother with all the other languages in the scope of this PR

@hypery2k
Copy link
Copy Markdown
Contributor Author

didn't get my circleci is failing.

@geekgonecrazy
Copy link
Copy Markdown
Contributor

circleci has been having a number of issues. :(

I've pushed a commit making some slight changes.

  1. to not add roles directly to the user in the map. Instead just return the roles as an array.
  2. To only loop over those that should be added or removed by filtering down the difference

geekgonecrazy
geekgonecrazy previously approved these changes Apr 13, 2019
@hypery2k
Copy link
Copy Markdown
Contributor Author

thanks

@geekgonecrazy
Copy link
Copy Markdown
Contributor

Just tried out with Okta. It works beautifully! :) Made one final adjustment

geekgonecrazy
geekgonecrazy previously approved these changes Apr 13, 2019
@hypery2k
Copy link
Copy Markdown
Contributor Author

great, i check it with latest keycloak and also works perfect. I will try to create another PR with some basic tests.
Would be end2end testing be needed? Could give it try in this new PR, if wanted.

@geekgonecrazy
Copy link
Copy Markdown
Contributor

geekgonecrazy commented Apr 13, 2019

Perfect! I think this PR is good to go then! :)

Sorry your other PR didn’t get addressed. :(

e2e testing would be great! But I think it would be incredibly difficult. Because would need an oauth provider that could consistently work in ci + provide roles.

But if you want to open a Pr to add testing of any sort to oauth.. that would be fantastic. Maybe just unit tests to ensure all of this logic holds sound?

@geekgonecrazy geekgonecrazy merged commit dd76eca into RocketChat:develop Apr 15, 2019
@rodrigok rodrigok mentioned this pull request Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Read Group info from OAuth provider

4 participants