-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add unittests for group filtering (Azure) #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! I like mocking out the group calls.
@@ -25,7 +25,7 @@ func TestSessionStateSerialization(t *testing.T) { | |||
} | |||
encoded, err := s.EncodeSessionState(c) | |||
assert.Equal(t, nil, err) | |||
assert.Equal(t, 3, strings.Count(encoded, ":")) | |||
assert.Equal(t, 4, strings.Count(encoded, ":")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a test including a group spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be 5 now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 items in encoded
string, 4 :
delimiters. Looks fine to me.
The only thing that is missed here is PermittedGroups
but they are stored in AzureProvider
structure and shouldn't be visible in SessionState
so we're good
providers/provider_data.go
Outdated
@@ -13,6 +13,7 @@ type ProviderData struct { | |||
ProfileURL *url.URL | |||
ProtectedResource *url.URL | |||
ValidateURL *url.URL | |||
GroupsURL *url.URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type of Group API is unique to Azure, isn't it? I don't think it should live in ProviderData if that's the case.
providers/azure.go
Outdated
// startswith - not supported | "https://graph.microsoft.com/v1.0/me/memberOf?$filter=startswith(displayName,%27groupname%27)" | ||
// substring - not supported | "https://graph.microsoft.com/v1.0/me/memberOf?$filter=substring(displayName,0,2)%20eq%20%27groupname%27" | ||
|
||
requestUrl := "https://graph.microsoft.com/v1.0/me/memberOf%3F$select=displayName" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The %3F
here didn't work for me, swapping back to a ?
seemed to fix it. That was Ubuntu 16.04 and Go 1.6.2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks! Not sure what ate that ?
. @pasoroki, poke this and I'll give it a merge :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm, there should be ?
instead of %3F
* Add unittests for group filtering (Azure)
No description provided.