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

Update plugin to use github.com/microsoftgraph/msgraph-sdk-go #62

Merged
merged 22 commits into from
Aug 9, 2022

Conversation

Subhajit97
Copy link
Contributor

@Subhajit97 Subhajit97 commented Jul 27, 2022

  1. azuread_application

    • Old and new data matched
    • Paging tested
    • Limit tested
    • N/A no mod queries using it
  2. azuread_conditional_access_policy

    • Old and new data matched
    • Paging tested
    • Limit tested
    • N/A no mod queries using it
  3. azuread_directory_role

    • Old and new data matched
    • Paging - N/A due to not supported
    • Limit tested
    • N/A no mod queries using it
  4. azuread_domain

    • Old and new data matched
    • Paging tested
    • Limit tested
    • N/A no mod queries using it
  5. azuread_group

    • Old and new data matched
    • Paging tested
    • Limit tested
    • N/A no mod queries using it
  6. azuread_identity_provider

    • Old and new data matched
    • Paging tested
    • Limit tested
    • N/A no mod queries using it
  7. azuread_service_principal

    • Old and new data matched
      • verified_publisher - (Removed) Getting null value for all record
      • published_permission_scopes - (Removed) Values are same with oauth2_permission_scopes
    • Paging tested
    • Limit tested
    • N/A no mod queries using it
  8. azuread_sign_in_report

    • Old and new data matched
    • Paging tested
    • Limit tested
    • N/A no mod queries using it
  9. azuread_user

    • Old and new data matched
      • refreshTokensValidFromDateTime - (Removed) This property is no longer valid, use signInSessionsValidFromDateTime instead.
    • Paging tested
    • Limit tested
    • Compliance mods queries using this table tested

@Subhajit97 Subhajit97 marked this pull request as ready for review July 28, 2022 16:05
@cbruno10 cbruno10 self-requested a review August 4, 2022 21:01
Copy link
Contributor

@cbruno10 cbruno10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Subhajit97 Are there other breaking changes, beside those listed in the current PR description? I found one column commented out in azuread_group, is_subscribed_by_mail, but that wasn't mentioned in the PR body. There are a few other issues opened by you in https://github.com/microsoftgraph/msgraph-sdk-go, but those are still open (do we have a workaround)?

Also, do we have a plan for switching over the identity provider table? I see an issue you created in microsoftgraph/msgraph-sdk-go#223, but that was closed due to inactivity from our side, so I'm not sure where that was left off.

In your data for azuread_group, I saw that the order of the owner_ids and member_ids column values were sometimes changed in the arrays, is Azure AD doing this? Or did the data in the actual resource change in between queries?

Is the data up to date in the query results repo?

Also, can you please and fix any linting errors? Thanks!

config/azuread.spc Show resolved Hide resolved
if err != nil {
return nil, err
return nil, fmt.Errorf("error creating client: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a error logging message here, e.g.,

plugin.Logger(ctx).Error("azuread_application.listAdApplications", "connection_error", err)

Do we also need the fmt.Errorf message here then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever we decide here, we should also make changes across the other functions/files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right @cbruno10 that makes sense. I updated all the tables to use the same logging standard.

azuread/table_azuread_application.go Show resolved Hide resolved
// }
// }
// }
// {Name: "is_subscribed_by_mail", Type: proto.ColumnType_BOOL, Description: "Indicates whether the signed-in user is subscribed to receive email conversations. Default value is true.", Transform: transform.FromMethod("GetIsSubscribedByMail")},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change, since the column looks like it was there before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbruno10, initially, I was getting some errors while trying to access this column. I've checked the SDK documentation, and in the new MSGraph SDK, it is supported only on the Get group API GET /groups/{ID} and returned only on $select. I had updated the table to add it back.


// Restrict the limit value to be passed in the query parameter which is not between 1 and 999, otherwise API will throw an error as follow
// unexpected status 400 with OData error: Request_UnsupportedQuery: Invalid page size specified: '1000'. Must be between 1 and 999 inclusive.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we remove this comment line, but kept the one above (which doesn't make sense anymore since it ends with throw an error as follow)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbruno10, thanks for pointing it out. This one was not intended; I have added it back.

KeyColumns: plugin.KeyColumnSlice{
// Key fields
{Name: "display_name", Require: plugin.Optional},
{Name: "account_enabled", Require: plugin.Optional},
{Name: "account_enabled", Require: plugin.Optional, Operators: []string{"<>", "="}},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're explicitly defining the Operators now, what qual operators are enabled by default on key columns? Is it normally just =?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, @cbruno10, if we don't explicitly define any Operators, it automatically adds a single equal (=).
https://github.com/turbot/steampipe-plugin-sdk/blob/main/plugin/key_column.go#L103

azuread/table_azuread_application.go Show resolved Hide resolved
{Name: "reply_urls", Type: proto.ColumnType_JSON, Description: "The URLs that user tokens are sent to for sign in with the associated application, or the redirect URIs that OAuth 2.0 authorization codes and access tokens are sent to for the associated application."},
{Name: "service_principal_names", Type: proto.ColumnType_JSON, Description: "Contains the list of identifiersUris, copied over from the associated application. Additional values can be added to hybrid applications. These values can be used to identify the permissions exposed by this app within Azure AD."},
{Name: "tags_src", Type: proto.ColumnType_JSON, Description: "Custom strings that can be used to categorize and identify the service principal.", Transform: transform.FromField("Tags")},
{Name: "verified_publisher", Type: proto.ColumnType_JSON, Description: "Specifies the verified publisher of the application which this service principal represents."},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the old SDK return data for this column, if so do you have an example of that data? Also, does the base Graph API still return this data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old SDK returns empty object {} for this column. I have a discussion with the MSGraph SDK team microsoftgraph/msgraph-sdk-go#228 (comment), and this property is not supposed to work for ServicePrincipals.

{Name: "created_date_time", Type: proto.ColumnType_TIMESTAMP, Description: "The time at which the user was created.", Transform: transform.FromMethod("GetCreatedDateTime")},
{Name: "mail", Type: proto.ColumnType_STRING, Description: "The SMTP address for the user, for example, jeff@contoso.onmicrosoft.com.", Transform: transform.FromMethod("GetMail")},
{Name: "mail_nickname", Type: proto.ColumnType_STRING, Description: "The mail alias for the user.", Transform: transform.FromMethod("GetMailNickname")},
{Name: "password_policies", Type: proto.ColumnType_STRING, Description: "Specifies password policies for the user. This value is an enumeration with one possible value being DisableStrongPassword, which allows weaker passwords than the default policy to be specified. DisablePasswordExpiration can also be specified. The two may be specified together; for example: DisablePasswordExpiration, DisableStrongPassword.", Transform: transform.FromMethod("GetPasswordPolicies")},
{Name: "refresh_tokens_valid_from_date_time", Type: proto.ColumnType_TIMESTAMP, Description: "Any refresh tokens or sessions tokens (session cookies) issued before this time are invalid, and applications will get an error when using an invalid refresh or sessions token to acquire a delegated access token (to access APIs such as Microsoft Graph)."},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an issue open for this in microsoftgraph/msgraph-sdk-go#227, are we getting data for this column, or have some workaround for it already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbruno10 As per the SDK team, they had logged an issue for adding those data in response.
microsoftgraph/msgraph-sdk-go#227 (comment)

Currently, we have a workaround to get these data. I had already updated the table to gather those data
https://github.com/turbot/steampipe-plugin-azuread/pull/62/files#diff-0355132864b3978d9dee70e144ba0b6b1328b6c14a4afbb7f33bc610eaa88f70R143

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbruno10 I have removed column refresh_tokens_valid_from_date_time, sincerefreshTokensValidFromDateTime should not be present anymore. Use sign_in_sessions_valid_from_date_time instead.

Comment on lines 99 to 100
{Name: "resource_behavior_options", Type: proto.ColumnType_JSON, Description: "Specifies the group behaviors that can be set for a Microsoft 365 group during creation. Possible values are AllowOnlyMembersToPost, HideGroupInOutlook, SubscribeNewGroupMembers, WelcomeEmailDisabled."},
{Name: "resource_provisioning_options", Type: proto.ColumnType_JSON, Description: "Specifies the group resources that are provisioned as part of Microsoft 365 group creation, that are not normally part of default group creation. Possible value is Team."},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these 2 columns, are we getting proper data? I saw an issue in microsoftgraph/msgraph-sdk-go#225, but no follow-ups in it from our side and it's still open.

Copy link
Contributor Author

@Subhajit97 Subhajit97 Aug 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbruno10 As per the SDK team, they had logged an issue for adding those data in response.
microsoftgraph/msgraph-sdk-go#225 (comment)

Currently, we have a workaround to get these data. I had already updated the table to gather those data
https://github.com/turbot/steampipe-plugin-azuread/pull/62/files#diff-2572ff1d29755c314dbeb51c592a532f7616f1141e9a2b7289a004a7e634cefcR209-R210

@Subhajit97 Subhajit97 linked an issue Aug 5, 2022 that may be closed by this pull request
@Subhajit97 Subhajit97 self-assigned this Aug 5, 2022
@Subhajit97 Subhajit97 requested a review from cbruno10 August 9, 2022 07:30
@Subhajit97 Subhajit97 changed the title WIP: Update plugin to use github.com/microsoftgraph/msgraph-sdk-go Update plugin to use github.com/microsoftgraph/msgraph-sdk-go Aug 9, 2022
@cbruno10 cbruno10 merged commit 0bd5311 into main Aug 9, 2022
@cbruno10 cbruno10 deleted the migrate-to-new-sdk branch August 9, 2022 19:55
@cbruno10
Copy link
Contributor

cbruno10 commented Aug 9, 2022

Note: I've merged this PR, even though the linting workflow is failing due to a timeout. I've run golangci-lint run --timeout=20m locally to check for any errors, but did not find any.

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

Successfully merging this pull request may close these issues.

Look into switching to official MS Graph Go SDK
2 participants