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

Get-GSGroupMembers doesn't take Group ID as Identity #249

Closed
Ephesoft-Stitus opened this issue Nov 11, 2019 · 17 comments
Closed

Get-GSGroupMembers doesn't take Group ID as Identity #249

Ephesoft-Stitus opened this issue Nov 11, 2019 · 17 comments
Assignees
Labels
bug documentation update Documentation update needed for this

Comments

@Ephesoft-Stitus
Copy link

Get-GSGroupMembers Identity parameter help states:

The email or GroupID of the target group

However, passing the group ID to the command results in a 404 because the key isn't found. It only works with passing the email of the group as the identity.

This does not work

$Group = Get-GSGroup 'mygroupid'
$Group.id | Get-GSGroupMembers

Result:

Get-GSGroupMember : Exception calling "Execute" with "0" argument(s): "Google.Apis.Requests.RequestError
Resource Not Found: groupKey [404]

This does work:

$Group = Get-GSGroup 'mygroupid'
$Group.email | Get-GSGroupMembers
@scrthq
Copy link
Member

scrthq commented Nov 11, 2019

Thanks for pointing this out, @Ephesoft-Stitus !! Checking to see if there's a different property on the request object that needs to be filled instead if the ID is passed, as the documentation on Google's side explicitly states that either the email or unique ID should be acceptable: https://developers.google.com/admin-sdk/directory/v1/reference/members/list

@scrthq scrthq self-assigned this Nov 11, 2019
@scrthq scrthq added bug documentation update Documentation update needed for this labels Nov 11, 2019
@scrthq
Copy link
Member

scrthq commented Nov 16, 2019

Found the issue, working on a fix now! Same fix will be applied to other Group functions as well where applicable. Thanks for raising this, @Ephesoft-Stitus !

@scrthq
Copy link
Member

scrthq commented Dec 28, 2019

hey @Ephesoft-Stitus - sorry on the delay here, this has become a bigger fix than anticipated due to Group IDs also being strings, so the same method of testing if the value passed is an email or an ID as we do with Users isn't applicable (we do that currently by testing if the value is able to be cast as a long integer).

Issue source: Email name parts (e.g. mike instead of mike@domain.com) are accepted for convenience via the function automatically appending the Domain to the value if it is not recognized as an ID and does not have an @ in the string. This is an incredibly convenient feature of PSGSuite that I'd like to not break. Since Group IDs aren't easily identified, we run into this issue.

I initially tried approaching this using a new Id parameter and parameter sets (e.g. Email vs Id), but some of the group functions have multiple parameter sets already, making that approach not too feasible.

I tried nesting try/catch statements, but I feel it not only adds to code complexity unnecessarily, but also elongates the overall completion time of the function since you'd be literally retrying the same API call underneath the hood if the first attempt with Domain appended failed to confirm if it was actually an ID that was passed. For example, if you do actually pass a non-existent email/ID to the function, it would need to try and fail twice before returning.


My current thought is to add a switch parameter to the Group functions to block the appending of the domain and try with whatever was passed as-is. This would effectively allow you to state that you know you are either passing full email addresses or IDs to the Identity / Email parameter and to not attempt to resolve the address if @ is not found in the value. Current suggested parameter name is DoNotResolveGroupIdentity with a shorter alias of DoNotResolve.

Thoughts?

@Ephesoft-Stitus
Copy link
Author

Ephesoft-Stitus commented Dec 28, 2019 via email

@scrthq
Copy link
Member

scrthq commented Dec 28, 2019

It might work out if we're able to tighten up that RegEx a bit, I'll do some testing as well

@WJurecki
Copy link
Contributor

@scrthq,

Just as a reference to tighten the RegEx I found that \d{2}([a-z]|\d){13} is (only) slightly tighter and passed all of our domain's group IDs.

@scrthq
Copy link
Member

scrthq commented Dec 29, 2019

The question is does that match any email name parts for groups? on mobile, but something like...

$null -eq (Get-GSGroup | Where-Object {$_.Email.Split('@')[0] -cmatch '\d{2}([a-z]|\d){13}'})

@WJurecki
Copy link
Contributor

In my use-case, I have no conflicts, so your test returns True.

Will this apply to everyone all the time? ... there is that rule of absolutes.

@scrthq
Copy link
Member

scrthq commented Dec 29, 2019

Mine returns False, so this would break my own domain basing that on RegEx. The flipside to this is that it's a non-issue if someone passes a full email, since that wouldn't even attempt to resolve and append a domain name. As someone where this would be impacting, the benefit outweighs the gain IMHO since I'm typically passing the full email anyway. Total impact for me is 3 groups out of 2313 total, so 0.13% of my groups run that risk, which is tiny. I tested the inverse as well and confirmed that the pattern matches every single group ID in my domain.

image

@WJurecki - This would apply to all -GSGroup functions, but only in the instance where someone passes just the email name part as the group ID. For what it's worth, we've run the same risk on the user side for a while now, but the is a smaller likelihood that someone would give a user an email address that's a bigint and could be confused with a User ID vs someone setting a group email to something that is 2 numbers followed by 13 a-z0-9 chars exactly.

@Ephesoft-Stitus @WJurecki - Thoughts on going the RegEx route given that it will always work as intended if the full email is passed? Or worth avoiding considering we've already proven that the RegEx route would still match false positives of actual emails?

@WJurecki
Copy link
Contributor

@scrthq,

I think the RegEx route would be a very acceptable solution because as you point out, passing a full email address always works. (Honestly I have always passed full email addresses and have never depended on the domain being added to what I passed.)

@scrthq
Copy link
Member

scrthq commented Dec 29, 2019

Tightened the RegEx up a little bit more and have narrowed it down to 1 unavoidable match against my 2313 groups now, still matching 100% of Ids as well: ^\d{2}[a-z\d]{13}$

@scrthq
Copy link
Member

scrthq commented Dec 29, 2019

Full pattern to avoid appending @domain.com to: ^([\w._%+-]+@[\w.-]+\.[a-zA-Z]{2,}|\d{2}[a-z\d]{13})$

@WJurecki
Copy link
Contributor

@scrthq,

That looks like it will work.

There are a couple minor things about the RegEx.

In the capture before the @ you use [\w._%+-]+, but the \w already includes the _ and therefore could be simplified to [\w.%+-]+.

In the domain name itself you are using [\w.-]+ which allows _ in the domain name, where as best I understand, is not allowed and therefore could be corrected to [a-zA-Z\d.-]+

For a final RegEx of ^([\w.%+-]+@[a-zA-Z\d.-]+\.[a-zA-Z]{2,}|\d{2}[a-z\d]{13})$

Obviously there are other cases where this won't detect invalid domain names (starting or ending with a -, having multiple consecutive ., length limits, currently allowed non-English characters, etc. But where do you stop with the small use cases?)

@scrthq
Copy link
Member

scrthq commented Dec 29, 2019

fair call outs! RegEx for email address verification can vary a bit, but definitely valid points. I've updated the code on my end with the final RegEx you provided =]

Side note - testing #216 as well right now, hoping to have all of these rolled out by tonight at some point 🙂

@scrthq scrthq mentioned this issue Dec 29, 2019
scrthq added a commit that referenced this issue Dec 29, 2019
## 2.35.0 - 2019-12-29

* [Issue #216](#216) - _Thank you, [@WJurecki](https://github.com/WJurecki)!_
    * Added `Add-GSSheetValues` to use the native `Append()` method instead of `BatchUpdate()` to prevent needing to calculate the last row like you do with `Export-GSSheet`. Since the input for this method has additional options and the output differs from what `Export-GSSheet` outputs, this has been moved to a unique function to prevent introducing breaking changes to `Export-GSSheet`.
* [Issue #221](#221)
    * Added: `Invoke-GSUserOffboarding` function to wrap common offboarding tasks for ease of access management automation.
* [Issue #248](#248)
    * Fixed `Get-GSSheetInfo` so it no longer defaults `-IncludeGridData` to `$true` if not specified in `$PSBoundParameters`.
* [Issue #249](#249)
    * Updated private function `Resolve-Email` with new `IsGroup` switch, then cleaned up all `*-GSGroup*` functions to use it so that Group ID's are respected based on RegEx match.
* [Issue #252](#252)
    * Added: `Archived` parameter to `Update-GSUser` to enable setting of Archived User licenses.
* Miscellaneous
    * Swapped instances of `Get-StoragePath` for `Get-ConfigurationPath` in `Import-SpecificConfiguration` and `Set-PSGSuiteConfig` to avoid alias related issues with PowerShell 4.0
@scrthq
Copy link
Member

scrthq commented Dec 29, 2019

Alright @Ephesoft-Stitus - Updates have been pushed out in v2.35.0 that should allow this to work as intended! Please let me know if you hit any issues.

@scrthq scrthq closed this as completed Dec 29, 2019
@Ephesoft-Stitus
Copy link
Author

Thanks @scrthq - I missed the updated in late December as I was travelling. Just checking back on this now as I realized I hadn't seen an update in a long time. This is really, really, great and should significantly improve performance for my use case.

@scrthq
Copy link
Member

scrthq commented Mar 4, 2020

@Ephesoft-Stitus No problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug documentation update Documentation update needed for this
Projects
None yet
Development

No branches or pull requests

3 participants