-
Notifications
You must be signed in to change notification settings - Fork 1
Feat/OIDC callback #174
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
Feat/OIDC callback #174
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @brosenberg42)
a discussion (no related file):
As discussed, I think adding your script to this doc would help job consumers understand what they need to do on their end.
I think it's fine to include in a breakout section in the doc and mention what you mentioned to me about how it also validates users, but that's not important to what we're trying test by using the script. I would add it to a new "Test callback authentication" section under the section you added.
docs/docs/OpenID-Connect-Guide.md
line 127 at r1 (raw file):
- "Client authentication" must be enabled. - "Standard flow" must be enabled. - "Service accounts roles" must be enabled.
It may be worth mentioning that this is only necessary when supporting callbacks due to how the WFM needs to request a token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @brosenberg42)
docs/docs/OpenID-Connect-Guide.md
line 216 at r2 (raw file):
- Assign the role created in step 2. 4\. Run jobs with the `CALLBACK_USE_OIDC` or `TIES_DB_USE_OIDC` job properties set.
Say "set to true" for clarity.
docs/docs/OpenID-Connect-Guide.md
line 224 at r2 (raw file):
The
Flask-pyoidc
package requires you to configure it to authenticate Web users
This sounds like we're saying that the user of the script needs to do something to configure it. For clarity, let's say:
Note that the script configures the
Flask-pyoidc
package to authenticate Web users, as required by the package, but we are only testing the authentication of REST clients.
Please mention that once the script is running, a user can submit a job via the Workflow Manager Swagger page with the following fields to test callbacks: "callbackMethod": "POST",
"callbackURL": "http://localhost:5000/api",
"jobProperties": {
"CALLBACK_USE_OIDC" : "true"
}, I think it's important to tell the user this next step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @jrobble)
docs/docs/OpenID-Connect-Guide.md
line 216 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Say "set to true" for clarity.
Done.
docs/docs/OpenID-Connect-Guide.md
line 224 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
The
Flask-pyoidc
package requires you to configure it to authenticate Web usersThis sounds like we're saying that the user of the script needs to do something to configure it. For clarity, let's say:
Note that the script configures the
Flask-pyoidc
package to authenticate Web users, as required by the package, but we are only testing the authentication of REST clients.
Done.
docs/docs/OpenID-Connect-Guide.md
line 225 at r2 (raw file):
Previously, jrobble (Jeff Robble) wrote…
Please mention that once the script is running, a user can submit a job via the Workflow Manager Swagger page with the following fields to test callbacks:
"callbackMethod": "POST", "callbackURL": "http://localhost:5000/api", "jobProperties": { "CALLBACK_USE_OIDC" : "true" },I think it's important to tell the user this next step.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)
Issues:
Related PRs:
This change is