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

Basic Groups support #48

Closed
wants to merge 14 commits into from
Closed

Basic Groups support #48

wants to merge 14 commits into from

Conversation

valerauko
Copy link

@valerauko valerauko commented Apr 23, 2021

Why?

Support for SCIM user Groups is currently missing
Closes #46

What?

  • add options to configure how to handle Groups
  • add API endpoints to deal with Groups

Caveats

  • The PUT updating of Group members is a bit hacky.

Testing Notes

I'm developing this using Okta as well so there shouldn't be anything extra to set up.

  • sync Groups
  • update Group name (or similar mapped attribute)
  • update Group membership
  • delete Group (delete after unlink)

Todo

I'm planning to mark this "ready for review" once I add rspec tests and fix lint violations

Alternatives Considered

There is the option to build a more complete User association handling as outlined in #39, but I felt that's a bit too big to chew for this purpose.

# like [:names, 0, :givenName]. `.dig` can then use that path
# against the params to translate the :name attribute to "John".

def path_for(attribute, object = controller_schema, path = [])
Copy link

Choose a reason for hiding this comment

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

Metrics/CyclomaticComplexity: Cyclomatic complexity for path_for is too high. [7/6]
Metrics/MethodLength: Method has too many lines. [16/10]

groups = @company
.public_send(ScimRails.config.scim_groups_scope)
.where(
"#{ScimRails.config.scim_groups_model.connection.quote_column_name(query.attribute)} #{query.operator} ?",
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [118/80]

@@ -0,0 +1,66 @@
module ScimRails
class ScimGroupsController < ScimRails::ApplicationController
Copy link

Choose a reason for hiding this comment

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

Style/Documentation: Missing top-level class documentation comment.

@@ -0,0 +1,79 @@
module ScimRails
class ScimGroupsController < ScimRails::ApplicationController
def index
Copy link

Choose a reason for hiding this comment

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

Metrics/AbcSize: Assignment Branch Condition size for index is too high. [36.24/15]
Metrics/MethodLength: Method has too many lines. [23/10]

@@ -61,20 +69,24 @@ def user_response(user)
# send those symbols to the model, and replace the symbol with
# the return value.

def find_value(user, object)
case object
def find_value(object, schema)
Copy link

Choose a reason for hiding this comment

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

Metrics/AbcSize: Assignment Branch Condition size for find_value is too high. [17.75/15]
Metrics/MethodLength: Method has too many lines. [18/10]

@@ -0,0 +1,10 @@
class CreateGroups < ActiveRecord::Migration[5.0]
Copy link

Choose a reason for hiding this comment

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

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

@@ -0,0 +1,10 @@
class CreateGroupUsers < ActiveRecord::Migration[5.0]
Copy link

Choose a reason for hiding this comment

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

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20181206184313) do
ActiveRecord::Schema.define(version: 2021_04_23_075950) do
Copy link

Choose a reason for hiding this comment

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

Style/NumericLiterals: Use underscores(_) as decimal mark and separate every 3 digits with them.

# from scratch. The latter is a flawed and unsustainable approach (the more migrations
# you'll amass, the slower it'll run and the greater likelihood for issues).
# This file is the source Rails uses to define your schema when running `bin/rails
# db:schema:load`. When creating a new database, `bin/rails db:schema:load` tends to
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [84/80]

# system, you should be using db:schema:load, not running all the migrations
# from scratch. The latter is a flawed and unsustainable approach (the more migrations
# you'll amass, the slower it'll run and the greater likelihood for issues).
# This file is the source Rails uses to define your schema when running `bin/rails
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [82/80]

@@ -0,0 +1,11 @@
FactoryBot.define do
Copy link

Choose a reason for hiding this comment

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

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

@valerauko valerauko marked this pull request as ready for review April 23, 2021 10:34
@wernull
Copy link
Member

wernull commented Apr 26, 2021

@valerauko thanks for this PR! It looks like a lot of good stuff at first glance. We'll try to get to it some time this week

@wernull
Copy link
Member

wernull commented May 12, 2021

@valerauko sorry for the delay. Things have got a little busy and this is something we really want to put proper attention on

@valerauko
Copy link
Author

No worries, take your time

@valerauko valerauko closed this Nov 19, 2021
@valerauko valerauko deleted the groups branch November 19, 2021 07:56
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.

Handle Groups
2 participants