search

morincer / teamcity-plugin-saml

The plug-in adds ability to authenticate users by SAML-based SSO providers (like Okta, Onelogin etc.)
MIT License
24 stars 16 forks source link

Add inital support for assigning groups from Okta response. #52

Closed fatmcgav closed 4 years ago

fatmcgav commented 4 years ago

This is an issue commit that adds support for assigning TeamCity groups based on the SAML response from OKTA.

New SAML response was generated using Samling, with 2 groups assigned.

Fixes #42

fatmcgav commented 4 years ago

@morincer I'd be interested in getting some early feedback on this PR.

Do the changes look sane to you? Any issues you can foresee or additional functionality you'd like to see added?

morincer commented 4 years ago

Thanks for the PR - I'll review it today and get back to you.

fatmcgav commented 4 years ago

Cool, cheers.

I'm working on the UI component currently, so should have that ready later this evening or tomorrow morning BST.

fatmcgav commented 4 years ago

@morincer So I've just added some additional commits which I think fulfil your comments.

I'm still struggling to test the Ui changes though. I've followed the steps outlined in the README, but for whatever reason the new fields added in 5a2d82e3e9167f316bb5d31cf2a24c11c706b4dc don't actually render. Are you able to provide any pointers?

morincer commented 4 years ago

@fatmcgav , well, I just usually execute "mvn package tc-sdk:reload" from the repository root and then reload plugin in admin UI and it does the trick. I'll check your code.

fatmcgav commented 4 years ago

@fatmcgav , well, I just usually execute "mvn package tc-sdk:reload" from the repository root and then reload plugin in admin UI and it does the trick. I'll check your code.

So I managed to get the UI to reload by nuking the servers dir and then re-starting stuff...

One issue I have just come across is that Okta doesn't have a friendly "Human" name on the response. Instead, it looks like it can pass explicit fields for user.firstName and user.lastName.

What do you think about adding a "Composite" attribute type which can be used to join a comma separated list of SAML attributes?

morincer commented 4 years ago

One issue I have just come across is that Okta doesn't have a friendly "Human" name on the response. Instead, it looks like it can pass explicit fields for user.firstName and user.lastName.

What do you think about adding a "Composite" attribute type which can be used to join a comma separated list of SAML attributes?

Manipulating SAML attributes is not what exactly the plugin is for. You can achieve the same on IdP level - Okta allows to use expressions when defining a custom attribute -

image

fatmcgav commented 4 years ago

One issue I have just come across is that Okta doesn't have a friendly "Human" name on the response. Instead, it looks like it can pass explicit fields for user.firstName and user.lastName. What do you think about adding a "Composite" attribute type which can be used to join a comma separated list of SAML attributes?

Manipulating SAML attributes is not what exactly the plugin is for. You can achieve the same on IdP level - Okta allows to use expressions when defining a custom attribute -

image

Yeh, after I posted the comment, I managed to find an example in the Okta KB that set the value to user.fullName. This value doesn't render in the drop-down, which is what made me think Okta didn't support it...

For reference, my Okta config looks like: image

fatmcgav commented 4 years ago

@morincer So I've just pushed another commit which I think rounds the code out fully.

In testing against a Dev okta instance, I'm able to create a new user in TC with groups assigned, and add and remove TC group assignments based on Okta group membership.

LMK what you think :)

fatmcgav commented 4 years ago

I've also just fixed the merge conflict :)

fatmcgav commented 4 years ago

A quick example of the login flow: TC-Login-SSO

fatmcgav commented 4 years ago

Right, I think this is ready for another look @morincer :)

fatmcgav commented 4 years ago

@morincer thanks for merging.

Is it possible to get a new release cut with this included?

morincer commented 4 years ago

Just released 1.3 with this change (just added small null change).

fatmcgav commented 4 years ago

Great, thanks for that @morincer

anherrera commented 2 years ago

This change is great! Allows us to use Okta with Teamcity!

I ran into two edge cases and I solved one of them but not the other:

  1. Okta group has spaces... Replace the \w+ regex with just .*
  2. Okta group has special characters, such as & - how do I set the group key to account for this? If I have & in the group key at all TeamCity breaks and won't add roles to it. Woops. I suppose we could create another group name in Okta without & in it but I was hoping there was another way.
morincer commented 2 years ago

Hi @anherrera Are you able to show me a portion of the SAML request with failure group names so I run a couple of tests for it? In general, I guess a good approach would be to replace non-valid group symbols with _ or something like that.