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

Add support for Okta Group claims to set user role #15020

Closed
wants to merge 13 commits into from

Conversation

peejaychilds
Copy link
Contributor

Please give a short description what your pull request is for

Allow the Socialite Okta support to ask for groups claim, and then use those okta groups to map to User level's

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.
  • If my Pull Request makes discovery/polling/yaml changes, I have added/updated test data.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

murrant
murrant previously requested changes May 1, 2023
app/Http/Controllers/Auth/SocialiteController.php Outdated Show resolved Hide resolved
app/Http/Controllers/Auth/SocialiteController.php Outdated Show resolved Hide resolved
app/Http/Controllers/Auth/SocialiteController.php Outdated Show resolved Hide resolved
Copy link
Member

@Jellyfrog Jellyfrog left a comment

Choose a reason for hiding this comment

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

You need to remove the Okta specific code
And change the logic so you specify the scope name for groups instead of hardcoded "groups"

And last, need to make the group name mapping configurable

@murrant
Copy link
Member

murrant commented May 3, 2023

@Jellyfrog shouldn't the "group" name line up with "role" names in LibreNMS? Or do want to require explicit mapping? Or should there be a "user group" concept in LibreNMS?

@Jellyfrog
Copy link
Member

@Jellyfrog shouldn't the "group" name line up with "role" names in LibreNMS? Or do want to require explicit mapping? Or should there be a "user group" concept in LibreNMS?

Well since we don't have groups in LibreNMS, I think this is the best way atm. We cant expect people to create a group in AD called "admin" for example :)

@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/saml2-group-mapping-giving-access-level/21356/2

@murrant
Copy link
Member

murrant commented May 9, 2023

I think code design is good now. Code style could use a clean up pass (I can tell you copied code from some older parts of LibreNMS). If I submit a cleanup, would you test it after?

Remove some redundant safety checks
Prevent bug where group name could contain a dot
Use match statement
save outside of setLevelFromClaim (it checks if there are changes before saving)
@Jellyfrog
Copy link
Member

I'll give it a go to try it out on some other provider.

@peejaychilds
Copy link
Contributor Author

I have put the refactored changes through our lower testing environments and it works as expected @murrant 🙏

@murrant murrant changed the title Add support for Okta Group claims to set User level Add support for Okta Group claims to set User role May 10, 2023
@murrant murrant changed the title Add support for Okta Group claims to set User role Add support for Okta Group claims to set user role May 10, 2023
@Jellyfrog
Copy link
Member

Like I wrote on discord, this doesn't work at all for azuread.
The problem is socialite doesn't use the id/access-token jwt content to get info about the user, it does a separate request to some user info endpoint, and in azures case it doesnt contain group info. there is a separate endpoint for that.

And I'm guessing it won't work for a lot of providers out there, need to test a few at least, else this will just lead to a billion questions.

@peejaychilds
Copy link
Contributor Author

peejaychilds commented May 10, 2023

I noticed the gui is borked for the Groups->Roles mapping.

I assume perhaps have to make a new config type rather than use ldap-groups and then make some type of javascript helper function to help with the populating the select boxes?

We don't really ever look in the GUI but I guess it would be nice if it worked as expected.

Should I have a go at this?

@Jellyfrog
Copy link
Member

So I did some more testing and github doesn't work
And Google via saml2 doesnt work(but quite Close actually.)

This kinda means I'm against adding it in its current state, have to give it some thought.

@peejaychilds
Copy link
Contributor Author

So I did some more testing and github doesn't work And Google via saml2 doesnt work(but quite Close actually.)

This kinda means I'm against adding it in its current state, have to give it some thought.

But it does work for Okta, which makes it pretty useful for shops with Okta with a fairly minor impact.

I'm assuming getting all the other providers to work would involve some upstream work to get it all nicely abstracted to be generic

@rgilijamse
Copy link

As I understand it, the problem is that claims/groups do not work for providers other than Okta. Would it otherwise be possible to separate just the 'Default Role' part of this PR and merge that? That should work for all providers, and is a useful feature on it's own.

@murrant
Copy link
Member

murrant commented May 19, 2023

Hmm it is hard to predict the upstream api. I'm uncertain if we should continue on this approach or try to set up a way to add per-provider code.

Yes, extracting the default role would be be easily mergable.

@WindoC
Copy link

WindoC commented Jun 20, 2023

Is there any plan to add the default_role feature on the next version?

Please add it is very useful for me. Thanks!

@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/socialite-permissions/21656/2

@murrant murrant added this to the 23.8.0 milestone Aug 4, 2023
@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/sso-not-showing-devices-after-set-up/21949/4

@murrant
Copy link
Member

murrant commented Oct 4, 2023

@peejaychilds roles have been merged, can you rebase this?

@murrant murrant removed this from the 23.8.0 milestone Oct 4, 2023
@peejaychilds
Copy link
Contributor Author

replaced via #15592

@peejaychilds peejaychilds deleted the okta_groups branch November 20, 2023 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants