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

Jump to bottom

SCIM support via laravel-scim-server package, using self-generated routes, secured by current API key system #10802

Closed
wants to merge 18 commits into from
Closed

SCIM support via laravel-scim-server package, using self-generated routes, secured by current API key system #10802

wants to merge 18 commits into from

Conversation

uberbrady
Copy link
Collaborator

So this does do a correct API auth check on the bearer token - which is what we want. But I'm not sure that it checks for the right permissions here. It could be fair to say that the token needs to have edit, update, list, delete - as that's what the library wants. Basically you need full dominion over the Users section for your token to work here.

BUT - if you use no token on one of the protected routes, or you use an incorrect token, you get the correct (401-style) responses. So this feels like the right general direction.

Leaving it as draft because though I may have Authn (Authentication), I'm definitely not sure about Authz (Authorization).

@uberbrady uberbrady marked this pull request as ready for review March 16, 2022 22:07
@uberbrady uberbrady requested a review from snipe as a code owner March 16, 2022 22:07
@uberbrady
Copy link
Collaborator Author

Okay - I have Authorization working now - the only API keys this will work with are Superuser ones. Any other one will throw a 403. Unfortunately, that 403 is really janky-looking, even if I add an Accept: application/json header. That's fixable but feels to me to be cosmetic so I think I'd allow it.

The more controversial change I made is to remove Ziggy. I couldn't get Ziggy's @routes directive to work correctly (it broke all page-loads...). I have an open issue with the package provider about that (limosa-io/laravel-scim-server#15) , but in case they aren't able to help, I figured I would try to pull it.

This means that any Vue page that has anything to do with any other Vue page should be tested pretty thoroughly.

I grepped through the code pretty thoroughly, and I couldn't find any remaining instances of the word Ziggy, and I grepped through all JS and all vue files, looking for the word route( and there was only one instance, which was in a comment, which was more about the Blade-PHP usage of the word.

In the process here I found a vestigial build file that isn't referenced anywhere, so I removed it. Assets should still build normally (well, they do for me...).

@uberbrady uberbrady changed the title SCIM self provided routes SCIM support via laravel-scim-server package, using self-generated routes, secured by current API key system Mar 17, 2022
snipe
snipe reviewed Mar 17, 2022
View reviewed changes
app/Models/SnipeSCIMConfig.php Outdated
'validations' => [

'urn:ietf:params:scim:schemas:core:2.0:User:userName' => 'required',
'urn:ietf:params:scim:schemas:core:2.0:User:password' => 'nullable',
Copy link
Owner

Choose a reason for hiding this comment

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

Our passwords are not nullable - will this conflict?

This brings up a different question - will this somehow be able to bypass the model level validation and/or settings validation? If it can, we’re in a world of hurt, since we’ll enable adding bad data, which can mean saving an object will fail later in a way we don’t expect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GREAT question. I don’t know. I don’t have a good SCIM test-rig, but if I can figure one out, I’ll test this - and confirm or reject it.

Model-level validation should fail if given a model that is invalid, so I’d hope that’d be caught. But hope is not certainty.

Copy link
Owner

Choose a reason for hiding this comment

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

Right, but how we fail is important here. If we don’t have a way to communicate the errors, it will add to our support load (and confusion).

Copy link
Contributor

Choose a reason for hiding this comment

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

For the major SCIM providers I know of, SCIM will never provide a password to the application. Generally, the passwords wont be stored with reversible encryption, and will not be provided to an SCIM endpoint.

The password would need to be nullable in the model/database. The user would only be able to successfully login with SSO at that point.

Alternatively, a welcome email could be sent instructing the user to login with a randomly generated password to access Snipe IT, or provide an activation link to enter a new password. That's how I have seen other applications implement this without SSO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough. If we need to, we can hardcode a 'xxxxxxx' password which is not possible to hash towards. We'll have to see how it stands up when Azure tries to SCIM us up, I guess?

snipe
snipe reviewed Mar 17, 2022
View reviewed changes
app/Models/SnipeSCIMConfig.php Outdated
'schema' => [Schema::SCHEMA_USER],

//eager loading
'withRelations' => [],
Copy link
Owner

Choose a reason for hiding this comment

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

Do you know what this does? We generally do load users with relations, so I’m not sure what impact this might have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope!

Copy link
Owner

Choose a reason for hiding this comment

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

Definitely not the answer I was hoping for, but I do appreciate your honesty lol

snipe
snipe reviewed Mar 17, 2022
View reviewed changes
config/app.php Show resolved Hide resolved
snipe
snipe requested changes Mar 17, 2022
View reviewed changes
Copy link
Owner

@snipe snipe left a comment

Choose a reason for hiding this comment

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

I left my comments inline - this looks amazing so far, just some concerns.

I am VERY excited for this PR!!

@uberbrady uberbrady requested a review from snipe March 19, 2022 14:14
snipe
snipe reviewed Mar 19, 2022
View reviewed changes
routes/scim.php Outdated Show resolved Hide resolved
@snipe
Copy link
Owner

snipe commented Mar 19, 2022

This looks great - How do we test it? I know @adagioajanes had mentioned he might be able to help with testing this.

@uberbrady
Copy link
Collaborator Author

This looks great - How do we test it? I know @adagioajanes had mentioned he might be able to help with testing this.

Great question, I’m not sure. I have some ideas -

  1. Actually try and integrate with Azure, which will allow for arbitrary SCIM integrations. Means we’d need an Azure Directory hookup, but that’s not out of the question.
  2. The SCIM API is pretty nicely defined (a little backwards, IMHO, but, still, nice), and we could just make REST requests to create, update, and delete users. Maybe build out a little test-rig?
  3. See if someone already built out some kind of “SCIM test” or “SCIM barebones client” or something like that, that we can use to test and mess around with stuff.

@adagioajanes
Copy link
Contributor

@uberbrady I can at least stand up an instance, and connect my Azure AD to it to see what we get. I will let you know my results.

@uberbrady
Copy link
Collaborator Author

uberbrady commented Mar 21, 2022 •
edited

@uberbrady I can at least stand up an instance, and connect my Azure AD to it to see what we get. I will let you know my results.

That’d be awesome!!!!

@adagioajanes
Copy link
Contributor

adagioajanes commented Mar 22, 2022 •
edited

We're actually off to a great start. The SCIM endpoint is /SCIM/v2 which is easy enough.

However, I am getting errors about attributes not being supported by Snipe IT. I went ahead and modified my mapping to only use the configured attributes you have for SCIM. But I am still getting an error from Snipe IT in Azure AD...

StatusCode: InternalServerError
Message: Processing of the HTTP request resulted in an exception. Please see the HTTP response returned by the 'Response' property of this exception for details.
Web Response: 
{"schemas":["urn:ietf:params:scim:api:messages:2.0:Error"],"detail":"Write to \"urn:ietf:params:scim:schemas:core:2.0:User:active\" is not supported","status":500}

Can we add a configuration for the 'Login Enabled' setting on users, and tie it to this attribute? It seems like Azure AD really insists on this particular attribute's existence.

@snipe
Copy link
Owner

snipe commented Mar 22, 2022

VERY exciting! Thanks very much to you @uberbrady and especially you, @adagioajanes - your help has been invaluable through this.

TrashcanHug

@adagioajanes
Copy link
Contributor

More great progress has been made. I am now able to provision new users with most of their attributes working. I believe @uberbrady said there is more work to be done on some more attributes. But this is definitely in a condition where some more testing is appropriate.

@JemTaylorUHI I believe you mentioned in #10412 that you have some resources available for testing. If that offer still stands, could your team do some testing of the new SCIM features on this branch and let us know your results? Additionally, what SCIM vendor will you be using?

@JemTaylorUHI
Copy link

JemTaylorUHI commented Mar 27, 2022 via email •
edited by snipe

@snipe
Copy link
Owner

snipe commented Mar 27, 2022

Hi @JemTaylorUHI - it’s not standard for us to set up customers with development instances (particularly because they can be buggy, as development branches sometimes are). Since this is additive, I’m more inclined to put this into production for v6 and then refine as we move forward. @adagioajanes has been super helpful with testing Azure, and although there are still some warts here, I’m mostly okay with this as-is. We’re seeing some errors and inconsistencies from one of the library dependencies for the SCIM server, so it’s very possible that we may end up doing a rewrite of the guts of this, but we are also MUCH better versed in how the SCIM protocol works now, so we’re in a way better position to roll our own if needed.

@JemTaylorUHI
Copy link

JemTaylorUHI commented Mar 28, 2022 via email

@adagioajanes
Copy link
Contributor

@JemTaylorUHI So far, SCIM testing has worked for me using the API token I generated with a global admin. Expiration is controlled by Snipe IT at token creation. I haven't needed to utilize any additional OAuth features in Azure AD to make this work.

Perhaps there is a feature or functionality you are talking about that I am not familiar with?

@JemTaylorUHI
Copy link

JemTaylorUHI commented Mar 28, 2022 •
edited

@JemTaylorUHI So far, SCIM testing has worked for me using the API token I generated with a global admin. Expiration is controlled by Snipe IT at token creation. I haven't needed to utilize any additional OAuth features in Azure AD to make this work.

Perhaps there is a feature or functionality you are talking about that I am not familiar with?

Ooops - I forwarded your email to Alistair and he replied to me but also cc: to github. You can safely ignore his comments which are a bit out of context!

@t4ov
Copy link

t4ov commented Mar 31, 2022

Hello. That looks very good. Just one question, will it support other vendors, like Jumpcloud to use SCIM? Thanks.

@snipe
Copy link
Owner

snipe commented Mar 31, 2022

It should, yes. We avoid vendor-specific integrations wherever possible - but it also depends on the vendor’s implementation. If they follow the correct protocol, it should work.

@snipe
Copy link
Owner

snipe commented Apr 5, 2022

Thanks everyone for all your convo and participation in this discussion! @uberbrady ended up creating a new PR for this (squashed), and it's now merged onto develop, so I'm going to close this one. (There is also a lively thread on Discord about this.)

@snipe snipe closed this Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snipe snipe snipe requested changes

@adagioajanes adagioajanes adagioajanes left review comments

Assignees
No one assigned
Labels
backend frontend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@uberbrady @snipe @adagioajanes @JemTaylorUHI @t4ov