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.

Sign up for GitHub

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

Jump to bottom

OKTA-289657: Add Factors API Integration Tests #391

Merged
merged 9 commits into from
Jun 17, 2020
Merged

OKTA-289657: Add Factors API Integration Tests #391

merged 9 commits into from
Jun 17, 2020

Conversation

arvindkrishnakumar-okta
Copy link
Contributor

@arvindkrishnakumar-okta arvindkrishnakumar-okta commented Jun 3, 2020 •
edited
Loading

Resolves

OKTA-289657

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2020 •
edited
Loading

Codecov Report

Merging #391 into dev_openapi_v2.0.0_major_rel will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@                        Coverage Diff                         @@
##             dev_openapi_v2.0.0_major_rel     #391      +/-   ##
==================================================================
+ Coverage                           71.09%   71.12%   +0.02%     
  Complexity                           1131     1131              
==================================================================
  Files                                 120      120              
  Lines                                3629     3629              
  Branches                              567      568       +1     
==================================================================
+ Hits                                 2580     2581       +1     
  Misses                                861      861              
+ Partials                              188      187       -1
Impacted Files Coverage Δ Complexity Δ
...com/okta/sdk/impl/config/YAMLPropertiesSource.java 87.17% <0.00%> (+2.56%) 11.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bf67f4...23d1d45. Read the comment docs.

bdemers
bdemers reviewed Jun 4, 2020
View reviewed changes
Copy link
Contributor

@bdemers bdemers left a comment

Choose a reason for hiding this comment

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

Not my call, but TokenUserFactor isn't a great name for a TOTP factor (or any factor) as "token" is a pretty generic term in the security world.

That said, I don't remember what other token types that class supports. IIRC, we had SMS and other factors (which are all token based)

@arvindkrishnakumar-okta
Copy link
Contributor Author

arvindkrishnakumar-okta commented Jun 5, 2020 •
edited
Loading

Not my call, but TokenUserFactor isn't a great name for a TOTP factor (or any factor) as "token" is a pretty generic term in the security world.

That said, I don't remember what other token types that class supports. IIRC, we had SMS and other factors (which are all token based)

Here is the entire list of User Factor types defined in spec:

x-openapi-v3-discriminator: mapping: call: '#/definitions/CallUserFactor' email: '#/definitions/EmailUserFactor' push: '#/definitions/PushUserFactor' question: '#/definitions/SecurityQuestionUserFactor' sms: '#/definitions/SmsUserFactor' token: '#/definitions/TokenUserFactor' 'token:hardware': '#/definitions/HardwareUserFactor' 'token:software:totp': '#/definitions/TotpUserFactor' u2f: '#/definitions/U2fUserFactor' web: '#/definitions/WebUserFactor' propertyName: factorType

So, as per your recommendation, there shouldn't be a TokenUserFactor at all as it is too vague a name (given that we have more specific definitions like HardwareUserFactor, TotpUserFactor et al)? Or even better, we could have TokenUserFactor as the super class for the other token types?

In any case, we would require a spec change to regroup these classes.

@arvindkrishnakumar-okta arvindkrishnakumar-okta force-pushed the okta-289657-add-factors-integration-tests branch from 23d1d45 to 8bf3a34 Compare June 9, 2020 15:50
bdemers
bdemers reviewed Jun 17, 2020
View reviewed changes
integration-tests/src/test/groovy/com/okta/sdk/tests/it/FactorsIT.groovy

TokenUserFactor tokenUserFactor = client.instantiate(TokenUserFactor)
.setFactorType(FactorType.TOKEN_SOFTWARE_TOTP)
.setProvider(FactorProvider.GOOGLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a little overlap with the activateTotpFactor for this one, maybe changing the name to include GoogleTotp would make it more clear (if that is the goal of this test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, yeah that's the idea of this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed test to testGoogleTotpUserFactorCreation.

bdemers
bdemers approved these changes Jun 17, 2020
View reviewed changes
Copy link
Contributor

@bdemers bdemers left a comment

Choose a reason for hiding this comment

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

Take a look at the above comment before merging

@arvindkrishnakumar-okta
Copy link
Contributor Author

Take a look at the above comment before merging

sure, will do.

@arvindkrishnakumar-okta
Copy link
Contributor Author

Take a look at the above comment before merging

sure, will do.

Addressed it.

@arvindkrishnakumar-okta arvindkrishnakumar-okta merged commit b6f0ec9 into dev_openapi_v2.0.0_major_rel Jun 17, 2020
@arvindkrishnakumar-okta arvindkrishnakumar-okta deleted the okta-289657-add-factors-integration-tests branch June 17, 2020 18:05
arvindkrishnakumar-okta added a commit that referenced this pull request Jul 7, 2020
@arvindkrishnakumar-okta @ivanezeigbo-okta @bdemers
OKTA-302317: Release Java Mgmt SDK v2.0.0 (#422)
03cabad 
* upgrade to openapi v2.0.0 (#380)

* openapi spec update - remove 200 response from generateCsrForApplication

* Update OktaOrgCleaner.groovy

* Fix flaky GroupsIT#groupUserOperationsTest test

* Update GroupsIT.groovy

* OKTA-279039: Fix flaky ApplicationsIT associateUserWithApplication test

* Update GroupsIT.groovy

* Addressed review comment

* OKTA-289656: Add Applications API Integration Tests

* OKTA-289655: Add Admin role API Integration Tests

* Open API Csr rename update (#394)

* openapi csr rename update

* Merge pull request #396

* revert csr model rename (#398)

* revert csr model rename

* open api spec update (#401)

* open api spec update

* Open API spec - fix binding enum value for ProtocolEndpoint model (#406)

* OKTA-289662: Add SMS Templates API Integration Tests (#395)

* OKTA-289665: Add Linked Objects Integration Tests (#404)

* Updated Licence Header in LinkedObjectsIT (#410)

* OKTA-289668: Add User Type Integration Tests (#405)

* Open API Update - Sync with #238 #239  (#411)

* sync with open api pr #240 (#413)

* Open API Update - Sync with #241 (#412)

* Open API Update - Sync with #242 (#414)

* Open API Update - Sync with Open API PR 243 (#416)

* OKTA-289657: Add Factors API Integration Tests (#391)

* OKTA-289659: Add Event Hooks Integration Tests (#399)

* OKTA-289659: Add Authorization Server Integration Tests (#402)

* OKTA-289660: Add Inline Hooks Integration Tests (#400)

* Open API Update - Sync with Open API PR 244 (#418)

* fixed copy paste error

* OKTA-289661: Add Feature API Integration Tests (#397)

* Open API Update - Sync with Open API PR 249 (#420)

* OKTA-289663: Add IdP Integration Tests (#409)

* resolved merge conflicts with master changes

* removed Default prefix from few Idp builders

* added swagger code gen check for backward compatibility requirements per review comments

* fixed findbugs error

* updates per swagger regen code and one other update to Idp builder

* fixed expiresAt param type in spec

* deprecated old classes

* refactored InlineHooksIT - deactivate created hooks so it can be cleaned up after test

* Remove deprecated code (#426)

* Remove deprecated code

* fix typo

* fixed PMD violation - unused inports

* Update integration-tests/src/test/groovy/com/okta/sdk/tests/it/CrudTestSupport.groovy

Co-authored-by: Brian Demers <bdemers@apache.org>

* Update integration-tests/src/test/groovy/com/okta/sdk/tests/it/FactorsIT.groovy

Co-authored-by: Brian Demers <bdemers@apache.org>

* Update integration-tests/src/test/groovy/com/okta/sdk/tests/it/UsersIT.groovy

Co-authored-by: Brian Demers <bdemers@apache.org>

* Update integration-tests/src/test/groovy/com/okta/sdk/tests/it/GroupsIT.groovy

Co-authored-by: Brian Demers <bdemers@apache.org>

* minor improvements (#427)

* OKTA-308935: Create Migration Guide for Java Mgmt SDK v2.0.0 (#423)

Co-authored-by: Ivan Ezeigbo <56391095+ivanezeigbo-okta@users.noreply.github.com>
Co-authored-by: Brian Demers <bdemers@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdemers bdemers bdemers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arvindkrishnakumar-okta @codecov-commenter @bdemers