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

OKTA-285607: add authorization servers integration tests #388

Merged
merged 3 commits into from Jun 9, 2020

Conversation

bryanapellanes-okta
Copy link
Contributor

Issue #

OKTA-285607

Code

  • Unit test(s)
  • Integration test(s)
  • Implementation

@bryanapellanes-okta bryanapellanes-okta marked this pull request as ready for review June 4, 2020 19:40
Copy link
Collaborator

@laura-rodriguez laura-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks for all the work here! I left a few comments. Basically, we shouldn't rely so much on certain default settings nor modify them.

Copy link
Collaborator

@laura-rodriguez laura-rodriguez left a comment

Choose a reason for hiding this comment

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

Looking good! Please check my comments. We need to make sure we have aliases for policies, scopes, and claims and use them instead.

@bryanapellanes-okta bryanapellanes-okta force-pushed the OKTA-285607-authorization-servers branch from 78de594 to a5ba442 Compare June 8, 2020 21:34
@bryanapellanes-okta
Copy link
Contributor Author

@laura-rodriguez I updated to use aliases. I ran all the tests locally against my own org and everything passed.

};

var createdAuthorizationServer = await testClient.AuthorizationServers.CreateAuthorizationServerAsync(testAuthorizationServer);
var createdPolicy = await testClient.AuthorizationServers.CreateAuthorizationServerPolicyAsync(testPolicy, createdAuthorizationServer.Id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an alias for CreateAuthorizationServerPolicyAsync?

var createdPolicy = await testClient.AuthorizationServers.CreateAuthorizationServerPolicyAsync(testPolicy, createdAuthorizationServer.Id);
try
{
var policies = await testClient.AuthorizationServers.ListAuthorizationServerPolicies(createdAuthorizationServer.Id).ToListAsync();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an alias for ListAuthorizationServerPolicies?

createdAuthorizationServer.Should().NotBeNull();
createdPolicy.Should().NotBeNull();

var retrievedPolicy = await testClient.AuthorizationServers.GetAuthorizationServerPolicyAsync(createdAuthorizationServer.Id, createdPolicy.Id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an alias for GetAuthorizationServerPolicyAsync?

var testPolicy = new OAuthAuthorizationPolicy
{
Name = $"{SdkPrefix}:Test Policy",
Type = PolicyType.OauthAuthorizationPolicy,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should rename OauthAuthorizationPolicy to OAuthAuthorizationPolicy to be consistent with the policy name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a pre-existing type from a couple years ago. Wasn't sure if changing it would cause issues if it is already referenced outside of what we can see.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have renamed other stuff as well keeping in mind that all SDKs will release a major version.

{
updatedPolicy.Should().NotBeNull();
updatedPolicy.Name.Should().Be($"{SdkPrefix}:Test Policy Updated");
updatedPolicy.Description.Should().Be( "Test policy description updated");
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Fix space after parentheses.

var createdOAuthScope = await createdAuthorizationServer.CreateOAuth2ScopeAsync(testOAuthScope);
try
{
var allAuthorizationServerScopes = await testClient.AuthorizationServers.ListOAuth2Scopes(createdAuthorizationServer.Id).ToListAsync();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have an alias for ListOAuth2Scopes?

createdAuthorizationServer.Should().NotBeNull();
createdOAuthClaim.Should().NotBeNull();
createdOAuthClaim.Name.Should().Be(testOAuthClaim.Name);
var updatedOAuthScope = await testClient.AuthorizationServers.UpdateOAuth2ClaimAsync(testUpdatedOAuthClaim, createdAuthorizationServer.Id, createdOAuthClaim.Id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

DO we have an alias for UpdateOAuth2ClaimAsync?

Copy link
Collaborator

@laura-rodriguez laura-rodriguez left a comment

Choose a reason for hiding this comment

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

LGTM.

@bryanapellanes-okta bryanapellanes-okta merged commit 613b46a into dev Jun 9, 2020
@bryanapellanes-okta bryanapellanes-okta deleted the OKTA-285607-authorization-servers branch June 9, 2020 14:55
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.

None yet

3 participants