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

OpenId UI Enhancements #16467

Merged
merged 15 commits into from
Jul 23, 2024
Merged

OpenId UI Enhancements #16467

merged 15 commits into from
Jul 23, 2024

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Jul 22, 2024

I made the following changes in the OpenID module

Applications UI

  • In the Applications list, I sorted the records by application display name.
  • In Both Create and Edit Views:
    • Moved the field Display Name to be the very first field since that is a UI field,
    • Moved the client secret field just below the client id. So the order is Display name, client id then client secret.
    • Added a way to create new random client ID and password. Also a way to copy the values. The values are new UUID values without dashes.
    • Localized the Flow phrase.

Scopes UI

  • Moved the field Display Name to be the very first field since that is a UI field.

Client Settings

  • Moved the Client Secret above the "Implicit Authorization Flow" section where it belongs.

Screenshot of New Buttons

Screenshot_6

@kevinchalet
Copy link
Member

Added a way to create new random client ID and password.

Looks good, but a secret with more entropy than a classical GUID/UUID would be nice. A random 256-bit value generated by a CSP and converted to hexadecimal or base64url would be great 😃

@MikeAlhayek
Copy link
Member Author

@kevinchalet I changed the Client Secret to use 256 hexadecimal password instead of UUID. Happy?

Let me know if you need anything else or if we can merge it?

@kevinchalet
Copy link
Member

Let me know if you need anything else or if we can merge it?

That's good. To nitpick, maybe we shouldn't fall back to the unsafe Math.Rand() when we're not in a secure context and just avoid displaying a "generate secret" button in that case?

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Jul 23, 2024

maybe we shouldn't fall back to the unsafe Math.Rand() when we're not in a secure context and just avoid displaying a "generate secret" button in that case?

@kevinchalet
Why not? At least at that point we are generating someone of a decent random password "even when the context is not secured". Now a ways all website should be secured so this should not be an issue.

Alternatively, we can fallback to using randomUUID() when we are in an in-secure context.

@MikeAlhayek
Copy link
Member Author

@kevinchalet I made some changes fallback on randomUUID() when we are in a none-secure context.

You know what is weird here? Is that I am not sure how is the randomUUID is expected to work in a non-secure context since both crypto.randomUUID() and crypto.getRandomValues() are only available in a secured context. Do you agree?

I think the randomUUID function to work correctly we should change it to use Math.random(). If someone is running an insecure app, then it's on them... can't help them there much.

const randomUUID = (options = {}) => {
    // Default options.
    const defaultOptions = {
        includeHyphens: true
    };

    // Extend the default options with the provided options.
    const config = { ...defaultOptions, ...options };

    let value;
    if (typeof crypto === 'object' && typeof crypto.randomUUID === 'function') {
        value = crypto.randomUUID();
    } else {

        value = ([1e7] + -1e3 + -4e3 + -8e3 + -1e11).replace(/[018]/g, c =>
            (c ^ (Math.random() * 16) >> c / 4).toString(16)
        );
    }

    if (!config.includeHyphens) {
        return value.replaceAll('-', '');
    }

    return value;
}

@MikeAlhayek
Copy link
Member Author

You know what, getRandomValues() is available even if we are in an in-secure context.

https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues

getRandomValues() is the only member of the Crypto interface which can be used from an insecure context.

So I don't think we need to worry about adding the fallback logic.

@kevinchalet
Copy link
Member

Why not? At least at that point we are generating someone of a decent random password "even when the context is not secured". Now a ways all website should be secured so this should not be an issue.

Well, secrets generated via Math.Rand() could be guessed, so using it is not ideal.

You know what is weird here? Is that I am not sure how is the randomUUID is expected to work in a non-secure context since both crypto.randomUUID() and crypto.getRandomValues() are only available in a secured context. Do you agree?

Here's what MDN says:

image

image

So crypto.getRandomValues() should be always supported even if we're not in a secure context, but not crypto.randomUUID() depending on the browser?

@MikeAlhayek
Copy link
Member Author

@kevinchalet funny thing. look at my last comment (few seconds before yours) I think we are on the same page. fixing that now

@MikeAlhayek
Copy link
Member Author

@kevinchalet made the final changes. Can we merge it now?

@kevinchalet
Copy link
Member

@kevinchalet made the final changes. Can we merge it now?

:shipit:

@MikeAlhayek
Copy link
Member Author

:shipit:

I need your approval so I can :shipit:.

Copy link
Member

@kevinchalet kevinchalet left a comment

Choose a reason for hiding this comment

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

🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants