Skip to content

Conversation

@samdroid-apps
Copy link
Contributor

@samdroid-apps samdroid-apps commented Jun 6, 2025

This PR updates the SAML authenticator to match the (more recently updated) OIDC authenticator.
Previously, the SAML authenticator only added roles to the root org, and only assigned roles during user creation.
Now it will add roles every login and to all orgs.

@CLAassistant
Copy link

CLAassistant commented Jun 6, 2025

CLA assistant check
All committers have signed the CLA.

@scudette
Copy link
Contributor

scudette commented Jun 9, 2025

Since orgs are dynamically added and removed I dont think the right approach is to specify the orgs in the config file. The oidc provider just sets permissions in all orgs :

if self.authenticator.Claims == nil ||

The provider allows the IDP to set a roles claim and then adjust the ACLs based on this claim on all available orgs. For consistency we should implement this issue in the same way as oidc

Usually when roles are assigned through the IDP we want the user to have access to all orgs.

Perhaps we need a more flexible way to assign new users automatically - this can be achieved by having the authenticator fire an event and then setting up some server event monitoring query to actually add the user to the relevant orgs. This allows the orgs to be flexibly configured.

@samdroid-apps samdroid-apps changed the title Add saml_user_orgs config option (#4225) saml: assign roles every login, to all orgs (#4225) Jun 9, 2025
@samdroid-apps
Copy link
Contributor Author

Great point @scudette. I've changed the approach of this PR to match SAML's behaviour to that of OIDC.

I was initially concerned that other velociraptor users might be relying on the SAML connector's root-only assignments for security. However, as long as this change is documented in any release notes, it should be fine.

Please take another look :)

@scudette scudette merged commit 48c6086 into Velocidex:master Jun 12, 2025
3 checks passed
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.

3 participants