Missing fixes from 3.1.2 in 3.3.x

[irstevenson]

There was a significant commit that made up 3.1.2 that has not been merged back onto master and thereby not develop:

https://github.com/jeffwils/grails-spring-security-saml/commit/60ab68fe3a8d1f9c74ee1d3de6da53758f53ab0b

Do we still need this fix?

If yes, we need to merge and test.

[valentingoebel]

If I have understood the fix correctly it allows you to specify a list of "role->saml group" maps, rather than a single "role->saml group" map.

3.1.2

userGroupToRoleMapping:
    - ROLE_MEMBER: member
    - ROLE_MEMBER: admin

current

userGroupToRoleMapping:
    ROLE_MEMBER: member
    ROLE_MEMBER: admin # not possible

The problem with the single map approach is that it's only possible to assign a role to a single group.

If we're going to break backwards compatibility wouldn't it be far more intuitive to have a single map that does "saml group -> list of roles"? After all the name of the property is "userGroupToRoleMapping" but for some reason the current version does the opposite namely "roleToUserGroupMapping".

new

userGroupToRoleMapping:
    member:
        - ROLE_MEMBER
    admin:
        - ROLE_MEMBER

Alternatively we could also support a mapping from saml groups to spring security core groups.

[valentingoebel]

There is also a minor change at the bottom of the commit. We definitively need this change.

defaultFailureUrl = conf.saml.loginFailUrl ?: '/login/authfail?login_error=1'

[irstevenson]

Okay, there's a bit more in here then I first noticed - was just just looking at the missed commits previously. So Let's list them out:

1. Change to support multiple groups per role - as you cover above;
2. Change which resulted in needing a list of maps for providers - as covered in #10 and corrected in release v3.3.0;
3. Added support back in for specifying a default IDP - see line 267 of SpringSecuritySamlGrailsPlugin.groovy. Seems to relate to #5;
4. Processing of configuration option defaults for Auto Assign Authorities and Auto Create - see lines 278-279 in SpringSecuritySamlGrailsPlugin.groovy; and
5. Adding support for specifying default failure URL on line 302 in SpringSecuritySamlGrailsPlugin.groovy - as you mention above;

It'd be good to move forward with a collection of atomic commits for these. Some are really simple and can be knocked on the head into develop one by one. They are:

3 and 5

Number 4 possibly also fits into this category, however what are you thoughts on that @valentingoebel , does your recent re-instatement of a default config fulfill this? (If not, could it, meaning 4 would no longer be needed.)

Number 2 is obviously resolved, and so that only leaves number 1 - the mapping of SAML groups to roles.

I gave this a fair amount of thought, and I think I came to:

• The initial simple one-to-one mapping probably covers most cases;
• However, there are then more complicated cases where 'many' starts coming into play; and
• Really, the best fit is probably application specific. And so,
• Your suggestion of the many-to-many mappings provided by the groups in Spring Security Core is possibly the best way to go.

We just need to keep it optional still - because for example, I don't plan to do any SAML group mapping in my app (instead it's an explicit role assignment per user who is also explicitly added).

So, next actions would be something like:

• I'll do two commits to knock off 3 and 5;
• I'm interested to hear @valentingoebel comment on the defaults with item 4; and
• I think it'd be best if we setup a new 'enhancement' ticket for SAML group to role mapping support to flesh out what should be done.

[valentingoebel]

The default value of autoCreate.key is defined in plugin.yml as "username" and this is sufficient as a replacement.

autoCreate.assignAuthorities is true by default but it is only used in SpringSamlUserDetailsService where it is guarded by if(autoCreate.active).

We don't need to add those lines of code to the latest version.

[irstevenson]

Basic items have been addressed, the remaining item of Group to Role mapping has now been detailed and split over into #41.

[valentingoebel]

The behavior of defaultIDP no longer matches the README.MD file. The EntityID of the IDP has to be entered rather than the name that was specified in the providers block.

[irstevenson]

Yeah, that seemed to come from no where suddenly. That was the item I mentioned in matrix a few weeks back where one day all was working fine, and then suddenly the next I had to rework my providers block - I didn't realise it was only the defaultIdp, so I also changed the provider key.

I'm not sure what triggered this though... I wonder if some underlying dependency changed. :confused: But I don't think it was specific to new code in 3.3.x - because at one stage all was fine with 3.3.x. :confused:

Should we set up a new issue for this? And/or, do you have more info?

[valentingoebel]

It's probably caused by your last commit which re-enabled the defaultIDP setting. I feel like the new behavior is correct though. A metadata file defined in the providers section could in theory contain more than one service provider so we need the full EntityID to differenciate between service providers.

[irstevenson]

Ahhh, gotcha. I see now, man I was so stressed at the time with to many things happening at once I missed the simple thing.

Alright, so all that makes sense and yes we should just update the doco - but we'll want to wait until #46 is merged - however it looks like we also need to update the example blocks in the doco too (not just the table). I'll create a ticket so that's clear. ;)

Mind you, there's probably a bit that needs to be done on the doco, so that'll be worthwhile either-way.