Skip to content
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

Group.GetMember question #20

Open
yriveiro opened this issue Feb 27, 2021 · 3 comments
Open

Group.GetMember question #20

yriveiro opened this issue Feb 27, 2021 · 3 comments
Labels
question Further information is requested

Comments

@yriveiro
Copy link
Contributor

Reading the code of this line:

func (c *GroupsClient) GetMember(ctx context.Context, groupId, memberId string) (*string, int, error) {

If I need to pass the tuple (group_id, member_id) and the return is an ID of the member, the method shouldn't be called IsMemberOf?

I would expect to have the full member object if I do a GetMember

@yriveiro yriveiro changed the title GetMember should be rewritten to IsMember Group.GetMember question Feb 27, 2021
@manicminer manicminer added the question Further information is requested label Feb 27, 2021
@manicminer
Copy link
Owner

Hi @yriveiro, thanks for the question! Members and owners have a bit of extra complexity because the member object is cast as a generic DirectoryObject by the API - since it can be a user, a service principal, or another group. This is definitely something that can be improved on, if you have any ideas or preferences I'd love to hear them :)

@yriveiro
Copy link
Contributor Author

I'm using the GetMember to check if an object from the Directory Object is a member of a group once the client doesn't provide it and iterate the full list of members of the group is a kinda waste of CPU cycles.

I would change this to:

func (c *GroupsClient) IsMemberOf(ctx context.Context, groupId, memberId string) (bool, error) {
    _, status, err := c.GetMember(groupId, memberId string)
    if err != nil {
        false, err
    }
    
    return true
}

And the equivalent for IsOwnerOf also and let GetMember/GetOwner return the full response from the API once has some extra info.

@manicminer
Copy link
Owner

I can see a GroupsClient.isMemberOf() method being useful, I'll look to add that (you're also welcome to send in a PR with the version in your last comment :).

I'm planning to look at ways to implement a generic DirectoryObjectsClient and if that works out then I think it makes sense to hook up GroupsClient members/owners methods to work with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants