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

Application improvements #474

Merged
merged 20 commits into from
Jul 21, 2021
Merged

Application improvements #474

merged 20 commits into from
Jul 21, 2021

Conversation

manicminer
Copy link
Contributor

@manicminer manicminer commented Jun 30, 2021

  • Case insensitive matching on UUIDs for passwords and certificates
  • Role/scope validation also checks IDs now that users must set it themselves
  • Extensive whole-resource validation when sign_in_audience is changed to include personal accounts, to avoid breakage
  • Updates Hamilton to v0.20.0 for fixes, features and improved odata query support

New Applications Features

  • Support additional properties in the api block
    • accept_mapped_claims
    • known_client_applications
    • requested_access_token_version
  • Export the disabled_by_microsoft attribute
  • Support the logo_url, marketing_url, privacy_statement_url, support_url and terms_of_service_url properties
  • Support the device_only_auth_enabled property
  • Support the oauth2_post_response_required property
  • Export the publisher_domain attribute (this is readonly, awaiting feedback on how we can set this)
  • Support the public_client block
  • Support the single_page_application block
  • Export two computed typemap attributes: app_role_ids and oauth2_permission_scope_ids to alleviate typeset handling with config boilerplate

Closes: #188
Closes: #244
Closes: #286
Closes: #320
Closes: #436
Closes: #462
Closes: #469
Closes: #477

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

aside from a few comments on schema this looks good!

internal/services/applications/application_data_source.go Outdated Show resolved Hide resolved
internal/services/applications/application_data_source.go Outdated Show resolved Hide resolved
internal/services/applications/application_resource.go Outdated Show resolved Hide resolved
},

"oauth2_post_response_required": {
Description: "Specifies whether, as part of OAuth 2.0 token requests, Azure AD allows POST requests, as opposed to GET requests.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this says "post allowed" vs the property name "response_required"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a tricky one since the docs are ambiguous on the behaviour. I'll try to dig up the actual meaning and make it clearer one way or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like this description, which was pulled from application manifest docs, is the only documentation available on this one. Will follow up separately and try to find out more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

any update on this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not as yet, have reached out to get some clarity on this field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've raised this separately as #491 against the 2.0.0 milestone in order to unblock the PR stack :)

Base automatically changed from v2 to main July 2, 2021 08:40
@manicminer
Copy link
Contributor Author

@katbyte I've added some additional validation to try and avoid applying bad application configurations; could you take another look before merging? Thanks! :)

@manicminer manicminer requested a review from katbyte July 2, 2021 13:47
@manicminer manicminer modified the milestones: v2.0.0, Blocked Jul 4, 2021
@manicminer
Copy link
Contributor Author

manicminer commented Jul 4, 2021

This is unfortunately blocked due to a plugin SDK issue around block handling Resolved with diff suppress func

@manicminer manicminer modified the milestones: Blocked, v2.0.0 Jul 4, 2021
@manicminer
Copy link
Contributor Author

Test results

Screenshot 2021-07-04 at 19 20 08

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

aside from a couple comments LGTM 🍰

@@ -252,6 +349,35 @@ func applicationDataSource() *schema.Resource {
Computed: true,
},

"single_page_application": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is a list shouldn't it be plural?

Suggested change
"single_page_application": {
"single_page_applications": {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block is intended to contain the settings for the application when it's functioning as a single page app (the API container is just spa but I felt it needed expanding)

},

"oauth2_post_response_required": {
Description: "Specifies whether, as part of OAuth 2.0 token requests, Azure AD allows POST requests, as opposed to GET requests.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

any update on this one?

@manicminer manicminer force-pushed the f/application-improvements branch from 2770927 to cdf4f18 Compare July 21, 2021 21:03
@manicminer manicminer merged commit 48f63a6 into main Jul 21, 2021
@manicminer manicminer deleted the f/application-improvements branch July 21, 2021 22:15
manicminer added a commit that referenced this pull request Jul 21, 2021
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.