Skip to content

Support for multiple/mixed attribute types.#245

Open
doconnor-clintel wants to merge 23 commits intoapokalipto:masterfrom
careright:multiple-roles
Open

Support for multiple/mixed attribute types.#245
doconnor-clintel wants to merge 23 commits intoapokalipto:masterfrom
careright:multiple-roles

Conversation

@doconnor-clintel
Copy link
Contributor

@doconnor-clintel doconnor-clintel commented Dec 6, 2023

Revised version of #159

  • Fixes specs
  • Adds coverage for multiple groups, based off of okta samples

@doconnor-clintel
Copy link
Contributor Author

Okay, that's positive.

This feels quite... ugly... to mutate global state in a class when a configuration flag is set.

}
route "root to: 'home#index'"

# TODO: https://github.com/heartcombo/devise/blob/main/lib/generators/active_record/templates/migration.rb - string vs array
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, potentially there should be some fancy string replacement done in the migration to make it

t.string :groups, array: true

@doconnor-clintel doconnor-clintel marked this pull request as ready for review December 6, 2023 08:43
@doconnor-clintel doconnor-clintel changed the title Multiple roles Support for multiple/mixed attribute types. Dec 6, 2023
@doconnor-clintel
Copy link
Contributor Author

doconnor-clintel commented Dec 19, 2023

Annoying.

NoMethodError:
        undefined method `single' for {"first_name"=>["John"], "email"=>["john.smith@example.com"], "groups"=>["admin", "reporting"], "multiple_but_single"=>["1", "2", "3"], "lastName"=>["Smith"]}:Hash

However, that should be a OneLogin::RubySaml::Attributes object with those methods.

@@ -1,3 +1,3 @@
module DeviseSamlAuthenticatable
VERSION = "1.9.1"
VERSION = "2.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this actually a breaking change? It seems like it's backwards-compatible, which is great! In which case, we could release 1.10.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this as a major version bump, as the reconfiguration of the attributes-map.yml is a bit of a pain; and people will have to adjust their custom attribute resolvers slightly.

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