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

GEODE-9933: documentation for authorization expiry #7248

Merged
merged 9 commits into from Jan 19, 2022

Conversation

jmelchio
Copy link
Contributor

@jmelchio jmelchio commented Jan 7, 2022

Describes the addition of throwing AuthenticationExpiredException in SecurityManager.authorize and SecurityManager.authenticate methods along with some additional information on token based authentication.

Copy link
Contributor

@davebarnes97 davebarnes97 left a comment

Choose a reason for hiding this comment

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

@jmelchio Thanks for your contribution to the user guide!
I approve the two example files as-is.
I request a few changes to the implementation files - I've attached a diff containing these changes.
One change is mandatory in each file - please replace Geode with the product_name variable (see diff for syntax).
A discretionary change in the implementing_authentication file would be my suggested re-phrasing (three occurrences) that emphasizes that the token is one alternative, and that the username/password combination is a second alternative. Your choice on this one - read it and see what you think.
GEODE-9933.DIFF.zip

Copy link
Member

@jinmeiliao jinmeiliao left a comment

Choose a reason for hiding this comment

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

We should somehow convey these to the readers:

  1. the Properties returned by the getCredentials call is passed directly to the authenticate call, and the subject returned by the authenticate call is passed directly to the authorize. So getCredentials() --> authenticate() --> authorize(). the output of the previous call is fed to the next. The interface doesn't dictate what should be in the input/output.
  2. When a AuthorizationExpiredException is thrown anywhere in the calling chain, the client will try one more time to call getCredentials again and re-login automatically behind the scene, if the re-try failed, user will then see AuthorizationExpiredException. Bear in mind there is a time gap between getCredentials call on the client and authorize call on the server, so if client returns a credential that's gonna expire in the very near future, even the retry might fail.
  3. limitation of this auto-retry: currently only supported on client/server protocol, and is not supported in event-dispatching (cq and registered interest) in multi-user client mode.

@jmelchio
Copy link
Contributor Author

@jmelchio Thanks for your contribution to the user guide! I approve the two example files as-is. I request a few changes to the implementation files - I've attached a diff containing these changes. One change is mandatory in each file - please replace Geode with the product_name variable (see diff for syntax). A discretionary change in the implementing_authentication file would be my suggested re-phrasing (three occurrences) that emphasizes that the token is one alternative, and that the username/password combination is a second alternative. Your choice on this one - read it and see what you think. GEODE-9933.DIFF.zip

Thanks @davebarnes97, I've applied the patch file and pushed the commit.

@jmelchio
Copy link
Contributor Author

We should somehow convey these to the readers:

1. the `Properties` returned by the `getCredentials` call is passed directly to the `authenticate` call, and the subject returned by the `authenticate` call is passed directly to the `authorize`. So getCredentials() --> authenticate() --> authorize(). the output of the previous call is fed to the next.  The interface doesn't dictate what should be in the input/output.

2. When a `AuthorizationExpiredException` is thrown anywhere in the calling chain, the client will try one more time to call `getCredentials` again and re-login automatically behind the scene, if the re-try failed, user will then see `AuthorizationExpiredException`. Bear in mind there is a time gap between `getCredentials` call on the client and `authorize` call on the server, so if client returns a credential that's gonna expire in the very near future, even the retry might fail.

3. limitation of this auto-retry: currently only supported on client/server protocol, and is not supported in event-dispatching (cq  and registered interest) in multi-user client mode.

@jinmeiliao I will update to add the information mentioned in the first point. I think the second point is covered in the updated docs, and I will update to clarify the thirst point.

@jinmeiliao
Copy link
Member

@jinmeiliao I will update to add the information mentioned in the first point. I think the second point is covered in the updated docs, and I will update to clarify the thirst point.

I think we should dedicate a section for the re-auth feature, outline the process, when the feature is started (1.15.0), what's the behavior on older clients (also for older client, the CQ/register interest client will be disconnected if they have expired credentials ), and those I mentioned in my bullet 3 above.

@davebarnes97
Copy link
Contributor

Thanks @davebarnes97, I've applied the patch file and pushed the commit.
Thanks, @jmelchio - Looks like you'll be making more changes in response to @jinmeiliao's review. I'll hold off on finishing my review until after those changes are in place.

Copy link
Contributor

@davebarnes97 davebarnes97 left a comment

Choose a reason for hiding this comment

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

Almost there.
Good new page on expiry. I edited it in place to make one format tweak: a blank line before the bullet list so it would display as intended.
One more request: Since you've added a new file, it needs an entry in the left-hand navigation source file: /geode-book/master_middleman/source/subnavs/geode-subnav.erb
Attached diff file shows the change.
GEODE-9933-2.DIFF.zip

@davebarnes97
Copy link
Contributor

One more comment for the record:
The retry behavior is a new feature in Geode v1.15, but much of the material relating to tokens was applicable in earlier Geode releases, and could be back-ported to earlier Geode docs after this PR is merged.
I will undertake to create a JIRA ticket for that work after this PR is merged.
Thanks, @jmelchio and @jinmeiliao, for your contributions to the docs!

handle.

Clients older than version 1.15 will also be able to do an automatic reconnect unless the connection
is one of the following types where the exception will always be propagated up the chain:
Copy link
Member

Choose a reason for hiding this comment

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

I think this is misleading. Older client with multi-user auth will still work in regular user operations like put/get etc.

Probably a diagram would explain this better

                       single user ops  |    single user CQ/RI  |  multi user ops  |  multi user CQ/RI
1.15 and later                          |                       |                  |         X
previous                                |             X         |                  |         X

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmelchio Be sure to say "Geode 1.15", as this source file is consumed by other products whose versioning may differ.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davebarnes97 updated the code

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this table shows client version, did we mention anywhere in the doc that the re-authentication feature is only supported by server with Geode 1.15 and higher

Where `AuthInitialize.getCredentials()` provides the `security properties` for `SecurityManager.authenticate()` which
in turn provides the `principal object` for `SecurityManager.authorize()`.

In case of the use of an external token provider we assume that this token provider will be asked for
Copy link
Member

Choose a reason for hiding this comment

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

Emphasize that there would be time gap between the call of getCredential and authorize

- emphasize time passing between calls
jmelchio and others added 2 commits January 18, 2022 16:45
Fleshed-out the table with a heading and “Y” vs “N” so every cell has a value.
Copy link
Contributor

@davebarnes97 davebarnes97 left a comment

Choose a reason for hiding this comment

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

Nice work, @jmelchio . Thanks for your contribution.

@jmelchio jmelchio merged commit 8f7193c into apache:develop Jan 19, 2022
mhansonp pushed a commit to mhansonp/geode that referenced this pull request Mar 11, 2022
* GEODE-9933: documentation for authorization expiry

Co-authored-by: Dave Barnes <daveba@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants