-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fix "/Schemas" endpoint; configurable default resources #133
Conversation
… in the 'unsupported' section which now work due to prior enhancements, that overlooked the related README.md update
@@ -600,8 +617,6 @@ Often, you'll find that bearer tokens are in use by SCIM API consumers, but the | |||
|
|||
### Specification versus implementation | |||
|
|||
* The `name` complex type of a User has `givenName` and `familyName` fields which [the RFC 7643 core schema](https://tools.ietf.org/html/rfc7643#section-8.7.1) describes as optional. Scimitar marks these as required, in the belief that most user synchronisation scenarios between clients and a Scimitar-based provider would require at least those names for basic user management on the provider side, in conjunction with the in-spec-required `userName` field. That's only if the whole `name` type is given at all - at the top level, this itself remains optional per spec, but if you're going to bother specifying names at all, Scimitar wants at least those two pieces of data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#113 disagreed and so this was altered, since it was a fairly arbitrary decision and did indeed not match the spec. I've kind of given up trying to figure out ways to have the SCIM spec make any kind of coherent sense now.
|
||
- `filter=userType eq "Employee" and emails.type eq "work" and emails.value co "@example.com"` | ||
|
||
...so adding a mapping for `emails.value` would then allow a database query to be constructed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The topmost example does now work (and I've added test coverage to prove it). Unsure exactly when this started working. It looks a bit like #115, but isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. I found a single incredibly minor grammatical mistake in a comment, but this is otherwise good to go.
lib/scimitar/engine.rb
Outdated
# | ||
# * Scimitar::Resources::User | ||
# * Scimitar::Resources::Group | ||
# | ||
# ...but if an implementation does not e.g. support Group, it can override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# ...but if an implementation does not e.g. support Group, it can override | |
# ...but if an implementation does not e.g. support Group, it can overriden |
great work here @pond, thanks for taking it on! |
|
||
```ruby | ||
Rails.application.config.to_prepare do | ||
Scimitar::Engine::set_default_resources([Scimitar::Resources::User]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pond although this appears to work, there are linter complaints about writing it the more idiomatic way:
Scimitar::Engine.set_default_resources([Scimitar::Resources::User])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fair. Thanks. I'll do a PR in the near-ish future that's just a style change for the above onto main
/v1
but won't release a new gem version given there's no functional change.
README.md
updates that were overlooked after some prior enhancementsbyebug
during development