UNIMOODLE / moodle-mod_certifygen

GNU General Public License v3.0
1 stars 1 forks source link

No se permiten otros administradores que no sea el mainadmin #81

Open tmas0 opened 2 days ago

tmas0 commented 2 days ago

Buenas @elena3ip y @xpazv,

En https://github.com/UNIMOODLE/moodle-mod_certifygen/blob/661810e01223cdbea2541a40451ec8fbf77c8248/classes/output/views/activity_view.php#L96

Y en https://github.com/UNIMOODLE/moodle-mod_certifygen/blob/661810e01223cdbea2541a40451ec8fbf77c8248/classes/certifygen.php#L745

Se hace uso de la función is_primary_admin, y dicha función solo devuelve el administrador principal de la plataforma. Usar esta función restringe e impide el normal funcionamiento cuando una persona ha sido nombrada administradora del sitio, por tanto, usando esta función se está restringiendo capacidades que debería tener dicho usuario. Cabe notar que al tener todas las capacidades, la segunda parte que proviene de #76, no aplica, ya que siempre devolverá false en ella.

Por tanto, solicitamos fehacientemente cambiar dicha función por is_siteadmin.

Un saludo

juacas commented 2 days ago

Toni, disculpa que te lleve la contraria, pero tampoco veo adecuado usar is_site_admin() ¿Cuál es el objetivo de comprobar el rol en lugar de comprobar únicamente el capability? Creo que todos los controles de acciones deberían realizarse únicamente con los capabilities. Los administradores tendrán o no tendrán los permisos según se defina en cada site.

Además me preocupa la lógica de: https://github.com/UNIMOODLE/moodle-mod_certifygen/blob/661810e01223cdbea2541a40451ec8fbf77c8248/classes/output/views/activity_view.php#L96

Donde se determina que tiene cualidad de "teacher" si no tiene la capability de certifygen:emitmyactivitycertificate. ¿Cuál es la lógica subyacente y para qué se usa?

En https://github.com/UNIMOODLE/moodle-mod_certifygen/blob/661810e01223cdbea2541a40451ec8fbf77c8248/lang/en/certifygen.php#L45C9-L45C89 se desecribe ese capability como "Issuing certificates in an activity" lo cual es evidente que sí puede hacer un profesor para otros usuarios.

Tenemos que aclarar el alcance y significado de los capabilities y no utilizar roles ni pseudoroles.

juacas commented 2 days ago

En https://github.com/UNIMOODLE/moodle-mod_certifygen/issues/76#issuecomment-2437094313 se añadió el capability mod/certifygen:emitmyactivitycertificate para distinguir a los estudiantes. La descripción del capability en los lang files debe ser algo como "Receive a certificate in an certifygen activity". Creo que es esto lo que se pretende con ese capability.

tmas0 commented 2 days ago

Buenas @juacas,

Estoy de acuerdo con esa línea que propones seguir, en plan teórico lo veo, ahora, aterrizando al problema, un has_capability en modo siteadmin siempre devuelve true, sea en contexto que sea, por tanto, aunque le pongas denegado o prohibido al rol específico que corresponda, pasará de él por esta razón.

Entonces, como workarround al mismo, la opción is_siteadmin la veo más adecuada a la propuesta inicialmente en código original, por la razón que al principio comenté en este ticket.

juacas commented 1 day ago

Correcto, pero ¿para qué sirve la comprobación site_admin? no le veo el sentido. ¿Es para que el siteadmin no pueda emitirse un certificado de una actividad a sí mismo?

Si éste es el caso, lo lógico es comprobar sea que el usuario está válidamente enrolado en el curso, ya que esa emisión no depende del rol sino de las condiciones de disponibilidad condicional.

Mi opinión por orden de preferencia:

1) usar solo condiciones relacioneadas con el problema: capabilities, condiciones de acceso a la actividad y estado de matriculación. 2) mejor is_site_admin que is_primery_admin si es necesario para parchear algún caso patológico detectado (que me gustaría entender).

tmas0 commented 1 day ago

Buenas,

Esa comprobación sirve para, en caso de ser administrador, mostrar todos los certificados de esa actividad, y dar la potestad de poder operar sobre ellos. Sin eso, un administrador no será capaz de ver los certificados emitidos, emitir o bien revocar.

Para evitar esa comprobación, la capacidad debería ser contraria, en lugar de revisar si un estudiante tiene o no esa capacidad, sería otorgar esa capacidad al contrario, que se otorgue a profesores o quien proceda, y en ese caso, en lugar de capar la vista, que sea expandir.

Entiendo que lo ideal es ir por el último camino, pero ello conllevará cambiar la capacidad, el nombre no sirve, y cambiar las comprobaciones. En este caso, sí se podría eliminar la comprobación de administrador.

Un saludo

juacas commented 1 day ago

Correcto. No es adecuado definir una Capability para usarla únicamente en caso de ausencia. Además hay un capability para ese tipo de control.

Es necesario utilizar para esto dos capability de forma que sean:

¿Es correcto @elena3ip ?

elena3ip commented 12 hours ago

@tmas0 gracias por el apunte. Lo cambio por el is_siteadmin()

Os explico el por qué de usar esa comprobación: En la pagina de la actividad debe de salir el listado de alumnos del curso, como no se quiere discriminar por roles y sí por capabilities un administrador que crea un curso aparece en el listado ya que tiene todas las capabilities a true. Para evitar eso, tengo que hacer la doble comprobación que no tenga la capability y que no sea administrador.