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

#1119 email show permission and privacy improvements #1312

Merged
merged 7 commits into from
Aug 12, 2017
Merged

Conversation

hex3l
Copy link
Member

@hex3l hex3l commented Aug 11, 2017

Now the command /email show requires the permission: "authme.player.email.see" and will return a partially hidden email.

E.g.
normal: my.email@example.com
hidden: my.*****@***ple.com
format: 1/3(id lenght) + *** + @ + *** + 2/3(domain lenght)

@hex3l hex3l requested review from ljacqu and sgdc3 August 11, 2017 02:44
@hex3l
Copy link
Member Author

hex3l commented Aug 11, 2017

I've figured out that these changes cb60d7b were useless :')

Sorry for the spam...

@@ -1,5 +1,5 @@
<!-- AUTO-GENERATED FILE! Do not edit this directly -->
<!-- File auto-generated on Sat Apr 29 18:27:38 CEST 2017. See docs/commands/commands.tpl.md -->
Copy link
Member

Choose a reason for hiding this comment

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

Please update permissions.md as well :)

int sid = frag[0].length() / 3 + 1; //Define the id view
int sdomain = frag[1].length() / 3 + 1; //Define the domain view
String id = frag[0].substring(0, sid) + "*****"; //Build the id
String domain = "***" + frag[1].substring(sdomain); //Build the domain
Copy link
Member

Choose a reason for hiding this comment

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

I love the +1 as to guarantee that there will always be a letter visible.
Is there a reason for the asterisks in id vs. domain to be of different length? I might find it less confusing if my email test@example.org became something like te***@***le.org rather than te*****@***le.org.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is not a particular reason, so I'll adopt your solution ;)

@@ -26,9 +26,18 @@
public void runCommand(Player player, List<String> arguments) {
PlayerAuth auth = playerCache.getAuth(player.getName());
if (auth != null && !Utils.isEmailEmpty(auth.getEmail())) {
commonService.send(player, MessageKey.EMAIL_SHOW, auth.getEmail());
commonService.send(player, MessageKey.EMAIL_SHOW, emailMask(auth.getEmail()));
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to make it configurable whether or not the email gets masked. It's not a must for now but I'm willing to bet that someone will create an issue for it in the future.

@@ -17,6 +19,9 @@
public class ShowEmailCommand extends PlayerCommand {

@Inject
private Settings settings;
Copy link
Member

Choose a reason for hiding this comment

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

You can use CommonService#getProperty ;) CommonService just offers the most used stuff from Settings, Messages and PermissionsManager to reduce the number of injected services.

@@ -132,6 +132,15 @@
public static final Property<Integer> EMAIL_RECOVERY_COOLDOWN_SECONDS =
newProperty("Security.emailRecovery.cooldown", 60);

@Comment({
"The maill showed using /email show will be partially hidden",
Copy link
Member

Choose a reason for hiding this comment

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

The mail shown using [...]

" hidden email: mye***@***in.com"
})
public static final Property<Boolean> EMAIL_PRIVACY =
newProperty("Security.privacy.email", false);
Copy link
Member

Choose a reason for hiding this comment

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

I was going to suggest to name it kind of like a question since it's a boolean, but then I started thinking of propositions (MASK_EMAIL, USE_EMAIL_MASKING or HIDE_FULL_EMAIL) and they don't really seem much better xP

@ljacqu
Copy link
Member

ljacqu commented Aug 12, 2017

I'm just nitpicking at this point—given @sgdc3's approval and mine feel free to merge this at any time.

@sgdc3
Copy link
Member

sgdc3 commented Aug 12, 2017

Just fix the wrong message and the service import optimization and we are ready to go! ;)

@sgdc3
Copy link
Member

sgdc3 commented Aug 12, 2017

Ok, let's merge this ;)

@sgdc3 sgdc3 merged commit 1dfb357 into master Aug 12, 2017
@sgdc3 sgdc3 deleted the 1119-email branch August 12, 2017 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants