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

Scimitar::ActiveRecordBackedResourcesController doesn't use mapped id from scim_attributes_map #35

Closed
nhumble-sa opened this issue Dec 13, 2022 · 9 comments

Comments

@nhumble-sa
Copy link

If I map id to another column in scim_attributes_map it is ignored by the default ActiveRecord backed controller. This means I have to override not only find_record, but also url_for so that the generated URLs have the correct id in them.

@pond
Copy link
Member

pond commented Jan 17, 2023

While I'm adding specific support so that it does not rely on the ActiveRecord features I'm about to describe, I would note that your issues make it sound like your ActiveRecord model subclass is not properly declaring the name of its primary key column per this guide via this API:

self.primary_key = :foo

When you do this, ActiveRecord creates an alias so that your model instances will respond to #id as an alias for the true attribute name, #foo. This is why the guide has that orange warning saying, "Active Record does not support using non-primary key columns named id." - because id is already a reserved special name.

The alias means that ActiveRecordBackedResourcesController's use of resource.id works just fine, even if the underlying primary key column name differs. Likewise, when using Model.find(...), ActiveRecord will automatically look for the correct column name; there's no need to override find_record in such a case. If we imagine model User has declared that its primary key is foo as above, then:

User.find(1)

...yields:

User Load (0.6ms)  SELECT "users".* FROM "users" WHERE "users"."foo" = $1 LIMIT $2  [["foo", nil], ["LIMIT", 1]]

On the issue of url_for I am not sure what you mean. Path helpers have no connection at all to your model's attributes and their parameter requirements are fixed. Regardless of what your model column names might be, it is required that one always specifies an ID for a single-resource method like #show via the id parameter. For example:

url_for({controller: :users, action: :show, id: 1})
# => "http://www.example.com/users/1"
url_for({controller: :users, action: :show, foo: 1})
# No route matches {:action=>"show", :controller=>"users", :foo=>1} (ActionController::UrlGenerationError)
url_for({controller: :users, action: :show, foo: 22, id: 1})
# => "http://www.example.com/users/1?foo=22"

...so the ActiveRecordBackedResourcesController calls to to_scim(location: url_for(action: :show, id: ...) are in fact correct regardless of your primary key column name, assuming you are using conventional Rails routing and not doing anything funky (just a simple resource(s) map in routes.rb).

Personally, I've never been a huge fan of the alias approach so I'm looking at a change set for Scimitar that will only query on the named column, though it's ended up a larger footprint than I expected. It should at least allow people to avoid using the ActiveRecord primary key declaration if they for some reason decided they didn't want it present.

@pond
Copy link
Member

pond commented Jan 17, 2023

I do note that a recent change to #index (v2.2.0) would probably fall foul of a default order of id ASC, which is fixed by the current change set and would be worked around by specifying an explicit other order.

@pond
Copy link
Member

pond commented Jan 17, 2023

#42 is now in review.

I note that the test suite passes whether or not MockUser calls self.primary_key=... so while this isn't really an official, tested mode of operation, it appears to - as expected - work fine now. I would generally recommend sticking to ActiveRecord documentation and properly declaring your non-id primary key column names inside your models, though!

@nhumble-sa
Copy link
Author

I have need to use a different column as we have an id column that is the primary key, but don't want to expose it externally and instead use a uuid column that was added more recently. From a security standpoint, you don't want to expose the number of users in your system or allow their access by a guessable identity.

@pond
Copy link
Member

pond commented Jan 25, 2023

@acceptto-nhumble We've been a bit overloaded but I'm trying to escalate the review of #42 so we can get that new version pushed out.

@nhumble-sa
Copy link
Author

No worries, I just wanted to clarify my use-case.

bagp1 added a commit that referenced this issue Jan 25, 2023
…keys/issue-35

Address issue #35, with caveats noted in issue #35 discussion
@pond
Copy link
Member

pond commented Jan 26, 2023

@acceptto-nhumble Yeah, that was useful, thanks.

I just pushed v2.3.0 to RubyGems, so that's available. I'll back-port to V1 when I get a moment.

@pond
Copy link
Member

pond commented Jan 26, 2023

...and that's now done, via #44 yielding version 1.4.0.

https://rubygems.org/gems/scimitar/versions

If this is working for you (or not!), please let me know so I can update the issue status. Thanks!

@pond
Copy link
Member

pond commented Feb 28, 2023

...given no response, I'm going to make the assumption that this is resolved to the OP's satisfaction and close the issue.

@pond pond closed this as completed Feb 28, 2023
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

No branches or pull requests

2 participants