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

CAMS-283 - Okta adapter #902

Merged
merged 11 commits into from
Sep 19, 2024
Merged

CAMS-283 - Okta adapter #902

merged 11 commits into from
Sep 19, 2024

Conversation

btposey
Copy link
Contributor

@btposey btposey commented Sep 16, 2024

Purpose

Get group membership information from Okta.

  • added UserGroupGateway interface
  • implemented OktaUserGroupGateway implementing UserGroupGateway

Testing/Validation

  • 100% test coverage for OktaUserGroupGateway
  • Tested the adapter against the Flexion dev instance of Okta

@btposey btposey force-pushed the CAMS-283-okta-adapter branch from 3066ce7 to 6197900 Compare September 16, 2024 17:55
@btposey btposey force-pushed the CAMS-283-okta-adapter branch from 81023ed to bb65a3c Compare September 17, 2024 18:49
@btposey btposey force-pushed the CAMS-283-okta-adapter branch from 2dd8872 to 0de51c0 Compare September 18, 2024 19:00
@btposey btposey marked this pull request as ready for review September 18, 2024 21:01
@btposey btposey requested review from a team as code owners September 18, 2024 21:01
fmaddenflx
fmaddenflx reviewed Sep 18, 2024
View reviewed changes
backend/functions/lib/adapters/gateways/okta/okta-user-group-gateway.ts Outdated
Comment on lines 34 to 44
* initialize
*
* Creates an Okta Client instance and retains it in module scope as a singleton.
* Subsequent calls to initialize return the previously created instance.
*
* See: https://github.com/okta/okta-sdk-nodejs?tab=readme-ov-file#oauth-20-authentication
* See: https://github.com/okta/okta-sdk-nodejs?tab=readme-ov-file#known-issues
*
* @param config UserGroupGatewayConfig
* @returns
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed recently that we've been leaving comment blocks like this to describe functions and their inputs/outputs presumably for some documentation generators etc. Is this a practice we should all start utilizing? What is the reasoning behind it?

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 JSDoc. I did use it here to point to relevant Okta documentation since this is an integration.

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 necessarily needed for all functions and "documentation lies" so I don't have a strong opinion to say it is something we should always do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. JSDocs are useful in that they are used by IDE's to provide details when using the function (or class, etc.). They can certainly lie, but it should be more apparent. It also appears that we may be able to add a linter or plugin that should help keep them up-to-date.

Copy link
Contributor

Choose a reason for hiding this comment

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

They have other uses too, but we aren't taking advantage of them.

fmaddenflx
fmaddenflx reviewed Sep 18, 2024
View reviewed changes
Copy link
Contributor

@fmaddenflx fmaddenflx left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few small things to fix or explain away.

@btposey
Fix jsdocs
4c64058 
Jira ticket: CAMS-283
@btposey btposey merged commit 9ba8b0e into main Sep 19, 2024
14 checks passed
@btposey btposey deleted the CAMS-283-okta-adapter branch September 19, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesobrooks jamesobrooks jamesobrooks left review comments

@amorrow-flexion amorrow-flexion amorrow-flexion approved these changes

@fmaddenflx fmaddenflx fmaddenflx 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.
4 participants
@btposey @jamesobrooks @fmaddenflx @amorrow-flexion