Store user credentials in cookie/localstorage

[andrewazores]

The user basic auth (base64(username:password)) or bearer (token) credentials should be persisted, such that upon Cryostat web-client reload or new tab opening the user is not required to re-enter their credentials. These credentials must also be persisted in a way that they are unique per Cryostat instance (distinguished by domain) and in a way that they are private to the web-client instance for a given domain (no cross-origin access).

There should also be a corresponding user menu added to the page masthead to allow users to "log out" of the web-client by clearing the stored credentials.

https://www.patternfly.org/v4/components/page/design-guidelines/#masthead

EDIT: actually, in the case that we detect the server is expecting Basic authorization, we should not offer the option to persist these credentials until we have backend support for creating a user session using these credentials. Otherwise we're writing their password to plaintext, which we don't want to do.

Saving the Bearer (JWT) tokens is acceptable, I think, but there should still be a checkbox or something to allow the user to opt-out of having these persisted.

[jan-law]

The UI could save the user's credentials into a cookie or localStorage and have a user menu with logout button, but this just hasn't been implemented.

After looking into cookies vs localStorage vs sessionStorage, I was wondering if you had a preference as to which we should use.
If we use a cookie would we need to notify users?
I know sessionStorage expires when the browser session ends, while localStorage data won't expire. Would it make sense to use sessionStorage to only store the credentials
during the session?

[andrewazores]

sessionStorage is probably the best bet for now, since I think it hardly raises any security concern assuming the user's browser implements it correctly, but it does smooth out the user's experience a fair amount.

Eventually we will want some way to do actual user login sessions for basic auth with expiring session tokens that we can store, rather than directly storing the user's credentials, but that's a longer term plan that includes several changes on the backend as well.

For Bearer auth, where the user is expected to know how to get credentials from the underlying platform and we hook into that auth server, then I think we are OK to persist those with the user's opt-in.

[andrewazores]

If we use a cookie would we need to notify users?

I believe this is only needed for things like tracking cookies, for analytics and advertising, but not required for functionality of simply keeping the user signed in to the application. It would apply for cookies as well as local/session storage, too.

[jan-law]

EDIT: actually, in the case that we detect the server is expecting Basic authorization, we should not offer the option to persist these credentials until we have backend support for creating a user session using these credentials. Otherwise we're writing their password to plaintext, which we don't want to do.

Saving the Bearer (JWT) tokens is acceptable, I think, but there should still be a checkbox or something to allow the user to opt-out of having these persisted.

Revisiting this issue again - since #261 intended to remember Basic auth credentials, would you rather I close #261 and then open a new PR, or modify the existing PR to remember Bearer tokens with a checkbox?

[andrewazores]

I think #261 is okay as it is since the credentials are only in sessionStorage. Enhancing the existing PR to also store the Bearer token into sessionStorage (with a checkbox to enable this) seems reasonable.

[jan-law]

One note I have is that the new Login.service holds state for both a username and a token, but only one of these will ever be in use at a time depending on the auth configuration of the server. Could you find a way to refactor the service and the components that access it so that there is just one piece of stored state that is interpreted as either a username or a token depending on the detected auth method?

As of #261, context.login.username is only used to display the username on the masthead. The Basic auth method stores the encoded username and password in context.login.token.When the backend can return a username from an OpenShift auth request, I was thinking of storing and displaying the username on the masthead just like the Basic auth method.

If I understood you correctly, do you mean I could remove context.login.username and instead re-compute the username from the Basic encoded token?

React.useEffect(() => {
   const sub =
      combineLatest([serviceContext.login.getToken(), serviceContext.login.getAuthMethod()])
      .pipe(distinctUntilChanged())
      .subscribe(parts => {
        const token = parts[0];
        const method = parts[1];
        if(!token || !method) {
          return;
        }

        switch(method) {
          case 'Basic':
            setUsername((Base64.decode(token)).split(':')[0]);
        case 'Bearer':
                    // TODO get username from backend OpenShift auth response?
        default:
          setUsername('');
      }
    });
  return () => sub.unsubscribe();
}, [serviceContext, serviceContext.login]);
...

[andrewazores]

More or less, yes - just one piece of string state that holds a token trivially works for the Bearer case. For the basic case, the user:pass is simple to encode/decode and can be stored in the same piece of state as the Bearer token. Then, anywhere that accesses the token/credentials needs to just be aware of the Basic/Bearer method state and know how to handle the "token" state accordingly. This mainly just means the Login service or Api service need to know about it to turn the "token" state into the correct request headers.

In cryostatio/cryostat#703, POST /api/v2/auth will return not only a 200 status code but also a V2 response object containing the authorized user's username. In the Basic case this just echoes back the user portion of the user:pass that was supplied by the client, whereas in the Bearer case the backend talks to the actual auth server and returns the username back to the requesting client. So, the web-client can simply use POST /api/v2/auth and pull out the returned username from the response regardless of the auth method - the effect hook snippet you posted wouldn't need to handle the auth methods and figure out the username itself. The Login service would just store the username from the POST response so that it can be displayed in the masthead or wherever else it may make sense to use/display.

[jan-law]

POST /api/v2/auth will return not only a 200 status code but also a V2 response object containing the authorized user's username.

Thanks, in that case, I'll assign myself to #317 and work from there

[jan-law]

Does this issue apply to JMX credentials as well? Whenever I refresh the page, I need to enter JMX credentials every time I want to see the Recordings tab.

[andrewazores]

It could, but we should also think about storing JMX credentials server-side. We already do that for automated rules, so it's just a matter of adding a couple of lines of code to enable the same functionality elsewhere.

https://github.com/cryostatio/cryostat/blob/22860afd139e842d81db6a8df539f2fc151fddf3/src/main/java/io/cryostat/net/web/http/AbstractAuthenticatedRequestHandler.java#L117

See also cryostatio/cryostat#565

[andrewazores]

ie when a successful connection is opened then we can store (in memory for now, until we figure out the security requirements for persistent them to disk) those credentials in the CredentialsManager. If we try to open a connection using stored credentials and it fails due to the credentials being incorrect then we should probably automatically remove those stored credentials.

We might also want to build a simple API and UI for listing targets that have stored credentials - without revealing any information about what those credentials are of course - so that users can delete stored credentials graphically. For example, for old targets that they no longer need to connect to, or for targets that have been redeployed and where the user knows in advance that the required credentials have changed.

[andrewazores]

I think this can be closed now that we properly delegate out to the platform OAuth server, right?

[jan-law]

Yes, thanks for checking on this.