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

Update doc to include IS-10 #364

Merged
merged 23 commits into from
Feb 23, 2024
Merged

Conversation

lo-simon
Copy link
Contributor

Update documents and diagrams to include IS-10 implementation.

@garethsb
Copy link
Contributor

garethsb commented Feb 14, 2024

@lo-simon Are the settings.h changes meant to be in this PR? They look related to IS-12, not IS-10.

Edit: Apart from the actual change mentioned in the commit comment "highest_pri and lowest_pri are used in Node only", of course!!

@lo-simon
Copy link
Contributor Author

Thanks, @garethsb, you are right, I must be using the IS-12 branch while doing the document update, I will fix it now.

@lo-simon
Copy link
Contributor Author

Edit: Apart from the actual change mentioned in the commit comment "highest_pri and lowest_pri are used in Node only", of course!!

When Authorization was implemented, highest_pri and lowest_pri were also used for the Authorization API, but then during the implementation phase, I noticed the Authorization API will well be not be running on the Registry, i.e. should not be using the same pri range as the Registration/Query API.

Documents/Architecture.md Show resolved Hide resolved
Documents/Architecture.md Outdated Show resolved Hide resolved
These [instructions](Getting-Started.md) explain how to build nmos-cpp itself, run the test suite, and try out the registry and node applications.
These [instructions](Getting-Started.md) explain how to build nmos-cpp itself, run the test suite, and try out the registry and node applications.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's changed here? 🤔

Documents/Architecture.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This diagram still shows the Device Code Flow grant type in it which should perhaps be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted, I will remove Device Code Flow grant

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit confused by the Authorization Process box at the bottom of the diagram. This contains a Client Credentials Flow box which suggests this is process is infinitely recursive!!!

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 am trying to show once the connection is broken between Auth server & the Node, Node will start over again to fetch Auth server metadata.... and Client Credentials flow

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the Client Credentials diagram, the Authorization Process box at the bottom of the diagram contains an Authorization Code Flow box suggesting an infinite recursion :|

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the "Is required to access protected API?" diamond is asking. Also on "No" it enters the "do authorization_operation" loop which only seems to exit on Failed - is that right?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's a separate process (like the "start authorization code flow" box) should it perhaps be "start authorization_operation loop"? (same applies I guess to the authorization_operation box at the bottom of the diagram)

Copy link
Contributor

@jonathan-r-thorpe jonathan-r-thorpe left a comment

Choose a reason for hiding this comment

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

LGTM

@jonathan-r-thorpe jonathan-r-thorpe merged commit 27dff31 into sony:master Feb 23, 2024
11 checks passed
@jonathan-r-thorpe jonathan-r-thorpe deleted the update-doc branch February 23, 2024 16:17
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.

3 participants