indigo-iam / iam

INDIGO Identity and Access Management Service
https://indigo-iam.github.io/
Other
102 stars 43 forks source link

Update login form display strategy #669

Closed Sae126V closed 7 months ago

Sae126V commented 11 months ago

Prevents access to the login form.

Need to prevent access to the login form when admin has decided to disable(Set to false) both localAuthenticationVisible and showLinkToLocalAuthn.

Sae126V commented 11 months ago

Hi, I think the behaviour is already like that, isn't it? See https://github.com/indigo-iam/iam/blob/develop/iam-login-service/src/main/webapp/WEB-INF/views/iam/login.jsp#L76.

Say, When Admin has set loginPageMode to Hidden (Might Happen edge case: Users who know the route link will still be able to access the login form even though admin doesn't want users to enter credentials to login). Does that make sense?

rmiccoli commented 11 months ago

Hi, I think the behaviour is already like that, isn't it? See https://github.com/indigo-iam/iam/blob/develop/iam-login-service/src/main/webapp/WEB-INF/views/iam/login.jsp#L76.

Say, When Admin has set loginPageMode to Hidden (Might Happen edge case: Users who know the route link will still be able to access the login form even though admin doesn't want users to enter credentials to login). Does that make sense?

Ok, clear now. Yes, it makes sense to me.

Sae126V commented 10 months ago

No worries. I thought the DISABLED Case is same as HIDDEN. If it is make no sense. I am happy to close this PR :)

enricovianello commented 10 months ago

Prevents access to the login form.

Need to prevent access to the login form when admin has decided to disable(Set to false) both localAuthenticationVisible and showLinkToLocalAuthn.

My only comment is that I'd change the "title" of this PR/fix. We're not preventing access, we're changing (in a correct way) the logic that hides a form. The login endpoint still login you if you present your right credentials (through a curl e.g.). Then, I'll update the PR title in order to be more clear about this. Something like "Update login form display strategy" e.g.

sonarcloud[bot] commented 10 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

federicaagostini commented 7 months ago

LGTM

sonarcloud[bot] commented 7 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

rmiccoli commented 7 months ago

Hi @Sae126V,

we were reviewing your PR that is fine. Currently, by setting the following properties:

The local authentication is still shown by adding sll=y parameter, but the functionality is disabled (see the attached screenshot). Screenshot from 2024-02-08 17-50-51

Let's decide together which behavior is preferred.