Two-Factor Authentication #374
Conversation
|
@megaalex Send PR with single commit and please resolve the conflicts |
alexivanov
force-pushed
the
tfaPhase1WIP
branch
3 times, most recently
from
c98f854
to
134625a
Compare
|
@nazeer1100126 ready for review |
alexivanov
force-pushed
the
tfaPhase1WIP
branch
3 times, most recently
from
6faf07d
to
359ff38
Compare
alexivanov
force-pushed
the
tfaPhase1WIP
branch
2 times, most recently
from
ce00e1a
to
d0c5df1
Compare
| @@ -3041,4 +3041,18 @@ public CommandWrapperBuilder deleteAdHoc(Long adHocId) { | |||
| this.json = "{}"; | |||
| return this; | |||
| } | |||
|
|
|||
| public CommandWrapperBuilder invaldiateTwoFactorAccessToken() { | |||
There was a problem hiding this comment.
invaldiateTwoFactorAccessToken spelling mistake.
| @@ -26,5 +26,4 @@ void sendToUserAccount(String organisationName,String contactName, | |||
| String address, String username, String unencodedPassword); | |||
|
|
|||
| void sendDefinedEmail(EmailDetail emailDetails); | |||
There was a problem hiding this comment.
there is no change in this file , please remove this file
| } | ||
|
|
||
| public static TFAccessToken create(String token, AppUser user, int tokenLiveTimeInSec) { | ||
| DateTime validFrom = new DateTime(); |
There was a problem hiding this comment.
make use of DateUtils to get tenant date & time.
There was a problem hiding this comment.
I opted for using timestamps for the expiry times for connivance as the period is not that long. Is using date time more feasible in this case also?
There was a problem hiding this comment.
@megaalex : Point is tenant timezone and hosted server timezone can be different, so use DateUtils.
| Integer defaultValue = 5; | ||
| Integer value = getIntegerConfig(TwoFactorConfigurationConstants.OTP_TOKEN_LENGTH, | ||
| defaultValue); | ||
| if(value < 1) { |
There was a problem hiding this comment.
instead of checking here for value less than 1 , from getInteger method only return default value
| emailService.sendDefinedEmail(emailData); | ||
| otpRequestRepository.addOTPRequest(user, request); | ||
| return request; | ||
| } else { |
There was a problem hiding this comment.
unnecessary else block.
There was a problem hiding this comment.
Without the else, no error would be returned if a request for OTP is sent for undefined delivery method. Would you like me to do the validation somewhere else?
There was a problem hiding this comment.
I mean to say, you no need to have else block. Directly throw new OTPDeliveryMethodInvalidException();
There was a problem hiding this comment.
Oh, makes perfect sense, yes 👍
| method="GET" requires-channel="https" /> | ||
| <intercept-url pattern="/api/**" access="isFullyAuthenticated()" | ||
| <intercept-url pattern="/api/**" access="isFullyAuthenticated() and hasAuthority('TWOFACTOR_AUTHENTICATED')" |
There was a problem hiding this comment.
for pattern="/api/*/twofactor , why we have only isFullyAuthenticated() access?
for other pattern like pattern="/api/** we have access access="isFullyAuthenticated() and hasAuthority('TWOFACTOR_AUTHENTICATED') ?
There was a problem hiding this comment.
In the implementation I am proposing, I look at 2fa as an authorisation problem. Users that are two-factor authenticated are granted the TWOFACTOR_AUTHENTICATED authority. Endpoints under /twofactor and /twofactor/validate don't require fully authenticated uses, thus no need to check for TWOFACTOR_AUTHENTICATED.
| baseDataValidator.reset().parameter(name).value(value).notNull().trueOrFalseRequired(value); | ||
| } | ||
|
|
||
| private void validateStringParameter(final String name, final JsonElement element, |
There was a problem hiding this comment.
with this approach of validation, we can not validate on length property.
if for any parameter if have max length 50 , then it will hit to database.
There was a problem hiding this comment.
I am not sure that I understand you completely. AFAICS the values are correctly verified for length > 1000. Do we need to verify the keys also as they are hard-coded?
There was a problem hiding this comment.
I mean to say, if i have two string parameters and 100 & 200 is max length allowed for them, it is not possible with this approach right?
There was a problem hiding this comment.
I validate string parameters here because of the 1000 char limit of the column in the 'key-value' table that is used to store the configs. I haven't really implemented per-key length limits because that would be kind of redundant for the set of this config parameters. I validate them only for the 1000 hard column limit. Sorry if that doesn't make sense.
api-docs/api-docs.iml
Outdated
| @@ -0,0 +1,9 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Add iml files to your .gitignore
|
@megaalex does all review comments taken care? If yes, can you send the PR with single commit please. |
alexivanov
force-pushed
the
tfaPhase1WIP
branch
from
817b1d2
to
1a966e8
Compare
|
@nazeer1100126 comments are addressed, commits are squashed. |
|
Hi @megaalex @nazeer1100126 , By when do you think this pull request will be merged ? Any plans of getting otp based authentication ( without using username/password) similar to what uber, whatsapp do. Thanks |
…t-on-savings-account-charges UC-19 Create,Edit and view charge details with min and max capability
This PR was done as part of GSoC 2017 project - Two-Factor Authentication.
Project spec located here.