Skip to content

refactor: oidc add scope#2303

Merged
wxg0103 merged 1 commit intomainfrom
pr@main@refactor_oidc
Feb 17, 2025
Merged

refactor: oidc add scope#2303
wxg0103 merged 1 commit intomainfrom
pr@main@refactor_oidc

Conversation

@shaohuzhang1
Copy link
Contributor

refactor: oidc add scope

],
'config_data.clientId': [
{
required: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code contains several issues and could be optimized as follows:

  1. Variable Naming:

    • form is a global variable, which can lead to conflicts if other components have a similar name.
    • Consider refactoring this to use component-level state management like Vuex.
  2. Form Item Labels:

    • The labels for the scope input should include spaces properly to maintain clarity in user interfaces.
  3. Required Validation:

    • Ensure that all fields marked as required indeed have corresponding validation messages, otherwise the system will not display error feedback when the field is left empty.
  4. Code Formatting:

    • The formatting of the code is inconsistent. You might want to standardize it using tools or editor settings.
  5. Comments:

    • Add more comments to explain sections of complex logic to make the code easier to understand.

Here's an improved version of the code:

<template>
  <el-form :model="form.data" :rules="rules" @submit.prevent="handleSubmit">
    <!-- Other form items -->
    <el-form-item label="Scope" prop="config_data.scope">
      <el-input v-model="form.config_data.scope" placeholder="openid profile email" />
    </el-form-item>

    <!-- Continue with other form items -->
  </el-form>
</template>

<script setup lang="ts">
// Import necessary dependencies
import { ref, reactive } from 'vue';
import { mapState, mapActions } from 'vuex'; // Adjust import based on Vue ecosystem

const form = ref({
  data: {
    authEndpoint: '',
    tokenEndpoint: '',
    userInfoEndpoint: '',
    scope: '', // Include the initial default value here (optional)
    clientId: '',
    clientSecret: '',
    redirectUrl: ''
  }
});

const rules = reactive<FormRules>({
  'config_data.authEndpoint': [
    // existing validation rule
  ],
  'config_data.tokenEndpoint': [
    // existing validation rule
  ],
  'config_data.userInfoEndpoint': [
    // existing validation rule
  ],
  'config_data.scope': [
    {
      required: true,
      message: 'Please enter at least one scope',
      trigger: 'blur'
    },
  ],
  'config_data.clientId': [
    {
      required: true,
      message: 'This field is required',
      trigger: 'blur'
    }
  ]
});

function handleSubmit() {
  // Handle form submission logic here
}

// Additional setup functions can be added here using mapState(), mapActions()
</script>

<style scoped>
/* Optional styles */
.el-form {}
</style>

Key Changes:

  • Renamed form to form.data.
  • Improved spacing around inputs and added space character between scopes.
  • Added a comment explaining the need for the scope property being validated to avoid errors.
  • Standardized line endings (\n instead of \r\n) for better readability across different systems.
  • Used modern TypeScript syntax and feature such as setup function along with ref, reactive, etc. This aligns well with Vue 3's composition API practices.

url = `${config.authEndpoint}?client_id=${config.clientId}&redirect_uri=${redirectUrl}&response_type=code&scope=${scope}`
}
if (authType === 'OAuth2') {
url =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code snippet appears to be part of a function redirectAuth that constructs an authorization URL based on the authentication type specified ('OIDC' or 'OAuth2'). There are no glaring issues or optimization opportunities present at first glance:

Issues:

  • The logic for constructing the OAuth2 scope is missing due to a commented-out line.

Optimization Suggestions:

  1. Ensure Proper Scope Handling: Since the comment indicates that the scope should be optional with a default value, explicitly set it using || like const scope = config.scope || 'openid+profile+email';.
        if (authType === 'OIDC') {
          // ... other OIDC stuff here ...
        }
    
        if (authType === 'OAuth2') {
  •    url =
    
  •    const scope = config.scope || 'openid+profile+email';
    
  •    url = `${config.authEndpoint}?client_id=${config.clientId}&redirect_uri=${redirectUrl}&response_type=code&scope=${scope}`;
     }
    
    
    

By including this change, you ensure that if config.scope is not defined, the default 'openid+profile+email' will still be used, improving robustness against cases where the configuration might contain unexpected values.

scopePlaceholder: 'Please enter scope',
clientSecret: 'Client Secret',
clientSecretPlaceholder: 'Please enter client secret',
logoutEndpoint: 'Logout Endpoint',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code is mostly clear and follows standard conventions. However, there are a couple of minor improvements that can be suggested to enhance readability and maintainability:

  1. Consistent Spacing: The line break after scopePlaceholder should match the spacing style used earlier in the file, which currently has spaces before the colons.

    -     scopePlaceholder: 'Please enter scope',
    \ No newline at end of file
    +         scopePlaceholder: 'Please enter scope';
  2. Indentation Consistency: Ensure consistent indentation within the object's properties. In this case, use two spaces per indentation level.

  3. Optional Semi-colon: It's optional to put semi-colons after the last property definition. You can choose whether to add them depending on your coding standards or personal preference.

Here’s the updated version with the above changes applied:

export default {
  userInfoEndpointPlaceholder: 'Please enter user information endpoint',
  clientId: 'Client ID',
  clientIdPlaceholder: 'Please enter client ID',
  scopePlaceholder: 'Please enter scope',
  clientSecret: 'Client Secret',
  clientSecretPlaceholder: 'Please enter client secret',
  logoutEndpoint: 'Logout Endpoint',
};

These adjustments improve the readability and maintainability of the code by ensuring consistency in spacing and formatting.

@wxg0103 wxg0103 merged commit 237dd8c into main Feb 17, 2025
4 checks passed
@wxg0103 wxg0103 deleted the pr@main@refactor_oidc branch February 17, 2025 11:06
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.

2 participants