Adding support for allowed groups in SAML Connector#1544
Conversation
kenperkins
force-pushed
the
saml-groups
branch
2 times, most recently
from
6e2fd5c to
827f3bb
Compare
|
I've now added a few tests that test the groups behavior for SAML:
|
|
I've added docs with my latest commit. |
srenatus
left a comment
srenatus
left a comment
There was a problem hiding this comment.
Thanks for looking into this. You're right, there's no harm done in adding this before (or even without) harmonizing the groups filtering code among different connectors.
One inline comment, but this looks great 🚀
|
A bird in the hand is worth two in the bush, so let's get this in and keep the refactor briefly discussed in #1476 in mind. @kenperkins I'd like to get another maintainer to approve this; and would you mind squashing these commits? 😃 |
|
You need me to squash before merge? Do you not squash/merge as part of GH merge button? (That's fine, just looking for clarity). |
|
Usually just hitting merge 😉
…On Tue, 10 Sep 2019, 18:16 Ken Perkins, ***@***.***> wrote:
You need me to squash before merge? Do you not squash/merge as part of GH
merge button? (That's fine, just looking for clarity).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1544?email_source=notifications&email_token=AAGUR3VGXBA2SA2ODNVVSXLQI7B5LA5CNFSM4IVHTYS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6LVMUY#issuecomment-530011731>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGUR3XFUCSS7ACBNQN6ESLQI7B5LANCNFSM4IVHTYSQ>
.
|
|
I'm not sure you're waiting on me, I'm totally ok with my commits to be squashed, if you need me to do it, please give me clear guidance and I'll re-push. |
|
@kenperkins sorry, was on thumbs. I'd appreciate if you could squash them. Thanks! There's a bit of commit guidance in the dev docs: https://github.com/dexidp/dex/blob/60264d440cd668fe601f8ade81e100c8cd95e7f1/Documentation/dev-dependencies.md#composing-commits -- but we haven't been too strict in following these lately 😉 Thanks again for your quick action here 🚀 |
- 4 new tests - Doc changes to use the group filtering
kenperkins
force-pushed
the
saml-groups
branch
from
9e51b15 to
285c1f1
Compare
|
So updated. |
|
Is there anything I can do here to help? |
|
@srenatus is there another maintainer that could assist in a review here? |
|
@kenperkins I suppose so! 🏓 @JoelSpeed @bonifaido could you please have a look? |
| test.run(t) | ||
| } | ||
|
|
||
| func TestGroupsWhitelist(t *testing.T) { |
bonifaido
left a comment
bonifaido
left a comment
There was a problem hiding this comment.
LGTM! Thank you for the PR.
|
🎉 |
|
Hey @kenperkins, do you have working config for allowedGroups? |
5 participants
I'm not suggesting this should be taken as is, but I wanted to use it as a conversation starter on adding SAML support for group filtering. This is based on conversation in #1476
I thought briefly about a generalized approach but right now I just wanted to get a primitive implementation vetted and ideally merged, and then follow that with figuring out what it looks like across providers.
Obviously, there are no tests or docs in this change.